Navigation Menu

Skip to content
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

Merged
merged 6 commits into from Aug 4, 2021

Conversation

alexcrichton
Copy link
Member

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, the extern "C" ABI is now considered as "may
    unwind". This fixes a longstanding soundness issue where if you
    panic!() in an extern "C" function defined in Rust that's actually
    UB 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" with panic=abort because that ABI
    can still unwind.

  • The aborting stub for non-unwinding ABIs with panic=unwind has been
    reimplemented. 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 may
    unwind 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 and
    such 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 enable
this 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.

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2021
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Jun 9, 2021

On panic=unwind, the extern "C" ABI is now considered as "may
unwind". This fixes a longstanding soundness issue where if you
panic!() in an extern "C" function defined in Rust that's actually
UB because the LLVM representation for the function has the nounwind
attribute, but then you unwind.

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?

The aborting stub for non-unwinding ABIs with panic=unwind has been
reimplemented. Previously this was done as a small tweak during MIR
generation, but this has been moved to a separate and dedicated MIR
pass.

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 nounwind is UB", and they run before this pass, then things would go horribly wrong, right?
To my knowledge we currently have exactly 1 semantics-changing MIR pass, and that is drop elaboration. It runs very early. In contrast, your pass runs very late, after all the actual MIR optimizations. I don't think that will work. At the very least, this pass needs to run about the same time drop elaboration runs -- we should get the MIR into a state where it has its proper semantics ASAP. Even then though this is still a big footgun IMO.

If we really want to keep this as separate passes, I think we should have some kind of flag in the MIR Body indicating which MIR this is. Basically, there should be enough information in the MIR body such that if we ran Miri on that MIR, it could figure out exactly what the rules are that apply to this code. Unfortunately, I can't quite figure out what the rules even would be before your new pass. I assume it would be something like "when unwinding hits a function frame that is itself not allowed to unwind, we abort"? (And then after your pass, the same case leads to UB, but the transformation ensures that that UB does not happen -- except if further transformations do things, which can only occur if there was already other UB.)

EDIT:

The aborting stub for non-unwinding ABIs with panic=unwind has been
reimplemented.

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?

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a 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.

@alexcrichton
Copy link
Member Author

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 -Cpanic=unwind because foo is marked as nounwind but the function does indeed actually unwind. This PR changes that program to avoid marking foo as nounwind, which means it's no longer UB. I had an idea about a possible change that continues to emit nounwind for extern "C" { ... } blocks since if anyone relies on this it's probably there, so that's a possibility if desired.

What does this mean for the operational semantics of MIR itself?

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 -Cpanic=abort mode. This means that MIR construction is possibly doing a lot of wasteful work with -Cpanic=abort since we don't actually want to create any cleanup blocks except for the sole "abort me" block. I found the MIR construction in this location very complicated and hard to understand, though, as opposed to writing a MIR pass which turned out to be quite easy.

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.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Since E0633 isn't used anymore, you'll need to remove it from the compile_fail annotations in the code example but otherwise looks good to me. :)

@@ -4,7 +4,7 @@ The `unwind` attribute was malformed.

Erroneous code example:

```compile_fail,E0633
```
Copy link
Member

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?

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Jun 13, 2021

In Rust today that's UB when compiled with -Cpanic=unwind because foo is marked as nounwind but the function does indeed actually unwind.

This is solved by adding feature(c_unwind) which will add some code in that function that catches unwinds and turns them into aborts. So I don't think you have to code anything to fix this issue, we just have to stabilize c_unwind. ;) Cc #84158

Or maybe I misunderstood which problem this PR is solving.

Another soundness issue remains though: #83116.

@RalfJung
Copy link
Member

This PR changes that program to avoid marking foo as nounwind, which means it's no longer UB.

That seems like a step in the wrong direction to me. extern "C" functions are not allowed to unwind, this is UB per the reference.

The right fix is to make sure that the function does not unwind.

@RalfJung
Copy link
Member

Cc @cratelyn @rust-lang/wg-ffi-unwind

@alexcrichton
Copy link
Member Author

