Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clarify what is UB #149
clarify what is UB #149
Changes from 19 commits
dfb93b2
1dcafde
280761a
86d9e2c
3241c00
909b14c
f59eca2
738a338
0f51082
efb5086
df5ff63
c41d492
1f613d8
c73730b
bd6215e
7386b5c
864625f
64bf0a5
86a89ae
c5778a1
7703c18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really strictly defined to unwind between two rust libraries that are directly calling eachother via
extern "C"
? Otherwise we should have a more precise definition.Specifically it might be that we just need to enumerate that you can only "exit a function via unwinding" if it has one of these 3 rust-specific calling conventions:
I think "rust-intrinsic" isn't supposed to unwind, but I might be wrong on that (perhaps the builtin operator impls have this ABI, somewhere?). But also "rust-intrinsic" kinda doesn't matter since those implementations are only provided by rustc. Only slightly matters if we care about helping devs assume raw re-exported intrinsics never unwind. But making an intrinsic a "normal" function would hardly be considered a breaking change!
Also I'm guessing incompatible compilation flags like mixing panic=abort is subsumed by the "target features" bullet below?
extern "platform-intrinsic" is also a thing but I think not relevant. I'm guessing it has something to do with libc functions that llvm is allowed to make implementation assumptions about, like malloc?)
Also I thought we had automatic guards against unwinding from an extern fn. Is that not the case? (I only work on panic=abort software so I never worry about this issue...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently de facto undefined to unwind out of a Rust function defined as
extern "C"
no matter the caller, because our LLVM IR generation addsnounwind
to such functions which makes it UB to unwind out of. IIUC lang team is saying this is deliberately and explicitly UB until explicitly stated to be supported in some cases, for which there's an ongoing RFC, rust-lang/rfcs#2699. The safeguard that aborts when you try to unwind out of anextern "C"
Rust function did exist but was reverted twice so far because of fallout for software that relied on it despite it being UB, see rust-lang/rust#58794 if you want the messy history and part of the discussion that led to the aforementioned RFC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of our bickering about unions does actually wrap back around to this point:
is
union { a: bool; b: bool; } = 3
"producing an invalid value as a field of a compound type"?(we can probably gloss over this, but it is something to make clearer when we have a better answer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer is "we don't know yet, see rust-lang/unsafe-code-guidelines#73".
So yes, this is a good question, and one that I would prefer we could skip over for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reword for consistency:
str
that isn't valid utf8There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we want to skip this entirely because this is just a library invariant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reword:
NonNull
that is null. (Requesting custom invalid values is an unstable feature, but some stable stdlib types, like NonNull, make use of it.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say "a type with custom invalid values that is one of those values" sounds rather awkward. I don't have a better proposal either, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggested reword and massive clarification:
Many have trouble accepting the consequences of invalid values, so they merit some extra discussion. The claim being made here is a very strong one, so read carefully.
A value is produced whenever it is assigned, passed to something, or returned from something. Keep in mind references get to assume their referents are valid, so you can't even create a reference to an invalid value. Additionally, uninitialized memory is always invalid, so you can't assign it to anything, pass it to anything, return it from anything, or take a reference to it. (Padding bytes are not technically part of a value's memory, and so may be left uninitialized.)
In simple and blunt terms: you cannot ever even suggest the existence of an invalid value. No, it's not ok if you "don't use" or "don't read" the value. Invalid values are instant Undefined Behaviour. The only correct way to manipulate memory that could be invalid is with raw pointers using methods like
write
andcopy
. If you want to leave a local variable or struct field uninitialized (or otherwise invalid), you must use a union or enum which clearly indicates at the type level that this memory may contain no values (see MaybeUninit for details).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied most of your suggestions but this one is big enough that it is probably easier to hand the PR off to you. ;) I'd love to do a pass over what you got when you are done, if you don't mind.
I like this new text, as usual in you very pointed style! One comment though:
That's not true for
MaybeUninit
.