update UB list for safe target_feature #1050
Conversation
src/attributes/codegen.md
Outdated
| @@ -133,7 +134,8 @@ Feature | Implicitly Enables | Description | |||
|
|
|||
| #### `wasm32` or `wasm64` | |||
|
|
|||
| This platform allows `#[target_feature]` to be applied to both safe and | |||
| Executing code with unsupported features is allowed (i.e., is not UB) on this platform. | |||
| Hence this platform allows `#[target_feature]` to be applied to both safe and | |||
There was a problem hiding this comment.
This may want to be reworded slightly, since I think we still want to reserve the right to say that if you actually execute undefined instructions that's UB. That being said the target inherently provides no mechanism or means by which to do this.
Put another way "executing code with unsupported feautres" is not necesarily explicitly safe on wasm platforms, it's just not inherently possible in the platforms itself.
Basically I want to leave us room to handle feature detection at some point in the future. Even then though that will be implemented in a way that you won't be able to get it wrong so it won't be unsafe (just sort of inherently with the wasm platform).
I dunno this is definitely sort of lawyer-y, but I think ideally this would be reworded a bit to say that #[target_feature] can be applied to safe functions because it's not possible to execute unknown instructions on wasm.
There was a problem hiding this comment.
This may want to be reworded slightly, since I think we still want to reserve the right to say that if you actually execute undefined instructions that's UB. That being said the target inherently provides no mechanism or means by which to do this.
UB is a Rust concept here, not a target concept. So this is about explaining what you are or are not allowed to do in Rust when compiling to this target.
In Rust it is definitely possible to write code that ends up executing unknown instructions, even when compiling to wasm: just call a target_feature function without appropriate checks. My understanding is that this is guaranteed to trap, but that is not the same as "it's impossible to do so".
There was a problem hiding this comment.
My understanding is that this is guaranteed to trap, but that is not the same as "it's impossible to do so".
AFAIK it causes the wasm module to fail to load at all if the simd instructions are not optimized away. The wasm runtime needs to know about all used instructions in a wasm module to be able to parse and typecheck them.
There was a problem hiding this comment.
Ok, I'll leave this to you then
There was a problem hiding this comment.
To be clear, I am open to suggestions. :)
There was a problem hiding this comment.
@alexcrichton this paragraph got reworded, what do you think about the latest version?
#[target_feature]may be used with both safe and
[unsafefunctions][unsafe function] on Wasm platforms. It is impossible to
cause undefined behavior via the#[target_feature]attribute because
attempting to use instructions unsupported by the Wasm engine will fail at load
time without the risk of being interpreted in a way different from what the
compiler expected.
src/attributes/codegen.md
Outdated
| @@ -133,7 +134,8 @@ Feature | Implicitly Enables | Description | |||
|
|
|||
| #### `wasm32` or `wasm64` | |||
|
|
|||
| This platform allows `#[target_feature]` to be applied to both safe and | |||
| Executing code with unsupported features is allowed (i.e., is not UB) on this platform. | |||
There was a problem hiding this comment.
| Executing code with unsupported features is allowed (i.e., is not UB) on this platform. | |
| Attempting to use instructions unsupported by the wasm engine will fail at load time | |
| without the risk of being interpreted in a way different from what the compiler expected. |
There was a problem hiding this comment.
I think it should explicitly say something about there being no Rust surface UB -- as the UB list says, this is defined per-target, so the targets should explicitly define it.
basically, there's a bool per target whether this is UB or not; we need to say explicitly which value taht bool ahs for wasm.
There was a problem hiding this comment.
I like the way bjorn3 has phrased it here. If you specifically want to make sure that the term "undefined behavior" shows up here, then I think a small adjustment could be made:
#[target_feature]may be used with both safe and unsafe functions on Wasm platforms.
Although it is undefined behavior to execute unsupported instructions, attempting to use instructions unsupported by the Wasm engine will fail at load time without the risk of being interpreted in a way different from what the compiler expected.
There was a problem hiding this comment.
Although it is undefined behavior to execute unsupported instructions
The thing is though that this is not UB. Rust makes it safe to invoke a function with the wrong target_feature on wasm, so if that was UB that would be unsound.
(The UB we care about here is Rust UB. It is the Rust backend responsibility to worry about target UB, and certainly of no concern for the Rust Reference to worry about what is or is not UB in wasm programs.)
There was a problem hiding this comment.
I understand there are two perspectives of UB, but that is a very fine distinction that doesn't come across in the current wording. The statement that "Executing code with unsupported features is allowed on this platform" by itself doesn't come across as being correct to me because the platform does not allow executing code with unsupported features (sort of the exact opposite of what is written). I understand that you're trying to say Rust doesn't care, but it isn't coming across that way to me.
Perhaps another crack at it:
#[target_feature]may be used with both safe and unsafe functions on Wasm platforms.
It is not undefined behavior to use the#[target_feature]attribute because attempting to use instructions unsupported by the Wasm engine will fail at load time without the risk of being interpreted in a way different from what the compiler expected.
There was a problem hiding this comment.
Yes I like that, thanks! I just slightly changed the second sentence.
|
So how do we make progress here? The current state of the reference is in contradiction with target_feature being safe on some platforms. |
Co-authored-by: Eric Huss <eric@huss.org>
RalfJung
force-pushed
the
target_feature
branch
from
95ed6a7
to
01cd6ed
Compare
Update books ## reference 8 commits in 9d289c05fce7254b99c6a0d354d84abb7fd7a032..0a2fe6651fbccc6416c5110fdf5b93fb3cb29247 2022-02-23 08:58:20 -0800 to 2022-03-15 09:32:25 -0700 - Documentation PR for cfg_panic (rust-lang/reference#1157) - Document aarch64 `target_feature` options (rust-lang/reference#1102) - Try to clarify destructor not being run scenario. (rust-lang/reference#1107) - Add undocumented Punctuation token Tilde `~` (rust-lang/reference#1149) - update UB list for safe target_feature (rust-lang/reference#1050) - Update const_eval.md for feature stabilization (rust-lang/reference#1166) - Remove `.intel_syntax`/`.att_syntax` support entirely. - Fix `.intel_syntax` directive ## book 3 commits in 3f255ed40b8c82a0434088568fbed270dc31bf00..036e88a4f135365de85358febe5324976a56030a 2022-02-27 21:26:12 -0500 to 2022-03-04 21:53:33 -0500 - Fix some links and small wordings - Snapshot of chapter 19 for nostarch - Clarify fully-qualified syntax explanation ## rust-by-example 2 commits in 2a928483a20bb306a7399c0468234db90d89afb5..d504324f1e7dc7edb918ac39baae69f1f1513b8e 2022-02-28 11:36:59 -0300 to 2022-03-07 09:26:32 -0300 - Fixed extra indentation at line 43 in Phantom Testcase example. (rust-lang/rust-by-example#1515) - Typo fixed in description of inline ASM cpuid function (rust-lang/rust-by-example#1514) ## rustc-dev-guide 3 commits in 32f2a5b4e7545318846185198542230170dd8a42..0e4b961a9c708647bca231430ce1b199993e0196 2022-03-01 10:45:24 -0600 to 2022-03-14 08:40:37 -0700 - update winget install instructions to ensure proper packages are installed (-e for --exact, and full package names to ensure arbitrary packages from the msstore source aren't installed) - Add missing rustdoc tests explanations - Fix incorrectly escaped backtick
Cc @alexcrichton