@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 feature(c_unwind) makes everything sound, and I'm also well aware that extern "C" is supposed to be nounwind. The c_unwind feature cannot be stabilized instantly as-is because it changes the behavior of programs, notably forcibly aborting on panic=unwind program which unwind past extern "C". According to zulip there's programs that rely on this behavior, and despite it being theoretically UB it's not actually UB for these programs in practice just yet and stabilizing c_unwind would break these programs.

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 c_unwind which does not break existing programs which looks like:

  1. Land this PR. This will change the codegen of extern "C" functions on panic=unwind to assume they might unwind. In other words we'll stop placing nounwind on things. This is to stem the tide of UB and remove a hole in Rust where the example I listed above is 100% safe code yet UB.
  2. Stabilize the extern "C-unwind" ABI. This allows any program using "C" today but wants to use unwinding to move to "C-unwind". At this point in time the "C" and "C-unwind" ABIs are effectively the same.
  3. After "C-unwind" is available on stable/beta/nightly, change the behavior of "C" back to placing nounwind everywhere and catching panics to then abort.

AFAIK that plan solves all active issues. The primary downside is that there's a window of time between steps 1 and 3 where extern "C" is in theory not optimized as well because it's no longer listed as nounwind. I don't know how we might best evaluate this and/or weigh that tradeoff. I also do not know the concrete set of programs which break if extern "C" catches panics and aborts. All-in-all this is, I think, a difficult tradeoff to weigh because both sides are pretty unknown. I'm proposing taking the more conservative relative to runtime error breakage as opposed to time-to-run breakage (aka things go a bit slower).

@RalfJung
Copy link
Member

RalfJung commented Jun 14, 2021

Sorry, your talking about fixing that panic! from an extern "C" lead me down the wrong track.

