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

Annotate unwind rust #2699

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@BatmanAoD
Copy link

commented May 11, 2019

Rendered

I haven't done this before, so please let me know if I've done anything incorrectly.

@Ixrec

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

This text about how the behavior could change at any time sounds like the very definition of an unstable feature, but it also sounds like this is being proposed for stabilization? That opens a big can of worms around our stability guarantees that ought to be discussed in the RFC. In particular, it seems like we should at least consider the option of making crates that depend on this become unstable, or contain that dependency in an unstable feature or something.

I think we're also missing a summary of why any crate would want to rely on this in the first place. I've probably read it somewhere before, but I've completely forgotten it now, and it's obviously critical to this proposal. Is it as simple as "catch_unwind has a performance cost"?

Show resolved Hide resolved text/XXX-annotate-unwind-rust.md Outdated
Update text/XXX-annotate-unwind-rust.md
Fix link markdown

Co-Authored-By: Joe ST <joe@fbstj.net>
@BatmanAoD

This comment has been minimized.

Copy link
Author

commented May 12, 2019

@lxrec Re: instability, what can change is the implementation of unwinding, not the observable behavior in Rust code. In the guide-level explanation, I mentioned that Rust-to-Rust unwinding will be well-defined. I should probably expand on that paragraph, but there are real use-cases for this. From a comment by @gnzlbg further down in one of those threads:

Doing Rust->Rust via FFI is a reasonable thing to do (e.g., DLLs, hot reloading in games, ...)

As long as the code on both sides of the FFI boundary uses the same compiler, changes to the implementation would not change the observable unwinding behavior.

Regarding the three "on the other hand" quotes you've cited:

  • The issue here is not about allowing people to solve a problem that they can’t solve today....

I found this post pretty confusing at first, but @gnzlbg later clarified:

What works 100% of the time is using an error code in FFI, converting from/to each language error handling mechanism. On functions exposed from Rust to FFI you catch panics and return an error code, and in FFI functions called from rust you raise the error code as a panic. On the other side of FFI, e.g., if it is C++, you catch all exceptions in exposed functions and return error codes, and when calling FFI you raise the error codes as exceptions.

And later acknowledged:

the problem with the C wrappers approach like @joshtriplett mentions is that it is a pain.

  • If you need to "unwind through C code", you either need to compile the C code yourself with a compiler that supports a form of unwinding as an extension, or try to find another way....

Yes, that's why I mentioned -fexceptions (GCC/LLVM) and SEH (MSVC) in the RFC. These are known to be compatible with Rust's current unwinding implementation.

  • I've prototyped a "plan B" of wrapping each FFI call in a C++ try/catch and translating caught errors to C-compatible error codes. It's doable, so if you decide to kill unwinding in 12 weeks, I could reluctantly switch to the workaround.

The problem with this is two-fold. First, I think it's probably not controversial to say that any solution requiring users to add C++ to a Rust/C project would be undesirable. Second, C++ doesn't actually provide strong guarantees than Rust for this; the only reason to use this as a solution would be if Rust implements the unwind-aborts-at-FFI-boundaries feature without providing an opt-out mechanism.

@jcranmer

This comment has been minimized.

Copy link

commented May 13, 2019

The short summary of the state of the affairs:

  • The desired use case is to throw an exception from Rust -> C/C++ -> Rust.
  • extern "C" functions in Rust are (supposed to be) marked nounwind, which means there's a hole in correctness if such a function throws an exception. The solution is to make all such functions abort if they would throw an exception, which is what is supposed to land already but is being held up because it breaks mozjpeg.
  • The only legal way in standard ISO C to throw an exception is to use setjmp/longjmp. On Windows, setjmp and longjmp are implemented using SEH, which is the same mechanism that Rust presently uses to throw exceptions.
  • This means that Rust cannot use setjmp/longjmp to throw exceptions into FFI code. There is therefore no way to throw exceptions out of Rust, so you have to use various forms of status codes and adjustments, which is possible but suboptimal.

What this RFC is proposing--in terms of implementation--is basically to provide a mechanism to make an extern "C" function not get the nounwind attribute and therefore not need abort semantics for safety. This leads to some complications:

  • It's not clear that the implementation legwork is sufficient for safety. One particular concern that has been brought up is the possibility of a Rust exception escaping via extern "C" into Rust code that comes from a different implementation of Rust with not-quite-compatible ABIs.
  • There is some dispute as to whether or not it is actually legal to catch/propagate exceptions between different languages or if it is truly UB and all working code is merely accidentally working. My contention is that this amounts to an ABI issue, so that if you use specific flags to get the compiler to conform to the ABI, it is legal code, but I will concede that this view is not universal.
  • There are some ideas for changing Rust's ABI for unwinding. I don't have a link handy to the idea, but it does mean that there we have to be careful about imposing constraints on the ABI.

Okay, with that out of the way, here are my personal thoughts on the RFC:

I'm definitely in favor of giving us better tools in Rust for interacting with unwinds and FFI exceptions. This RFC doesn't go all the way there, but it doesn't need to do so to solve the more immediate and pressing problem: the minimal implementation change mentioned above sounds like the right change, so long as we have analyzed to make sure that the different Rust ABI situation can be handled in all cases. But I don't think this gives the right semantics to achieve that change.

The property of unwinding to me is a fundamental component of the ABI--not only is there an exception coming via some unwinding side channel, but the mechanics of how that side channel works. This means that it is effectively part of the calling convention, as function pointers also have to know if their target is a nounwind function call or an unwind(Rust) call. I'm not sure encoding such information into an attribute is the right way to do it, but it's also mostly orthogonal to the calling convention that usually goes in the extern "foo" part.

Also, exposing the ABI of Rust that would happen with marking #[unwind(Rust)] extern "C" feels too constrictive. It would be better to have an #[unwind(native)], whose semantics are "this causes unwinds out of this function to use the appropriate native unwinding implementation" (i.e., SEH, Itanium, ARM EH ABI, SjLj, as appropriate). This would mean that Rust must support such an unwinding implementation in perpetuity, even if it chooses to use a different unwinding implementation internally. On the other hand, this RFC as it stands would force any alternate unwinding implementation to have to try to handle the different-Rust-ABIs problem correctly, and probably to somehow keep the backchannel established if there is C code in the middle (while the RFC prohibits the Rust->C->Rust case explicitly, it does try to backdoor it by telling you when it will actually work).

@gnzlbg
Copy link
Contributor

left a comment

The RFC proposes a feature to solve propagating panics in a Rust->X->Rust code chain, where X is written in a programming language that is somehow able to propagate Rust panics properly. The feature proposed does a very poor job at solving this problem.

This feature could be useful to solve other problems, e.g., like allowing those that want to link Rust code dynamically (Rust->Rust with the same toolchain), e.g., in a dll, using the C ABI, to be able to use Rust panics.

# Summary
[summary]: #summary

A new function annotation, `#[unwind(Rust)]`, will be introduced; it will

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

nit: use present tense - this RFC introduces...


A new function annotation, `#[unwind(Rust)]`, will be introduced; it will
explicitly permit `extern` functions to unwind (`panic`) without aborting. No
guarantees are made regarding the specific unwinding implementation.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

We do make some guarantees though. For example, when using a particular toolchain to compile Rust code, all extern functions and extern function declarations with this attribute are assumed to use the same panic implementation.

[summary]: #summary

A new function annotation, `#[unwind(Rust)]`, will be introduced; it will
explicitly permit `extern` functions to unwind (`panic`) without aborting. No

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

Only extern "ABI" functions ? (all ABIs ? or which ones). Shouldn't this also be supported on extern function declarations (extern { fn-decl }) ?

around the `libpng` and `libjpeg` C libraries) that make use of the current
implementation's behavior. The proposed annotation would act as an "opt out" of
the safety guarantee provided by aborting at an FFI boundary.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

These library authors decided to rely on an implementation detail of undefined behavior, they knew about it, and have been actively blocking the fix of soundness hole in the language without putting in any work towards a solution.

If anything, this is probably the strongest argument against this RFC. It sends the message that it is ok to rely on UB, exploit implementation details, etc. knowingly as long as you complain loud enough when things break because then the language will be bent in your favor.

Calling it an ""opt out" of a safety guarantee" does not help.


Honestly I don't think even mentioning this would do this RFC a favor. From the use cases discussed, the Rust-Rust dynamic linking one felt like the only one that could motivate this. It might be better to focus on that instead.

This comment has been minimized.

Copy link
@BatmanAoD

BatmanAoD May 13, 2019

Author

I didn't realize you felt this way about the specific impetus for the request and the proposal itself; I had thought, from you last comments in the issue thread, that you were in agreement that this was a reasonable request and a viable proposal.

I disagree with your perspective on the UB issue and am willing to have that conversation, but I'm now wondering if that discussion should be continued back on the internals thread so that it doesn't make this PR too noisy. If I had understood that we didn't yet have agreement on the way forward, I wouldn't have opened this PR.

This comment has been minimized.

Copy link
@BatmanAoD

BatmanAoD May 13, 2019

Author

(By "the UB issue" I don't mean expecting LLVM not to use nounwind for optimization; I agree that doing so is incorrect.)

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 20, 2019

Contributor

I didn't realize you felt this way about the specific impetus for the request and the proposal itself; I had thought, from you last comments in the issue thread, that you were in agreement that this was a reasonable request and a viable proposal.

I thought this comment expressed what I believe would be an appropriate motivation for this attribute.

This comment has been minimized.

Copy link
@BatmanAoD

BatmanAoD May 20, 2019

Author

@gnzlbg I agree that FFI Rust->Rust is an appropriate motivation for the attribute, but it felt disingenuous to me to advocate for it on that ground alone since it's not the real motivation for myself or @kornelski to be pushing for an annotation of some sort changing FFI unwind behavior. What I took from your phrasing in that comment about Rust->X->Rust was that you agreed it would be appropriate for @kornelski and anyone else to use the proposed annotation for Rust->X->Rust as long as they realize that they're relying on an implementation detail:

...by accident, it would allow those doing Rust->X->Rust to continue to "work" as long as their assumptions about implementation details still hold.

Knowledge about implementation details doesn't seem like an overly burdensome requirement for unsafe interactions between languages; that's how the computing world managed to function without a formally defined C ABI for several decades.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 20, 2019

Contributor

What I took from your phrasing in that comment about Rust->X->Rust was that you agreed it would be appropriate for @kornelski and anyone else to use the proposed annotation for Rust->X->Rust as long as they realize that they're relying on an implementation detail:

I don't see where I agreed to that. My comment says that "panics in Rust->Rust FFI" is a simple extension to the language that solves an useful problem, and that such a feature would allow some of the people that are invoking undefined-behavior in Rust today to continue doing so after the soundness fix for panics in Rust FFI lands (which AFAICT is all they wanted).

I never suggested #[unwind(Rust)] as a solution to "panic in Rust->X", nor say that it would be appropriate for anyone to write code with undefined behavior. If anything, I showed multiple times how to write code without UB in stable Rust today, and argued that because that is possible "panics in Rust->X" might not be a problem worth solving.

than zero will cause the program to be aborted. This is the only way the Rust
compiler can ensure that the function is safe, because there is no way for it
to know whether or not the calling code supports the same implementation of
stack-unwinding used for Rust.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

I don't think this is the main issue.

The Rust language guarantees that extern "C" functions do not panic, and these functions are optimized accordingly by LLVM. If they were to actually panic, the optimizations would be incorrect, which results in undefined behavior. Therefore, the language requires these functions to abort if a panic were to escape from them, but due to a bug in the implementation, this does not happen today. This bugs allows safe Rust programs to trigger UB, which makes the implementation unsound.

None of this has anything to do with the unwinding implementation of other languages. In fact, even if Rust code compiled with the exact same toolchain was used to call this function, the unsoundness bug would still be there, because LLVM will still optimize this code under the assumption that it does not panic.

}
assert!(result.is_err());
}
```

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

See above, this example is unsound.

}
```

**PLEASE NOTE:** Using this annotation **does not** provide any guarantees

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

The behavior of extern functions and extern function declarations that unwind (in any way - not only via Rust panics) is undefined. #[unwind(Rust)] allows these functions to let a Rust panic! escape, which is something that they cannot do without this attribute.

Explaining that does not really need mentioning anything about the panic implementation. If you want to mention that, just mentioning that the Rust panic implementation is unspecified, that is, it can change at any time, should be enough.

Since the behavior may be subject to change without notice as the Rust compiler
is updated, it is recommended that all projects that rely on unwinding from
Rust code into C code lock the project's `rustc` version and only update it
after ensuring that the behavior will remain correct.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

If this feature were aimed at solving this problem, then I don't think that having to do this to be able to use this feature correctly would be good enough.

I don't think this feature can solve this problem well without major changes.


Unwinding for functions marked `#[unwind(Rust)]` is performed as if the
function were not marked `extern`. This is identical to the behavior of `rustc`
for all versions prior to 1.35 except for 1.24.0.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

This reads a bit like this feature is an opt-out workaround for some language change. I don't think the RFC should frame that this way.


This annotation has no effect on functions not marked `extern`. It has no
observable effect unless the marked function `panic`s (e.g. it has no
observable effect when a function returns normally or enters an infinite loop).

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg May 13, 2019

Contributor

Note that the attribute does have an effect from the point-of-view of how the generated code can be optimized, even if the marked function never panic!s in practice.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented May 13, 2019

@gnzlbg Well...I do think that the specific annotation unwind(Rust) is better for Rust->Rust than Rust->X->Rust. But I also think that the more pressing and important issue really is Rust->X->Rust, and that the RFC ought to solve that problem. I believe there was resistance to the unwind(native) annotation in the other thread(s), but that's actually the one I would personally prefer. It would be a bit strange, though, because the meaning of "native" could depend on the toolchain, linker flags, platform, etc.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

But I also think that the more pressing and important issue really is Rust->X->Rust, and that the RFC ought to solve that problem.

Do you think that the feature proposed here is a good solution to the Rust->X->Rust problem?

I believe there was resistance to the unwind(native) annotation in the other thread(s), but that's actually the one I would personally prefer.

Something like that is IMO a better solution for Rust->X->Rust than this.

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented May 20, 2019

Do you think that the feature proposed here is a good solution to the Rust->X->Rust problem?

"Good" is ambiguous, but I do think it would be sufficient, at least for now. But #[unwind(native)] would be a better solution, I. believe.

Something like that is IMO a better solution for Rust->X->Rust than this.

It seems to me that #[unwind(native)] would also meet the needs of Rust->Rust; would you agree?

Looking again at the internals post and previous GitHub discussion, I don't actually see specific objections to #[unwind(native)]. Would you be more inclined to support a proposal for #[unwind(native)]? Or perhaps would you prefer to see both #[unwind(Rust)] and #[unwind(native)] as separate RFCs?

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

"Good" is ambiguous, but I do think it would be sufficient, at least for now.

Usually (always?) RFCs start by specifying the problem they intend to solve, and then they set out to find the "best" solution to that problem, where by "best" one should understand that the "best" solution often involves many trade-offs and constraints. So while this RFC might start at "good"/"sufficient", for it to be merged it will need to be the "best" solution to the problem it intends to solve, given the constraints.

It seems to me that #[unwind(native)] would also meet the needs of Rust->Rust; would you agree?

#[unwind(native)] would unwind with (probably) whatever the native C++ implementation does, which isn't necessarily a Rust panic. That is, Rust->Rust FFI code that wants to unwind using a "Rust panic", cannot do that with #[unwind(native)].

Would you be more inclined to support a proposal for #[unwind(native)]? Or perhaps would you prefer to see both #[unwind(Rust)] and #[unwind(native)] as separate RFCs?

I think both features solve slightly different problems, but these problems are related. I don't really mind much about whether features to solve these problems are proposed in one or multiple RFCs, but I think that each of these problems should be worth solving on its own, and that each feature that solves them should solve their problem well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.