target_feature 1.1 error should print the list of missing target features

[est31]

Given the following code:

#![feature(target_feature_11)]

#[target_feature(enable = "sse2", enable = "avx", enable = "sse")]
pub fn foo() {}

#[target_feature(enable = "sse")]
fn bar() {
    foo();
}

The current output is:

error[E0133]: call to function with `#[target_feature]` is unsafe and requires unsafe function or block
 --> src/main.rs:8:5
  |
8 |     foo();
  |     ^^^^^ call to function with `#[target_feature]`
  |
  = note: can only be called if the required target features are available

The note is easy to be misinterpreted: the function can also be called if the target features are not available, but it requires an unsafe annotation.

I think it would be nice for the note to list the missing target features. In this case, this would be sse2 and avx.
If one or more of the missing target features are enabled in the current build config, like via -C target-feature or -C target-cpu, or because the default is just that, then it should acknowledge this situation and explain that a #[target_feature] is still required.

So I'd imagine something like:

error[E0133]: call to function with `#[target_feature]` is unsafe and requires unsafe function or block
 --> src/main.rs:8:5
  |
8 |     foo();
  |     ^^^^^ call to function with `#[target_feature]`
  |
  = help: in order for the call to be safe, the context requires the following additional target features: sse2, avx
  = note: the sse2 target feature being enabled in the build configuration does not remove the requirement to list it in `#[target_feature]`

---

TLDR: I ask for two things:

1. removing the note in the general case (I think what it contains is already said in the main error msg), but if there are missing target features that are enabled in the build configuration, emit a note to point out that even though they are enabled, this has no influence on #[target_feature] (it's two different systems; see my example for the proposed wording)
2. adding a help that lists the missing target features (ideally if too many like more than 5 are missing, then cut the list to keep the error message short ("avx, sse2, sse, aes, foobar, and 3 more"))

cc #69098

@rustbot label A-diagnostics F-target_feature_11

[LeSeulArtichaut]

This seems like a nice first issue to me, I can mentor someone who would want to pick this up.

Mentoring instructions

The error originates from

rust/compiler/rustc_mir_transform/src/check_unsafety.rs

Lines 363 to 373 in 18caf88

	let callee_features = &self.tcx.codegen_fn_attrs(func_did).target_features;
	// The body might be a constant, so it doesn't have codegen attributes.
	let self_features = &self.tcx.body_codegen_attrs(self.body_did.to_def_id()).target_features;
	// Is `callee_features` a subset of `calling_features`?
	if !callee_features.iter().all(|feature| self_features.contains(feature)) {
	self.require_unsafe(
	UnsafetyViolationKind::General,
	UnsafetyViolationDetails::CallToFunctionWith,
	)
	}

The missing features should probably be added as additional context to UnsafetyViolationDetails::CallToFunctionWith.

The error is then emitted in

rust/compiler/rustc_mir_transform/src/check_unsafety.rs

Lines 557 to 611 in 18caf88

	for &UnsafetyViolation { source_info, lint_root, kind, details } in violations.iter() {
	let (description, note) = details.description_and_note();
	match kind {
	UnsafetyViolationKind::General => {
	// once
	let unsafe_fn_msg = if unsafe_op_in_unsafe_fn_allowed(tcx, lint_root) {
	" function or"
	} else {
	""
	};
	let mut err = struct_span_err!(
	tcx.sess,
	source_info.span,
	E0133,
	"{} is unsafe and requires unsafe{} block",
	description,
	unsafe_fn_msg,
	);
	err.span_label(source_info.span, description).note(note);
	let note_non_inherited = tcx.hir().parent_iter(lint_root).find(|(id, node)| {
	if let Node::Expr(block) = node
	&& let ExprKind::Block(block, _) = block.kind
	&& let BlockCheckMode::UnsafeBlock(_) = block.rules
	{
	true
	}
	else if let Some(sig) = tcx.hir().fn_sig_by_hir_id(*id)
	&& sig.header.is_unsafe()
	{
	true
	} else {
	false
	}
	});
	if let Some((id, _)) = note_non_inherited {
	let span = tcx.hir().span(id);
	err.span_label(
	tcx.sess.source_map().guess_head_span(span),
	"items do not inherit unsafety from separate enclosing items",
	);
	}
	err.emit();
	}
	UnsafetyViolationKind::UnsafeFn => tcx.struct_span_lint_hir(
	UNSAFE_OP_IN_UNSAFE_FN,
	lint_root,
	source_info.span,
	format!("{} is unsafe and requires unsafe block (error E0133)", description,),
	|lint| lint.span_label(source_info.span, description).note(note),
	),
	}
	}

where it can be tweaked as required, through the usual DiagnosticBuilder methods, e.g. note.

@rustbot label: +E-easy +E-mentor

[KittyBorgX]

@rustbot claim

[KittyBorgX]

@LeSeulArtichaut Thank you for the opportunity!
If I am to understand it correctly, I just add a note in the 2nd file ( compiler/rustc_mir_transform/src/check_unsafety.rs) right?

[LeSeulArtichaut]

Basically, yes. Make sure to add it to both the hard error (in UnsafetyViolationKind::General => { /* ... */ }) and the unsafe-op-in-unsafe-fn lint (in UnsafetyViolationKind::UnsafeFn => { /* ... */ }).

[rohaquinlop]

@rustbot claim

[est31]

For greater clarity, I have amended the issue description with a TLDR consisting of a list of the changes I'd like there to be.

[rohaquinlop]

@est31 I'm trying to replicate the same error but I'm getting a different output when running the test. I already tried with different parameters but it's still the same.

#![feature(target_feature_11)]

#[target_feature(enable = "sse2", enable = "avx", enable = "sse")]
pub fn foo() {}

#[target_feature(enable = "sse")]
fn bar() {
    foo();
    //~^ ERROR call to function with #[target_feature] is unsafe and requires unsafe function or block
}

fn main() {
    bar();
}

I'm getting this error messages

error: the feature named `sse2` is not valid for this target
  --> $DIR/issue-108680.rs:3:18
   |
LL | #[target_feature(enable = "sse2", enable = "avx", enable = "sse")]
   |                  ^^^^^^^^^^^^^^^ `sse2` is not valid for this target

error: the feature named `avx` is not valid for this target
  --> $DIR/issue-108680.rs:3:35
   |
LL | #[target_feature(enable = "sse2", enable = "avx", enable = "sse")]
   |                                   ^^^^^^^^^^^^^^ `avx` is not valid for this target

error: the feature named `sse` is not valid for this target
  --> $DIR/issue-108680.rs:3:51
   |
LL | #[target_feature(enable = "sse2", enable = "avx", enable = "sse")]
   |                                                   ^^^^^^^^^^^^^^ `sse` is not valid for this target

error: the feature named `sse` is not valid for this target
  --> $DIR/issue-108680.rs:6:18
   |
LL | #[target_feature(enable = "sse")]
   |                  ^^^^^^^^^^^^^^ `sse` is not valid for this target

error: aborting due to 4 previous errors

And with this file I'm getting the same

#![feature(target_feature_11)]

#[target_feature(enable = "sse2", enable = "avx", enable = "sse")]
pub fn foo() {}

#[target_feature(enable = "sse")]
fn bar() {
    foo();
    //~^ ERROR call to function with #[target_feature] is unsafe and requires unsafe function or block
}

Also, I have a question, I want to "print" some variables during the tests execution but it seems like I need to use debug! but if I use it the files won't compile. Do you have some docs or examples about "printing" during tests so I can look at them? Thank you!

[est31]

@rohaquinlop sse, avx and sse2 are target features for the x86 target family, so likely you have a different target. Are you on an M1/M2? Maybe you could replace the target features with ones for aarch64 then? Example features are neon, aes, crc, mte, ...

Given that aarch64-unknown-linux-gnu is a tier 1 target, you should either make it x86 specific and add a // only-x86_64, or make it aarch64 specific and add a // only-aarch64. for more see the section in the rustc dev guide book.

The error message you quoted is definitely also improvement worthy. Something like "sse is valid for target family abc but current target is of family def" or something. That's a bit harder though to achieve, and a different issue.

[rohaquinlop]

@est31 Ohhh I see, thank you, I'll continue working on it

[rohaquinlop]

@est31 Once I solve this issue we could work on the error message that I was getting

[rohaquinlop]

@est31 I'm having troubles trying to get the variables callee_features and self_features in the function check_unsafety, do you have any ideas how can I do it?.

This is what I'm trying

let callee_features = &tcx.codegen_fn_attrs(def_id).target_features;
let self_features = &tcx.body_codegen_attrs(def_id.to_def_id()).target_features;

But I'm getting the same Vec for both variables. So Basically I don't know how to get the self_features.

[est31]

@rohaquinlop you are passing it the same two def ids, you need the def id for the called function. You should modify the UnsafetyViolationDetails enum, especially the CallToFunctionWith variant, to pass over more data. Either the two sets of callee and self features, or the def id of the function.

[rohaquinlop]

@est31 It's almost done, one thing that I don't understand well is the new note, how can I check which target-features were given in the build config?

Here's how it's looking now.

error[E0133]: call to function with `#[target_feature]` is unsafe and requires unsafe function or block
  --> $DIR/issue-108680.rs:9:5
   |
LL |     foo();
   |     ^^^^^ call to function with `#[target_feature]`
   |
   = help: in order for the call to be safe, the context requires the following additional target features: neon aes sha2 sha3 sm4.
   = note: can only be called if the required target features are available

error: aborting due to previous error

For more information about this error, try `rustc --explain E0133`.

Also, just for testing the output with the limit of 3 features in the help here's how it will look (I know that it's 5 but just to show you)

error[E0133]: call to function with `#[target_feature]` is unsafe and requires unsafe function or block
  --> $DIR/issue-108680.rs:9:5
   |
LL |     foo();
   |     ^^^^^ call to function with `#[target_feature]`
   |
   = help: in order for the call to be safe, the context requires the following additional target features: neon aes sha2 and 2 more.
   = note: can only be called if the required target features are available

error: aborting due to previous error

For more information about this error, try `rustc --explain E0133`.

[est31]

@rohaquinlop That's great progress already!

how can I check which target-features were given in the build config?

I think you could look for the feature in tcx.sess().target_features. If it's not there, it's not enabled in the build config.

[rohaquinlop]

@est31 I Think I found where is the target-feature stored when it's given through the build config which is in this variable &tcx.sess.opts.cg.target_feature, but my question now is how can I test it running the x.py file?

I'm trying in this way, but it's not working, it gives me an error:
./x.py test tests/ui/target-feature/issue-108680.rs --build='--target-feature=neon' --stage 1 --bless --rustc-args --crate-type=lib

[est31]

I Think I found where is the target-feature stored when it's given through the build config

What you gave tracks the command line flags given to rustc directly. That's not what we want: some target-features are enabled by default, and some might be enabled through other cli args like -C target-cpu or such. The user will still expect them to be enabled as that's how it works for cfg(target-feature) so the error should work with the ones that are actually enabled in the end.

Regarding the second, you should add a comment like // compile-flags -C target-feature=+neon to your test file, it will pass that command to the compiler. For more reading, it's explained on the same page of the dev guide that I've linked above.

[rohaquinlop]

@est31 OMG! And I spent all the last days finding how to do it!!! 🤦🏾‍♂️