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
panic runtime and C-unwind documentation #1226
base: master
Are you sure you want to change the base?
panic runtime and C-unwind documentation #1226
Changes from 2 commits
7965d3d
d1339c4
6ce66b4
276ff8e
a540310
28b3ffd
621dac9
db97d54
117ca17
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.
You changed the meaning of the text by making this about the final binary. I am not sure whether this is correct. I'd prefer if you could just restore the old wording, which was AFAIK reviewed by a bunch of people, and leave further improvements to a later PR so that the diff over the previous consensus can be more easily reviewed.
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.
In fact I am fairly sure your new wording is wrong. If the unwinding comes from an external crate, enters through a -Cpanic=unwind crate, and then propagates into a -Cpanic=abort crate, that is UB, even if the binary is -Cpanic=unwind.
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.
We certainly talked about the original text a lot, but my impression was that you still considered it confusing. I started the change because it seemed related to the entirely-new "foreign linking" section, but seemed even more confusing in its original form when juxtaposed with the new section.
I believe the rule has always been about whether or not the "abort" runtime is linked, and since only one panic runtime can be linked in a final Rust binary, I don't understand how the introduction of the word "final" changes this. I can remove the word "final", though.
A panic=abort crate will have shims for aborting the process in the presence of unwinding at
extern
boundaries. If there's no extern boundary, then the two crates are compiled as source code so they can't have different panic modes in the first place.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.
That's not right. You can link together different crates with different flags.
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 old text was:
Actually looking at this I think there is a mistake here -- the problematic case is linking a crate with the "unwind" runtime, but compiling it with
panic=abort
, isn't it?panic=abort
sets a bunch of "nounwind" flags, and ultimately what this here is all about is preventing those from causing UB. So for a concrete example:C-unwind
function from a "Rust" ABI functionnounwind
C-unwind
call ever unwindsIt doesn't even matter which panic runtime is linked in, I think? This is about panics from the outside world after all.
The panic runtime would come in in a situation like this:
unwrap
and marks that call withnounwind
since it has the "Rust" ABIunwrap
ever failsNot sure where we say that that is UB.
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.
Would this be correct?
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.
Yes that sounds right. Cc @nbdd0121 to be sure.
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.
But also, don't we have to say somewhere that if a crate is built with panic=abort, and later linked with the panic=unwind runtime, that's UB too?
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.
In my view this is resolved by my later commits, but someone should take a look.
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.
This comment also likely needs updating, since it's not about the panic=abort runtime, it's about mixing panic=abort and panic=unwind crates irrespective of which runtime is ultimately linked in.
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 don't get the last sentence.
Also, this should explain the gap in this rustc check. Clearly there's a gap, otherwise we wouldn't have to document this UB.
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 UB is only possible when using a linker other than rustc (a "foreign linker" as described in the section above), so AFAIK there is no gap in rustc itself.
This sentence explains why the Rust ABI is not included in the lint, which is what you were originally asking about before I changed the verbiage.
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.
Ah, that's existing text so I missed it. It should probably mention
dlopen
-like situations? Even if not mixing Rust an non-Rust code, one may be using a non-rustc linker.The entire section should then probably say:
And then the note can just say that when using rustc as a linker, those requirements are enforced automatically.
This section isn't about the lint though? It's about a hard error on panic option mismatches. That one can't rely on what is "expected" to happen, it should be sound. Did the sentence end up in the wrong paragraph?
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.
That will have to wait for @BatmanAoD then.
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 pushed proposed wording to https://github.com/RalfJung/reference/commits/c-unwind-documentation/. Unfortunately I can't push to this PR.
(EDIT: I deleted that branch and added the commits in this PR as @BatmanAoD gave me access, thanks a lot. :)
I sure hope what that text says is correct, but it would be good if @BatmanAoD or @nbdd0121 could take a look. :)
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 don't have time right now to look, but I've added both you and Gary Guo as collaborators on my fork. I do hope to get confirmation from him because he understands this edge condition better than I do, I think.