Repository navigation
panic in a no-unwind function leads to not dropping local variables #123231
Description
Activity
Nominating for t-lang discussion to get their vibe on this question.
Wow that was years ago. What's the take-away from that discussion?
As pointed out in the discussion, changing how we insert abort to functions is sufficient to change the observed behaviour of the implementation, but the key is to decide what's the allowed behaviour of any implementation.
Options:
- To not call any destructors if panic happens in no-unwind context.
- Is quite desirable and can be helpful for debugging
- This is currently the behaviour for MSVC SEH in cleanup context.
- We probably can't implement this behaviour for all platforms.
- To call all destructors before the unwind.
- Additional code needs to be injected to guarantee this (especially for MSVC SEH cleanup case)
- Questionable value about running all destructors if abort is to happen.
- Leave it unspecified whether destructors will be called at all, or how many call frames are to be unwound before aborting.
- Maximum flexibility for implementation
- Consistent with C++'s
std::terminatespecification. - Can avoid adding landing pads and destructor code if we know for certain abort is about to happen.
Some additional complexity involves a foreign exception. For example, if we have a mixture of stack frames with C++ and Rust then any specification may result in surprises. E.g. when a nounwind frame is introduced by a C++ noexcept function, it's up to C++ personality function to decide whether a Rust panic may trigger C++
std::terminatein phase 1 unwinding before any Rust destructor is called, or trigger in phase 2 after Rust destructor is called and terminate when it reaches C++. If we have a C++ -> Rust -> C++ -> Rust call stack if will mean that we might have some Rust frames have destructors called but not other Rust frames.cc @rust-lang/wg-ffi-unwind
- To not call any destructors if panic happens in no-unwind context.
To not call any destructors if panic happens in no-unwind context.
Judging from prior discussion, "no-unwind context" is a very specific technical term here and not something visible in Rust? Or what exactly do you mean?
Is quite desirable and can be helpful for debugging
Why? If by this you mean the current behavior, I find it undesirable and confusing and thus hindering debugging.
Or do you mean that the panic will somehow predict whether it will during its unwinding hit a no-unwind stack frame and then change behavior early on? That's spooky-action-at-a-distance, so I also don't think that's desirable.
Maximum flexibility for implementation
That's a pretty poor argument IMO, our job is to provide consistent and predictable semantics to our users whenever that is possible with reasonable performance.
Some additional complexity involves a foreign exception.
For this issue I only care about Rust panics.
Let me explain in greater detail, this'll be long..
For unwinding, there are a few type of cases for nounwind/noexcept/whatever:
- A stack frame contains no unwind metadata / reaches end of frame
- Personality function determines the callsite is nounwind and unwinding should terminate
- There's a try/catch and the exception handler calls terminate.
Unwinders can either do single phase unwinding, or do two phase unwinding. For the latter, it do a unwind first without calling any destructors to find the frame that will catch the exception, and then starts an unwind with cleanups. If a catching frame cannot be found, the unwinding process fails at phase 1 (Let's ignore forced unwind for now). The Itanium C++ ABI unwinder has two phases.
So if you have a C code (compiled without unwind tables) that calls into Rust
C-unwindfunction and panics, then it's case (1), and phase 1 will fail. No destructor will be executed.A C++ noexcept function is of case (2) in GCC. With GCC's personality function implementation, the phase 1 will consider a noexcept function as a catching frame, and complete (stop at this frame) without failing. When unwind happens into the noexcept frame, no cleanup is performed and a termination happens immediately (similar to Rust's behaviour today).
A C++ noexcept function is of case (3) in Clang because it doesn't yet support encoding the information to be exposed to personality function. noexcept is codegened as
try {...} catch(...) { terminate() }(roughly). Since it's a catch, phase 1 will stop at the frame. Upon phase 2 reaching this frame, it will executeterminate()without calling any destructors in the final frame (since it's a try/catch and thus terminate is executed with the variables still in-scope, so rightfully the destructor of the last frame is skipped). This is also similar to Rust's behaviour today.There's a subtle difference between (2) and (3) w.r.t to optimisation. Say we have this code:
#include <cstdio> struct D { ~D() { fprintf(stderr, "Drop!\n"); } }; static void foo() { D d; throw ""; } static void bar() noexcept { D d; foo(); } int main() { bar(); }
in both GCC and Clang, you will get one
Drop!print only. If you turn on GCC's optimisation though, you will get noDrop!prints. You still get oneDrop!print with Clang + opt. In no cases you get two prints. They're all acceptable behaviour because C++ spec says in this case it is unspecified whether the destructors are called.The Rust behaviour today is very similar to clang's behaviour in the example.
Now to answer your questions
Or do you mean that the panic will somehow predict whether it will during its unwinding hit a no-unwind stack frame and then change behavior early on? That's spooky-action-at-a-distance, so I also don't think that's desirable.
Yes, I mean this. Since unwind is of two phases, the phase 1 can determine if unwind is possible and can skip phase 2 entirely. It's already the case that Rust panic will cause nounwind at all if it escapes into functions with no unwind tables, or, with MSVC SEH, into cleanup code.
It's helpful for debugging because all the stack frames are intact so you can inspect all frames upon abort. Currently when we unwind, hit an
extern "C"frame, and print a panic-cannot-unwind message. If RUST_BACKTRACE is not enabled, then the first panic prints no stack trace and you already lost the information when abort happens. What could been done is to use phase 1 to figure out that unwind will cause terminate, and then print stack trace, along with an indication of which frame prevents the unwinding from happening.That's a pretty poor argument IMO, our job is to provide consistent and predictable semantics to our users whenever that is possible with reasonable performance.
Given FFI is very important regard to
extern "C"and all unwindable ABIs, I think it's also important to consider consistency with other languages. At detailed above, the implementation today is very consistent with C++ implementation's behaviour.We certainly can define the behaviour to be "all destructors" being executed. It'll simply be requiring adding
try/catcharound allextern "C"functions. However, I am not sure it'll be desirable. When GCC backend is getting better or if LLVM adds support for encoding an "terminate" action, this will prevent us from moving over from case (3) to case (2) which can reduce code size quite significantly.If leaving this unspecified allows better optimisation w.r.t. landing pad sizes, then IMO we should allow such optimisation given that a panic escaping to unwindable FFI interface is very rare and almost always a bug (given abort is imminent). If one wants all destructor to be called, they can very easily implement that behaviour with catch_unwind.
Some additional complexity involves a foreign exception.
This will happen with Rust panic in presence with foreign frames, as well as foreign exception with Rust frames. I think it'll less consistent if our specification of Rust panic behaviour depends on whether a foreign frame is present or not.
- I'll reply in full later, for now I have a clarification question:The Itanium C++ ABI unwinder has two phases.Itanium is dead, why should we care? 2-phase unwinding sounds like a lot of unnecessary complexity to me.^^ But I guess they had their reasons.
The Itanium C++ ABI is the abi used by gcc and clang on most non-windows targets.
The ABI was standardized for IA-64, hense the name, but the spec is used on many platforms.Rust already uses Itanium EH for panics on the same set of targets.
The Itanium C++ ABI unwinder has two phases.
Itanium is dead, why should we care?
Basically every UNIX uses the same unwinder ABI as replacement for the old SjLj unwinder ABI which had non-zero overhead even when not throwing any exceptions. arm32 iOS is the only SjLj target we support(ed).
Thanks for explaining the Itanium thing.
While exploring what C++ does is interesting, I don't think C++ is necessarily a good guiding star to follow. We tend to value cross-platform consistency and predictability much more than C++ does. Having the number of drops depend on the optimization level sounds completely unacceptable to me.
So, ignoring what C++ does -- what are the downsides to saying that consistently, everything must be dropped until the boundary, i.e. even in the last stackframe?
Yes, I mean this. Since unwind is of two phases, the phase 1 can determine if unwind is possible and can skip phase 2 entirely. It's already the case that Rust panic will cause nounwind at all if it escapes into functions with no unwind tables, or, with MSVC SEH, into cleanup code.
What exactly does this mean? You are assuming that I know what all these words mean. :) Can you state this in terms of what the Rust programmer sees as end-to-end behavior?
It's helpful for debugging because all the stack frames are intact so you can inspect all frames upon abort.
That argument applies to all panics. You are suggesting to make debugging better for some small subclass of panics. I don't think it's worth doing this only for "panics that happen to lead to an abort later". In fact I think that makes debugging worse because for some panics you'll see the full stack and for some you won't.
Instead, just set a breakpoint on some symbol inside the panic machinery. (AFAIK we have a dedicated symbol for that?) Or set panic=abort. In both cases the debugger will reliably trap before unwinding begins.
Given FFI is very important regard to extern "C" and all unwindable ABIs, I think it's also important to consider consistency with other languages. At detailed above, the implementation today is very consistent with C++ implementation's behaviour.
I think consistency with C++ is just as often something we explicitly don't want as we disagree with the C++ design philosophy. I also doubt most C++ programmers will even know that this is how C++ behaves, so the consistency only helps those few people that know the ins and outs of how unwinding is implemented.
If leaving this unspecified allows better optimisation w.r.t. landing pad sizes, then IMO we should allow such optimisation given that a panic escaping to unwindable FFI interface is very rare and almost always a bug (given abort is imminent). If one wants all destructor to be called, they can very easily implement that behaviour with catch_unwind.
All panics are always a bug.
The question is whether those landing pad size wins are worth it for the extra confusion that inconsistent behavior will cause. And as I said above I think making this opt-level-dependent is completely inacceptable. That would mean if I see a panic in my release build and then try to debug it in a debug build it will behave completely differently! Maybe it's okay to say that behavior can differ between targets and between Rust versions, but I don't think we want any more variability than that.
77 remaining items
When I say "destructor" I think I mean what you call "drop glue": I am referring to
drop_in_place.yes standard confusion between "destructor=Drop::drop" and "destructor=drop_in_place". Does the reference have a standard for the naming there (I think it does, calling
drop_in_place"destructor" and not givingDrop::dropa special name)?I find everything other than "drop impls are always nounwind" or "double-panics insta-abort" weird in some cases, and "drop impls are always nounwind" and "double-panics insta-abort" have their own disadvantages (tho I'm still not convinced they are not the right solution). But this is a digression.
I think there are two questions here:
- What should we guarantee
- What should the implementation do
Do we have any examples of real code that was depending on the current behavior? As I understand it, the point of this stabilization is to take code that was always technically UB but had no way to be correctly written, so I think we should try to be accommodating here.
I think that "any given instance of unwinding might abort for implementation-defined reasons" is descriptive, but that as long as unwinding does not abort, unwinding control flow should be very well-defined.
And if it's well-defined, I think the version after #129582 is a more obvious control flow than the version in 1.81.
Reacted by Ralf Jung and Crystal DurhamWhat should we guarantee
I think we should guarantee that we run all destructors during unwind, and leave room for "unwind might fail to initiate and abort immediately instead" to account for 2-phase unwinding. That's reasonably easy to understand. This is currently not the case but #129582 implements that, IIUC.
I'm not aware of any code that would rely on the abort happening "early", i.e. skipping some destructors. On current stable, the destructor in the OP example actually does run, so the proposed guarantee (implemented by #129582) is also closer to the status quo than what happens in current beta.
What should the implementation do
It should implement the guarantee. :)
@nbdd0121 what are the downsides of #129582?
Previously, arguments against "guaranteed destructor" semantics included- consistency with C++
- code size
- we may not be able to implement these semantics on all targets
The last point doesn't seem to apply for this PR as it is entirely target-independent. C++ AFAIK allows these destructors to be run, but gcc and clang decide against it. I would guess most C++ programmers are not aware of this, and it seems unlikely anyone would rely on this. Code size of course is increased if we generate more unwinding code, but people that build their code with
-Cpanic=unwindpresumably want that code to be emitted -- we have-Cpanic=abortfor those that want those code size reductions.As long as "unwind might fail to initiate and abort immediately instead" is allowed:
- Last point won't apply
- I think we can still omit landing pads, as long as we change the personality function to ensure that the unwind will always fail to initiate when landing pads are omitted, so code size improvement is still theoretically possible.
I think we can still omit landing pads, as long as we change the personality function to ensure that the unwind will always fail to initiate when landing pads are omitted, so code size improvement is still theoretically possible.
This would be compliant with the specification, but I don't think the code size improvements in that case are worth it unless proven otherwise.
Hasn't this been resolved by rust-lang/reference#1226?
With
panic=unwind, when apanicis turned into an abort by a non-unwinding ABI boundary, either no destructors (Dropcalls) will run, or all destructors up until the ABI boundary will run. It is unspecified which of those two behaviors will happen.The case that led to me opening the issue, where some but not all destructors run, is no longer permitted.
Reacted by Crystal DurhamSounds right to me.
Raised by @CAD97:
Reproducing example:
I would expect "Noisy Drop" to be printed, but it is not.
IMO it'd make most sense to guarantee that with panic=unwind, this destructor is still called. @nbdd0121 however said they don't want to guarantee this.
What is the motivation for leaving this unspecified? The current behavior is quite surprising. If I understand @CAD97 correctly, we currently could make "Noisy Drop" be executed by tweaking the MIR we generate.
Tracking: