rustc: Allow safe #[target_feature] on wasm

[alexcrichton]

This commit updates the compiler's handling of the #[target_feature]
attribute when applied to functions on WebAssembly-based targets. The
compiler in general requires that any functions with #[target_feature]
are marked as unsafe as well, but this commit relaxes the restriction
for WebAssembly targets where the attribute can be applied to safe
functions as well.

The reason this is done is that the motivation for this feature of the
compiler is not applicable for WebAssembly targets. In general the
#[target_feature] attribute is used to enhance target CPU features
enabled beyond the basic level for the rest of the compilation. If done
improperly this means that your program could execute an instruction
that the CPU you happen to be running on does not understand. This is
considered undefined behavior where it is unknown what will happen (e.g.
it's not a deterministic SIGILL).

For WebAssembly, however, the target is different. It is not possible
for a running WebAssembly program to execute an instruction that the
engine does not understand. If this were the case then the program would
not have validated in the first place and would not run at all. Even if
this were allowed in some hypothetical future where engines have some
form of runtime feature detection (which they do not right now) any
implementation of such a feature would generate a trap if a module
attempts to execute an instruction the module does not understand. This
deterministic trap behavior would still not fall into the category of
undefined behavior because the trap is deterministic.

For these reasons the #[target_feature] attribute is now allowed on
safe functions, but only for WebAssembly targets. This notably enables
the wasm-SIMD intrinsics proposed for stabilization in #74372 to be
marked as safe generally instead of today where they're all unsafe due
to the historical implementation of #[target_feature] in the compiler.

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[nagisa]

Marked this as T-lang (@rust-lang/lang) because this kind of change seems mostly a language change, rather than, say, an implementation one (which does look good to me).

[joshtriplett]

Looks good to me!

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[scottmcm]

Hmm, this does seem fine, but leaves me wondering where the edge is for the "WebAssembly which is a 100% safe target at execution time". Is it basically that target_feature is, in a sense, not part of the Rust Abstract Machine? Because I assume that we wouldn't want to allow, say, pointer dereferences in safe code in WASM even though it's a safe target where reading anything in the WASM memory is "fine", in a sense.

[joshtriplett]

@scottmcm It's still Rust's responsibility to make sure you can't invoke UB via safe code. But given the WebAssembly verifier, if your program loads at all, your functions (with or without #[target_feature]) will all be callable, and you can't invoke UB by calling them.

[alexcrichton]

The way I think about this is that Rust still has UB in the sense of raw pointers and such. This UB is exploited by the optimizer so while your final program will have deterministic behavior on wasm it may not be the behavior that you intended. The determinism is just a property of the target, not of the language.

For something like target-specific intrinsics the reason that #[target_feature] requires unsafe, and I believe the only reason, is that a guarantee cannot be provided about what happens when an old CPU executes a future instruction. Targets like x86 (afaik) provide no guarantees in this regard, so your code could actually be UB if you execute it on the wrong computer. Targets like WebAssembly, however, have no need for this restriction, due to the validation propeties of the target. WebAssembly specifically (and no other target) provides the guarantee that all old "cpus" will fault (aka trap) executing some future instruction.

Basically the target_feature attribute has precisely one reason why it's unsafe, and WebAssembly specifically is designed such that the reason does not apply. This does not apply to WebAssembly as a whole because the execution model still includes UB for things like optimizers and such. None of that UB is what target_feature is unsafe, though.

[RalfJung]

For something like target-specific intrinsics the reason that #[target_feature] requires unsafe, and I believe the only reason, is that a guarantee cannot be provided about what happens when an old CPU executes a future instruction.

Currently, target_feature-UB is listed in the reference as regular UB, which means the optimizer could exploit it, so making such things safe would be unsound even on a wasm target. If we want to change this, we need to somehow get a guarantee from LLVM that the optimizer will not exploit target_feature mismatches for optimizations. I think accepting this change must be blocked on that investigation. If/when this change lands, we then also need to adjust the reference and remove or at least qualify the target_feature-UB.

More generally, I am somewhat concerned about having safety depend on the target. Having the same code be safe or unsafe for different targets is very surprising IMO, and also reinforces an incorrect (but widespread) model of UB/unsafe that focuses on "what the target/hardware" does -- which is not an adequate way to think about UB/unsafe.

[jackh726]

Since this is a language change, going to reassign to lang-team.

r? @joshtriplett

[alexcrichton]

I'm happy to update the reference here, but I personally still believe that this should be a safe operation on wasm. All the concrete reasons this is unsafe on x86 and other architectures don't apply to wasm. For example there's no such thing as executing an unknown instruction. There's no such thing as interpreting one instruction as another. These are intrinsic properties of the wasm target.

It's a good point to not push too much into the realm of equating UB and machine behavior, but at the same time I think reasonable adjustments should still be possible. I'm not trying to say "wasm has no UB so Rust should have no unsafe on wasm". I very much understand why that's never what we would want. This very specific feature of wasm is the only thing in question (the #[target_feature] attribute).

I would not want to commit to requiring unsafe here without an example of where things could actually go wrong. All the examples in other threads of why it's unsafe on x86, for example, are not applicable to WebAssembly. To be more specific I would personally want something concrete to point to instead of just sort of being fearful of LLVM and its implementation. My interest in this is stabilizing wasm simd, though, and I do not have time to go learn the entire LLVM backend and deduce that this is safe or not. If that's the requirement then I'd ask @joshtriplett if he's ok removing the concern on the stabilization issue.

[RalfJung]

All the concrete reasons this is unsafe on x86 and other architectures don't apply to wasm. For example there's no such thing as executing an unknown instruction. There's no such thing as interpreting one instruction as another. These are intrinsic properties of the wasm target.

The question is, does LLVM perform optimizations based on "this target flag is not that in that function so that function will never call this code here"? If yes, that would be a reason this is unsafe that applies on all targets, since things happen on the LLVM IR level.

Crudely speaking, LLVM could transform

// Use your favorite wasm target feature instead.
#[target_feature(enable = "sse4")] unsafe fn foo_sse4() {}

fn main() {
  unsafe { foo_sse4() }
}

into

fn main() {
  unsafe { unreachable_unchecked() }
}

If it did, I hope we agree that this would mean we couldn't make such calls safe on wasm.

I don't know if it actually does this, or if it documents that it won't do this. It might well be that neither of these applies, which means we'd ride a dangerous line of relying on unspecified LLVM details -- but potentially we could submit upstream patches to improve the docs and thus establish this as a proper guarantee.
Cc @rust-lang/wg-llvm your expertise is needed here. :)