I have proposed a stabilization story for c_unwind which does not break existing programs which looks like:

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 nounwind (to avoid having so many programs be UB), and there was fierce resistance because we shouldn't bless that pattern of using UB, and we should provide a proper fix instead. I was of a different opinion back then, but was unable to convince T-lang, so nounwind remained.

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.

  • There's something with function pointers? When calling a function pointer we know the ABI, so we should be able to use that to determine if this is allowed to unwind or not. So shouldn't this go through the same match abi { in fn_can_unwind that regular calls go through?
  • There's a fix for "C-unwind" ABI is unsound with "-Cpanic=abort" #83116. How is this fixed? I see that "C-unwind" functions are now allowed to unwind independently of the panic strategy, but what about code like this:
pub fn test_exported() {
  extern "C-unwind" {
    fn test();
  }
  test();
}

test_exported will still have the nounwind flag, and yet it may unwind here?

  • The abort-on-unwind behavior of extern "C" functions (the thing that the c_unwind feature opts-in to) is reimplemented as a MIR pass. [1] As I mentioned, such MIR passes that change the fundamental rules of the language are tricky: effectively each such pass creates a new MIR dialect, and each MIR optimization needs to be audited against each dialect, or we need to ensure that it only runs on the dialect it was designed for. So what are the benefits of doing this as a separate pass, that they outweigh these architectural costs?
    I explained my concern with these dialects in a bit more detail in Better documentation of our MIR dialects #86299. If you truly think a new dialect is the right choice here (I cannot judge that), then I think we should do this pass as early as possible, i.e., roughly at the same time as drop elaboration -- in particular I think the final batch of MIR passes, inside optimized_mir, should not do any dialect transitions.

[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 C-unwind function is called in an abort=panic normal Rust function. Let me call the latter the "call-side shim", and the former (originally implemented by #76570) the "function-side shim". It was my understanding that the call-side shim was not implemented yet, only the function-side shim. But the PR description says it re-implements the call-side shim. I am confused.^^

/// 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.
Copy link
Member

@RalfJung RalfJung Jun 14, 2021

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.^^

Copy link
Member Author

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.

@alexcrichton
Copy link
Member Author

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.

because more than 2 years ago I suggested to stop emitting that nounwind (to avoid having so many programs be UB), and there was fierce resistance because we shouldn't bless that pattern of using UB, and we should provide a proper fix instead

I might have actually been one of the proponents back then of keeping nounwind? If that was the case then I can at least say that the difference now is that there's a concrete thing on the future which will bring nounwind back. AFAIK everyone wants extern "C" to be nounwind in the long run.

My hope is that, yes, extern "C" unwinding is considered "UB" but we'll have a grace period of adaptation where we don't take advantage of the UB. I should point out though that this transition plan I don't think was ever written down until #86155 (comment), so others likely still want to weigh in.


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 CodegenAttrFlags is now meaningless for function pointers since they can't have attributes attached to them.

There's a fix for #83116. How is this fixed?

This is the purpose of the new MIR pass I added, and your example depends on compilation flags. When compiled with panic=abort the outer function cannot unwind, but the inner function call can unwind, so the MIR pass adds a "cleanup" for the inner function which aborts the process.

When compiled with panic=abort the outer function can unwind, and the inner function can unwind, so the MIR pass does nothing.

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 extern "Rust" ABI conditionally is considered to unwind based on the -Cpanic compilation setting.

effectively each such pass creates a new MIR dialect

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.

@ghost
Copy link

ghost commented Jun 14, 2021

yes, extern "C" unwinding is considered "UB"

It seems that Miri should report that as UB, but it looks like Miri will not because it is still only using fn_can_unwind (which will be changed to return true) to determine whether unwinding is UB in this PR?

@BatmanAoD
Copy link
Member

@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?

@ghost
Copy link

ghost commented Jun 15, 2021

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 ctfe: bool (or <const CTFE: bool>) parameter to layout::fn_can_unwind, and making it continue to return false if ctfe is true.

@RalfJung
Copy link
Member

Basically all of that is fixed by this pass.

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 Abort. But actually it seems like what happens is that every function call inside the Rust code that can unwind separately has an Abort inserted for the unwind case?

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).
That doesn't sound too hard to document, so if we add this to #86299 then I think I am fine. However, I also still think this pass should run early, around the same time as drop elaboration -- so, somewhere inside run_post_borrowck_cleanup_passes.

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.

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.

@alexcrichton
Copy link
Member Author

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 -Cpanic=abort mode (since that's in theory wasteful to even generate). I actually fully implemented this and everything was working and was about to polish it for pushing here when I stumbled across this comment.

It appears that we want to generate the same MIR regardless of -Cpanic=abort or not for analysis passes, which makes sense to me. I was a little surprised though that I didn't break anything in the test suite given a comment like that, but in any case this makes me much more wary of modifying the MIR construction to be different than before.

One thing I also discovered is that the no_landing_pads pass is pretty aggressively run in a lot of cases. I had to remove the pass entirely because otherwise it was removing the landing pads in -Cpanic=abort mode that are for catching unwinds from "C-unwind" and then aborting. @RalfJung you've been focusing on dialect shifts in this PR, but it seems like the problem may be more pervasive than this PR? I can try to fix this PR but it seems like that wouldn't help much with any preexising issue...

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 run_post_borrowck_cleanup_passes point you mentioned. This leads me to some questions:

  • What should the MIR look like going into the borrowck passes? Before this PR the only difference in the MIR between panic=abort vs panic=unwind is that the root unwind landing pad block had either a resume or an abort terminator based on the compilation mode (which doesn't really affect analysis that much). Should I leave that in?
  • Should I remove the no_landing_pads pass? If MIR construction leaves in landing pads for -Cpanic=abort they'll otherwise just all get immediately ripped out by the pass. I actually think I got lucky in that my pass here just happened to run later in the compiler.
  • Currently the pass is modeled by, in functions that can't unwind, having all calls that can unwind go to a block that aborts the process. Given all this information above and about dialects, is this still the right model to have?

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 no_landing_pads pass makes me a bit wary of how deeply baked in the assumption of panic=abort equates to zero unwinding is in rustc. For example another surprise that I found when working on this is that I was primarily looking at LLVM IR and was very surprised to see landing pads generated for normal Rust function calls but not for calls to extern "C" functions (in debug mode). It turns out there's an optimization in codegen where if the funtion can't unwind we ignore the unwind edge. This seems like the kind of thing that should be present in the MIR, though, but currently MIR doesn't make any attempt to remove cleanup blocks for function calls which can't unwind.

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:

Oh, so this same pass does both the "call-side shim" and the "function-side shim" (in the terms that I defined above)?

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 abort) there's only one basic block per function with the abort, and any calls which can unwind simply unwind to the aborting basic block. For example if you have an extern "C" function and you call 500 extern "C-unwind" functions, there's only going to be one basic block that aborts, and all of those function calls will unwind to the same basic block.

One thing that may also be worth pointing out is that one of the major changes in this PR is the structure of the fn_can_unwind function. Before this PR that function simply said false in panic=abort mode, claiming that nothing can unwind. That was perhaps the single biggest fix (although subtle) in this PR. By consulting the ABI first it fixes logic in the compiler around dealing with landing pads, invoke, etc.

@bors
Copy link
Contributor

bors commented Jun 17, 2021

☔ The latest upstream changes (presumably #86399) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

@RalfJung you've been focusing on dialect shifts in this PR, but it seems like the problem may be more pervasive than this PR?

That pass looks to me like it operates under the assumption that with PanicStrategy::Abort, the program will never unwind. Under that assumption, the pass is a correct optimization, and not a dialect shift.

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 PanicStrategy::Abort.

What should the MIR look like going into the borrowck passes?

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.

Should I remove the no_landing_pads pass?

I think this pass can be fixed, as described above.

Currently the pass is modeled by, in functions that can't unwind, having all calls that can unwind go to a block that aborts the process. Given all this information above and about dialects, is this still the right model to have?

What are the possible alternatives?
I think your pass can be explained as a dialect shift as sketched above:
"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 ends up doing Resume, we have UB)."

The no_landing_pads pass makes me a bit wary of how deeply baked in the assumption of panic=abort equates to zero unwinding is in rustc.

I agree. However, is there another alternative for handling calls to C-unwind with panic=abort? The only alternatives I see are

  • Simply reject compiling such calls. Probably a no-go.
  • Handle this directly in the codegen backend. That doesn't feel right, either, however.

It should not be too hard to audit the existing passes for this assumption; basically grep for PanicStrategy::Abort.
I should add that while I know a lot about the syntax and semantics of MIR, I am actually not very deeply familiar with what all our MIR passes do. @rust-lang/wg-mir-opt contains the people for that.

One thing that may also be worth pointing out is that one of the major changes in this PR is the structure of the fn_can_unwind function. Before this PR that function simply said false in panic=abort mode, claiming that nothing can unwind.

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.

@bors
Copy link
Contributor

bors commented Aug 3, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 3, 2021
Update some `C-unwind` bits and then
@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 4, 2021

📌 Commit bb68c66 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2021
@bors
Copy link
Contributor

bors commented Aug 4, 2021

⌛ Testing commit bb68c66 with merge 25b7648...

@bors
Copy link
Contributor

bors commented Aug 4, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 25b7648 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2021
@bors bors merged commit 25b7648 into rust-lang:master Aug 4, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 4, 2021
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #86155!

Tested on commit 25b7648.
Direct link to PR: #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).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 4, 2021
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).
@alexcrichton alexcrichton deleted the abort-on-unwind branch August 4, 2021 23:56
@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 5, 2021
@alexcrichton
Copy link
Member Author

I've tagged this PR with relnotes since this is somewhat likely to impact user-facing programs. The main description of this change is that the compiler will infer whether or not a function can unwind primarily based on its ABI. This is mostly relevant for FFI functions, e.g. those using the "C" ABI as-is stable today.

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 extern "C" function can't unwind so it would inform LLVM that the foo function was nounwind, an LLVM attribute that enables certain optimizations on the assumption that the function never unwinds. The function in this case, however, does indeed unwind, which can cause UB. This PR fixes this bug.

The fix in this PR is somewhat nuanced. The blunt explanation is that rustc assumes that extern "C" can unwind for now, unlike before. This means that the nounwind attribute is not applied to the foo function above which means it works correctly. This, however, is likely to prevent optimizations which were previously happening because all extern "C" functions are assumed to "may unwind".

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 "C" ABI cannot unwind. The compiler will unsafely assume this for extern "C" { ... } declared functions, and it will guarantee it for Rust-defined functions by inserting code that aborts the process in case it unwinds. The Rust compiler also supports the "C-unwind" ABI for C functions which actually can unwind, although that ABI is not stable at this time.

So, in summary:

  • This PR fixes a soundness hole
  • The fix is temporary which is to assume that "C" functions can unwind
  • In the future "C-unwind" will be stable and "C" will no longer be assumed to unwind
  • In the future programs will break if they assume unwinding through extern "C"-defined functions in Rust will works. These functions will need to switch to extern "C-unwind"

The main breakage expected from this PR is perf impact. The compiler no longer assumes nounwind for FFI functions, which may impact performance in some situations. At this time there are no known concrete regressions, it's just suspected that this will happen. In the future the compiler will stabilize the "C-unwind" ABI. After the stabilization of the "C-unwind" ABI (and that ABI reaching stable) then the assumption that the "C" ABI cannot unwind will be reimplemented. This will be implemented to ensure that user-defined programs always have a route to working on stable Rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet