-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
rustc: Fill out remaining parts of C-unwind ABI #86155
Conversation
Some changes occurred in diagnostic error codes Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This is surprising (and when I tried to do exactly this around 2-3 years ago, there was a lot of backlash). This is explicitly documented as UB, and the abort-on-unwind shim was supposed to be the right fix here, wasn't it? Or maybe I am misunderstanding what you mean here; what is an example of code that is affected by this?
What does this mean for the operational semantics of MIR itself? Transformation passes are not supposed to change program behavior, so having this as a pass sounds problematic to me. What if other passes want to exploit "unwinding through If we really want to keep this as separate passes, I think we should have some kind of flag in the MIR EDIT:
Looking at the code, I think you mean a different sub than what I thought you meant based on the text... is this the abort-on-unwind that is inserted in a function like extern "C" fn export() {
some_rust_fn_that_might_unwind();
} to ensure that it itself never unwinds, or is this something else? |
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.
Don't remove error codes if unused, update the explanation saying it's not emitted by the compiler anymore.
Oh I'm just following suggestions on zulip for what to do, I don't particularly have a preference right now for whether this changes code today one way or another. Specifically what's affected is: extern "C" fn foo() {
panic!()
} In Rust today that's UB when compiled with
Oh I hadn't considered this, I was just doing something that was easy to implement. I'm happy to move things around or dig into things elsewhere and try to figure out what's going on. Ideally this would happen at the time of MIR construction. Currently MIR construction always creates dtor blocks no matter what, and then they're just optimized away by either LLVM or this pass I'm adding in I'd rather not take on the job of specifying too much about MIR myself here, and I'd ideally prefer to not take on the job of optimizing MIR construction as much as possible. If someone can help me figure out how to migrate this pass to being inherent to building MIR that would be appreciated. |
This comment has been minimized.
This comment has been minimized.
Since E0633 isn't used anymore, you'll need to remove it from the |
@@ -4,7 +4,7 @@ The `unwind` attribute was malformed. | |||
|
|||
Erroneous code example: | |||
|
|||
```compile_fail,E0633 | |||
``` |
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 think it still needs to be marked as compile_fail
, no?
This comment has been minimized.
This comment has been minimized.
This is solved by adding Or maybe I misunderstood which problem this PR is solving. Another soundness issue remains though: #83116. |
That seems like a step in the wrong direction to me. The right fix is to make sure that the function does not unwind. |
Cc @cratelyn @rust-lang/wg-ffi-unwind |
@RalfJung oh hm you may be missing some context? I discussed this changed with wg-ffi-unwind on zulip which has some more info there. I'm well aware that I'm also aware of #83116. Although not directly linked on Zulip or in this PR (I wasn't aware of that issue), this PR fixes that issue. I have proposed a stabilization story for
AFAIK that plan solves all active issues. The primary downside is that there's a window of time between steps 1 and 3 where |
Sorry, your talking about fixing that
Thanks for repeating this for me. :) I am a bit surprised by this plan, because more than 2 years ago I suggested to stop emitting that But, if now people are fine this this, that's great. :) So to be clear, this does not constitute a change in our semantics, we still consider this UB officially. We'll just refrain from exploiting that for now to give the ecosystem some time to adopt `"C-unwind"´. Right? Now it seems the PR is also doing some other things at the same time. (Separate PRs would have made review and discussion a lot easier.) Let me see if I understood those correctly.
pub fn test_exported() {
extern "C-unwind" {
fn test();
}
test();
}
[1] At least I originally though that's what happens, based on the PR description. However, the comments inside the PR say something else: they sound more like this is about the shim that is needed when a |
/// This pass walks walks over all functions calls which may possibly unwind, | ||
/// and if any are found sets their cleanup to a block that aborts the process. | ||
/// This forces all unwinds, in panic=abort mode happening in foreign code, to | ||
/// trigger a process abort. |
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.
To me this sounds like a description of a soundness fix for #83116. But your PR description makes it sound like this just reimplements an existing thing in a different way. Which of the two is true? Or was the soundness bug already fixed before, by the shim that this now re-implements? I am quite confused here.^^
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.
Basically all of that is fixed by this pass. This pass:
- Fixes "C-unwind" ABI is unsound with "-Cpanic=abort" #83116 - there was no logic in the compiler before to insert an aborting edge on the unwind.
- Reimplements the previous abort-on-unwind logic. Previously this was a small tweak during MIR construction, but I backed out that part of MIR construction and instead this pass takes care of it now. This is also the behavior change I mentioned above, where previously the frame's destrutors would run before aborting and now they won't.
Oh to be clear this is just an idea/proposal, I don't think this has been like formally agreed upon or anything like that, I figured that would happen inline here.
I might have actually been one of the proponents back then of keeping My hope is that, yes, Review-wise I found it best to keep everything in one PR since this is basically one fundamental fix of "use the ABI as the source of whether a function can unwind" and then there was fallout from there. For example the function pointer business comes from the fact that all function pointers were asssumed to unconditionally be able to unwind, but the fix in this PR is to no longer make that assumption and instead only look at the ABI to determine whether it's able to unwind or not. Taking into account
This is the purpose of the new MIR pass I added, and your example depends on compilation flags. When compiled with When compiled with More generally what the pass does is that for all functions which cannot unwind, it ensures there's aborting cleanup for all function calls that can unwind. Whether or not something can unwind is dictated by its ABI. The
I'm not expert in MIR and I don't really want to do anything impactful here. This is the first time I've ever interacted with MIR. Sounds like what I did was wrong. I think this makes more sense to happen at MIR construction time, but as I mentioned above I couldn't make heads-or-tails of the code that inserts drops. I expected MIR passes to be like any old compiler pass which just "does something", but if it's considered now as creating a MIR dialect it seems like this would be blocked on me learning how to alter MIR construction instead of doing what this PR currently does. |
It seems that Miri should report that as UB, but it looks like Miri will not because it is still only using |
@hyd-dev That's an interesting call-out. The intent is indeed to make the implementation well-behaved, so it seems unsurprising that Miri would see the implementation as "not UB"; is there any other way we can signal to Miri that something is "formally UB" even though it's implemented in such a way as to ensure the LLVM IR (currently) generated has no UB? |
I think that we can still make Miri complain about it by adding a |
Oh, so this same pass does both the "call-side shim" and the "function-side shim" (in the terms that I defined above)? I thought what would happen for a extern "C" fn foo() { /* a bunch of Rust code */ } is that there'd be a single "catch_unwind"-like construction wrapped around the entire function, that would cause an Okay, I think I finally get why I was so confused. There's a single pass uniformly doing what I thought needed to completely separate things. With that understanding I think I can also describe what the change in MIR semantics is that is effected by this pass: before this pass, when a function call with an unwinding ABI inside a non-unwinding function unwinds, this implicitly aborts execution instead of jumping to the "cleanup" block. After this pass, such calls behave as normal and jump to the cleanup block (and if that cleanup block does not abort, we have UB).
FWIW I would say that even in "any old compiler", this kind of pass introduces a new dialect, and one needs to be careful about which pass works on which dialect. Maybe I am overly paranoid here, and I am certainly biased towards a particular, language-based view on compilers and compiler passes. |
Ok I tried to dig into this more and see what it might look like to simply construct MIR differently rather than running a pass later. My initial goal was to have the MIR, by construction, be the right MIR from the start and not have to really deal with landing pads at all in It appears that we want to generate the same MIR regardless of One thing I also discovered is that the From what I learned it appears the by-construction we don't want to take panic=abort and such into account when constructing MIR (because this could affect later passes). I think this means that nothing can run about simplifying/pruning blocks/etc until after all analysis has run. This is presuambly the
To reiterate I'm very new to MIR so I'm looking to be guided through all these. I'm not comfortable making these decisions entirely alone because I don't really know enough about MIR to be confident in a decision. My feeling is that even if I were to move around this pass in the pass orderings things would still be somewhat fragile in the compiler. The My gut is that a pass like the one I've got here is roughly along the lines of what fits best into rustc. I don't think it fits great (and it could probably fit somewhere else as you mentioned) but other tweaks are probably best left to experience and refactoring things over time (and hoping we have a solid test suite). To answer some of your specific questions you had @RalfJung:
Sort of, yeah, but as you mentioned later it's not actually two shims. The only shim we need is "if your function can't unwind, but you call something that can unwind, we abort on that unwind". For functions where this "shim" is needed (it's not really a shim per-se it's just a basic block ending in One thing that may also be worth pointing out is that one of the major changes in this PR is the structure of the |
☔ The latest upstream changes (presumably #86399) made this pull request unmergeable. Please resolve the merge conflicts. |
That pass looks to me like it operates under the assumption that with The issue is that when Rust code calls C code which unwinds, this assumption is wrong. So if we want to allow that, I think we need to adjust the pass to avoid removing landing pads from function calls which can unwind even with
I'm afraid this is earlier in the MIR pipeline than what I have worked with. It might be a question for @nikomatsakis @wesleywiser or other borrowck experts.
I think this pass can be fixed, as described above.
What are the possible alternatives?
I agree. However, is there another alternative for handling calls to
It should not be too hard to audit the existing passes for this assumption; basically grep for
Yes, I agree what this PR does makes a lot more sense. That's one of the changes I thought of when I said maybe this PR can be split into multiple PRs. But there are probably non-obvious inter-dependencies here. |
5f99c14
to
797e0bd
Compare
💔 Test failed - checks-actions |
Update some `C-unwind` bits and then
@bors: r=nikomatsakis |
📌 Commit bb68c66 has been approved by |
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@25b7648. Direct link to PR: <rust-lang/rust#86155> 💔 miri on windows: test-pass → test-fail (cc @eddyb @oli-obk @RalfJung). 💔 miri on linux: test-pass → test-fail (cc @eddyb @oli-obk @RalfJung).
I've tagged this PR with Before this PR Rust has been unsound since I think forever. Specifically this program exhibits undefined behavior: extern "C" fn foo() { panic!() }
fn main() { foo() } The Rust compiler, prior to this PR, would infer that an The fix in this PR is somewhat nuanced. The blunt explanation is that rustc assumes that The nuance is that this state of affairs is temporary and not desired in the long term. In the long term the compiler will assume that the So, in summary:
The main breakage expected from this PR is perf impact. The compiler no longer assumes |
This commit intends to fill out some of the remaining pieces of the
C-unwind ABI. This has a number of other changes with it though to move
this design space forward a bit. Notably contained within here is:
On
panic=unwind
, theextern "C"
ABI is now considered as "mayunwind". This fixes a longstanding soundness issue where if you
panic!()
in anextern "C"
function defined in Rust that's actuallyUB because the LLVM representation for the function has the
nounwind
attribute, but then you unwind.
Whether or not a function unwinds now mainly considers the ABI of the
function instead of first checking the panic strategy. This fixes a
miscompile of
extern "C-unwind"
withpanic=abort
because that ABIcan still unwind.
The aborting stub for non-unwinding ABIs with
panic=unwind
has beenreimplemented. Previously this was done as a small tweak during MIR
generation, but this has been moved to a separate and dedicated MIR
pass. This new pass will, for appropriate functions and function
calls, insert a
cleanup
landing pad for any function call that mayunwind within a function that is itself not allowed to unwind. Note
that this subtly changes some behavior from before where previously on
an unwind which was caught-to-abort it would run active destructors in
the function, and now it simply immediately aborts the process.
The
#[unwind]
attribute has been removed and all users in tests andsuch are now using
C-unwind
and#![feature(c_unwind)]
.I think this is largely the last piece of the RFC to implement.
Unfortunately I believe this is still not stabilizable as-is because
activating the feature gate changes the behavior of the existing
extern "C"
ABI in a way that has no replacement. My thinking for how to enablethis is that we add support for the
C-unwind
ABI on stable Rust first,and then after it hits stable we change the behavior of the
C
ABI.That way anyone straddling stable/beta/nightly can switch to
C-unwind
safely.