[alexcrichton]

I can try to answer the question to the best of my ability, but I'm not an LLVM expert in that I'm aware of all of LLVM's design decisions.

The example program above, I think, is an optimization that can never happen. The whole premise of enabling extra target features on some functions is that you can transition to them from other functions that don't have the target features enabled. This can only safely happen if you properly guard first on platforms like x86, but from LLVM's perspective all it will ever see is function main called function foo_sse4. The pattern you listed above is already exactly how simd works on all other platforms (including x86).

I can't really imagine a scenario in which LLVM could legally apply such an optimization. Maybe if LLVM deduced that you didn't properly check for sse4 support then it could do such an optimization, but at that point we're working with an antagonistic instead of an optimizing compiler.

I'm not really sure I can say what needs to be said to satisfy your concerns though @RalfJung. I'm not an LLVM expert in that I'm aware of all of LLVM's historical design decisions. I've worked extensively with LLVM with lots of targets, but all I can say here is my opinion and anecdotal evidence. I don't have the connections (or the time/motivation really) to canvas LLVM at large or other folks at large for what we can do in this situation.

In some sense this to me feels like infinite loops. Infinite loops should never have been UB in LLVM from Rust's perspective, but they were in LLVM. Over time that was fixed in LLVM. Similarly calling future instructions really should not be UB in WebAssembly, it just really doesn't make sense for the target. If we discover something in the future where it's actually UB then I think we should fix that. I don't think we should be beholden to a fear that one day something will accidentally be UB even though you didn't write unsafe in Rust. That to me sounds like a compiler bug that needs fixing rather than something we need to redesign library APIs around. I'm pretty confident that there's no UB today and while I'm not confident that there won't be UB tomorrow I feel like we can fix it when we get there.

[nikomatsakis]

@rfcbot reviewed

I was persuaded by our discussion in today's lang team meeting.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[RalfJung]

I can't really imagine a scenario in which LLVM could legally apply such an optimization. Maybe if LLVM deduced that you didn't properly check for sse4 support then it could do such an optimization, but at that point we're working with an antagonistic instead of an optimizing compiler.

I'm not one for anthropomorphizing compilers -- if LLVM's spec allows this, we might dislike the spec, but I wouldn't call that "antagonistic".

However, chances are the spec doesn't really say either way, so an upstream bugreport asking for spec clarification might be a good idea. I'm not sure about this though, hence me pinging our LLVM crew. :)

If we discover something in the future where it's actually UB then I think we should fix that. I don't think we should be beholden to a fear that one day something will accidentally be UB even though you didn't write unsafe in Rust. That to me sounds like a compiler bug that needs fixing rather than something we need to redesign library APIs around. I'm pretty confident that there's no UB today and while I'm not confident that there won't be UB tomorrow I feel like we can fix it when we get there.

That's totally fair -- basically we're taking a bet here that if actual spec issues come up, we can work with LLVM to have them change/extend their spec so that it works for us. Seems like a reasonable bet to me.
I can't judge the risk of LLVM already doing something like this right now, so I will leave that risk assessment to others. :)

[alexcrichton]

ping @joshtriplett would you be up for reviewing and r+'ing this?

[joshtriplett]

Given that we have lang consensus, and that @nagisa said it looks fine from an implementation perspective:

@bors r+

[bors]

📌 Commit 7fed92b has been approved by joshtriplett

[bors]

⌛ Testing commit 7fed92b with merge 016e9b5...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 016e9b5 to master...