patchable-function-entry: Add unstable compiler flag and attribute

[nebulark]

Tracking issue: #123115

Add the -Z patchable-function-entry compiler flag and the #[patchable_function_entry(prefix_nops = m, entry_nops = n)] attribute.
Rebased and adjusted the canditate implementation to match changes in the RFC.

[rustbot]

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[nebulark]

I am very unsure about the error handling in codegen_attrs.rs

Creating a new error type seemed a bit too much for those errors.
Should I rephrase the errors? Should I add tests for those?

[maurer]

As author of the RFC and the two commits cherry-picked over, this LGTM (there's a bit of grunge in the git history with an unresolved conflict in the rebase, but it gets cleaned up by the last commit).

[maurer]

Why did we switch off name_value_literal()? It looks like you've re-implemented it by first extracting out the symbol manually (even if name_value_literal() doesn't work for some reason, you should consider symbol_or_empty() rather than the first two steps). I do see that you can no longer directly call it on the item and would need to use if let Some((sym, lit)) = item.meta_item().and_then(|mi| mi.name_value_literal()) {, but it seems like it'd be a lot shorter?

[nebulark]

Thanks for suggesting symbol_or_empty! I found name_or_empty, which was probably what you meant and it allowed me to get rid of that ugly span parsing.
I also took the oppurtunity add better error diagnostics, when parsing fails.

[workingjubilee]

The cg_llvm and cg_ssa changes to the actual codegen, and the attendant FileCheck, look good to me. I'd like it if a T-compiler reviewer gave their formal opinion on the diagnostics code given I'm not as familiar with most of the diagnostics code, and the diagnostics in the codegen parts are usually quite... sparse.

[bors]

☔ The latest upstream changes (presumably #126914) made this pull request unmergeable. Please resolve the merge conflicts.

[estebank]

For future reference, the message format doesn't use title-case nor periods. Maybe we should, but let's remain consistent with what we already have:

Suggested change

	tcx.dcx().span_err(item.span(), "Expected name value pair.");
	tcx.dcx().span_err(item.span(), "expected name value pair");

[estebank]

Suggested change

	tcx.dcx().span_err(
	item.span(),
	format!(
	"Unexpected parameter name. Allowed names: {}, {}",
	sym::prefix_nops,
	sym::entry_nops
	),
	);
	tcx.dcx().struct_span_err(
	item.span(),
	"unexpected parameter name"
	).span_label(
	item.span(),
	format!(
	"expected {} or {}",
	sym::prefix_nops,
	sym::entry_nops
	),
	).emit();

[estebank]

Suggested change

	tcx.dcx().span_err(item.span(), "Expected name value pair.");
	tcx.dcx().span_err(item.span(), "expected name value pair");

[estebank]

Suggested change

	tcx.dcx().span_err(
	name_value_lit.span,
	"Expected integer value between 0 and 255.",
	);
	tcx.dcx().struct_span_err(
	name_value_lit.span,
	"integer value out of range",
	).span_label(
	name_value_lit.span,
	"value must be between `0` and `255`",
	).emit();

[nebulark]

This error message would also be triggered when passing in a string, so the suggested wording might be a bit weird in that case. Do you think it makes sense to make the check for the string seperate again or is this just a minor issue?

[estebank]

I think it is worth it to be more accurate whenever possible. you can make this be

Suggested change

	tcx.dcx().span_err(
	name_value_lit.span,
	"Expected integer value between 0 and 255.",
	);
	let mut err = tcx.dcx().struct_span_err(
	name_value_lit.span,
	"invalid literal value",
	);
	if is_numeric {
	err.span_label(
	name_value_lit.span,
	"value must be between `0` and `255`"
	);
	} else if is_str {
	err.span_label(
	name_value_lit.span,
	"some appropriate message"
	);
	}
	// Are there other cases to handle? can we hit this error without a numeric or str literal?
	err.emit();

[estebank]

Suggested change

	tcx.dcx().span_err(attr.span, "Must specify at least one parameter.");
	tcx.dcx().span_err(attr.span, "must specify at least one parameter");

[estebank]

The message in expect needs to be the description of the condition that wasn't met.

Suggested change

	PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total >= prefix")
	PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total < prefix")

[nebulark]

Don't know if I understood you correctly, but to get a value the condition 'total >=prefix' must be true. The panic would happen if that condition is false. So I'd think it would be correct as written?

[estebank]

What will happen if total < prefix is that at runtime we'll get a panic that looks like the following:

thread 'main' panicked at compiler/rustc_interface/src/tests.rs:818:8:
total >= prefix

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7e6fd3fe91aec1a58fdd5b198388ec23

Instead we should provide additional context beyond that. Having said that, this is a nitpick to make it easier for people to understand that happened in this test, it's not a deal breaker.

[workingjubilee]

Suggested change

	PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total >= prefix")
	PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total must be greater than prefix")

[workingjubilee]

maybe?

[workingjubilee]

or just unwrap, this is just a test, really, and the condition is trivially obviously true.

[workingjubilee]

cool, thanks!

@bors r=estebank,workingjubilee rollup

[bors]

📌 Commit 8d246b0 has been approved by estebank,workingjubilee

It is now in the queue for this repository.