-Ctarget-feature is not unsafe (any more)

[RalfJung]

I had no idea we are document -Ctarget-feature as unsafe. But that hasn't been our intent for a while, a lot of effort has been spent to make it safe. We currently still just warn when you do unsafe things, rather than emitting a hard error, but I feel like that warning is a better place to provide guidance and context than a blanket "this is unsafe" without any explanation of what the safety requirements are.

Cc @workingjubilee @Amanieu @bjorn3

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 19 candidates

[bjorn3]

It is still unsafe in that enabling a target feature that doesn't exist on CPU that will run the executable is UB. It isn't limited to trapping, instructions can actually be interpreted in the wrong way. For example lzcnt on x86_64 decodes as rep bsr (where the rep prefix is ignored as useless) on CPUs without BMI1 support.

[RalfJung]

Oh that's what they mean? I see. We should explain that then.
And that would equally apply to -Ctarget-cpu.

I updated the PR.

[asquared31415]

Is the wording "subsume in terms of features" and "have the given features" sufficient? Is there a way for a different target-cpu to have the same set of target features, but still be incompatible in some way, or is everything encoded in "features"? I'm fairly certain that there exist things that aren't necessarily additive features.

[RalfJung]

LLVM has non-additive features but we usually don't expose those as Rust target features.
So I think features should capture everything. We can always adjust this again later if we realize the wording is confusing / incorrect -- it is definitely better than the status quo.

[wesleywiser]

I agree that this wording is better than the previous but I'm not sure declaring these options as unsafe for this reason makes sense to me.

We have to assume to one degree or another that the user will execute the generated code in an environment that is fit for purpose. If I cross compile via --target and then try to run the generated binary on the wrong target, isn't it possible that will cause UB? Even rustc main.rs has the same problem if I copy the output to a different platform and try to run it but there's no arguments provided to that invocation that could realistically be declared "unsafe".

It seems to me we're missing some kind of general disclaimer that output is expected to run within the environment they were compiled for and deviating from that could cause UB. These options could then note that they raise the expectations of the runtime environment higher than they typically are with the same consequences if that expectation is invalid.

View changes since this review

[bjorn3]

If I cross compile via --target and then try to run the generated binary on the wrong target, isn't it possible that will cause UB?

If either the specified or actual target doesn't use ELF, this is effectively guaranteed to result in the kernel complaining about an unsupported executable. If the executable file format doesn't match, that trivially results in an error. If the CPU architecture is wrong, all the kernel will also always complain. And for both PE and Mach-O the header has a field indicating the exact target OS. ELF is the odd one out in that EI_OSABI is ignored by several kernels (including Linux). If the specified or actual target is *-linux-gnu or *-linux-musl and you dynamically link, then you are effectively guaranteed to get an error about the dynamic linker missing as both glibc and musl use a unique name that no other target uses (ld-linux.so.2 and ld-musl-$arch.so.1 respectively).

In any case I think you are more likely to accidentally run an executable compiled with unsupported CPU features than compiled for the wrong OS.

[wesleywiser]

Sure, but none of that applies to free-standing targets right?

[RalfJung]

@wesleywiser what would you suggest we do? Not call this "unsafe" but keep the rest of the new wording? I think we should definitely say that it is UB to run this on a CPU without the given target features. Everything that can cause UB is unsafe so I don't have a strong opinion on whether we explicitly say "this feature is unsafe".