Stream: project-ffi-unwind

Topic: RFC follow-up


view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 03:18):

How do we want to address petrochenkov's point that there must be some safe way to unwind through system and thiscall ABIs in order to remove #[unwind(allowed)] from the standard library?

Additionally, do we want to re-open discussion of whether unwinding should be the default rather than an opt-in behavior?

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 03:19):

I.e. the unwinding ABI would be extern "C", and we'd introduce a new C-without-unwinding ABI

view this post on Zulip Vadim Petrochenkov (Jun 19 2020 at 08:57):

I'd say that the "ABI" part and "unwind" part should ideally be orthogonal, but right now introducing extern "(system,thiscall) unwind" for the standard library use and not stabilizing them should be ok.

view this post on Zulip Vadim Petrochenkov (Jun 19 2020 at 08:58):

If the unwind part becomes orthogonal in the future, like extern "C" unwind fn foo(), then we can always keep "C unwind" for compatibility and desugar it into "C" unwind.

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:56):

that's a really good point

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:56):

I'm surprised we didn't talk about it already!

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:57):

It does feel to me like the "right thing" is for the unwind to be kind of orthogonal, but perhaps that just means having "unwind" variants of system/thiscall/C

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:57):

at least for now

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 13:57):

We did...that's how RFC 2699 worked, at least at one point

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:57):

ok well I wasn't involved :)

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:57):

I just mean that in these recent conversations we haven't talked about non-C APIs that unwind

view this post on Zulip nikomatsakis (Jun 19 2020 at 13:58):

anyway I think i'm fine with just saying that we have extern "system unwind"

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 15:33):

I'm hesitant. Shouldn't system mean "automatically pick the right ABI for this system"? That's why we have it in addition to stdcall, even though stdcall and C could in theory replace system (using annotations to select the right one for different targets).

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 15:33):

If that's the case, then perhaps system should _always_ be "unwind-safe".

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 15:34):

.....on platforms that are known to support unwinding.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 15:35):

We did briefly discuss whether we needed unwind for other ABIs during the lang team meeting on 2 March: langteam-sync/design-meeting-2020-03-02.md

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 19 2020 at 15:36):

The need for system to unwind makes me want to reconsider the "C"/"C nounwind" option.

view this post on Zulip nikomatsakis (Jun 22 2020 at 18:33):

honestly I forget where/when we use system

view this post on Zulip nikomatsakis (Jun 22 2020 at 18:34):

it seems likely to be fairly niche

view this post on Zulip nikomatsakis (Jun 22 2020 at 18:34):

but maybe not :)

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 18:35):

petrochenkov's example is panic's implementation on Windows: https://github.com/rust-lang/rust/blob/master/src/libpanic_unwind/seh.rs#L310

    extern "system" {
        #[unwind(allowed)]
        pub fn _CxxThrowException(pExceptionObject: *mut c_void, pThrowInfo: *mut u8) -> !;
    }

view this post on Zulip nikomatsakis (Jun 22 2020 at 18:36):

right, are there other places we use it? is it primarily just the stdlib?

view this post on Zulip nikomatsakis (Jun 22 2020 at 18:36):

or is it something used "out there" in the ecosystem?

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 18:38):

Hm... I don't know. But to me, the following make me lean toward "system should always unwind":

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 18:42):

It looks like it's used in quite a few places, but I don't know if all 11k of these are "real" or not. But from looking through a half dozen pages of results, I don't see any red flags that my search isn't valid. https://github.com/search?q=%22extern+system%22+language%3ARust&type=Code

view this post on Zulip Vadim Petrochenkov (Jun 22 2020 at 18:49):

Resetting the discussion to the point of "whether C-like ABIs should be considered unwind by default" certainly wasn't my goal.

view this post on Zulip Vadim Petrochenkov (Jun 22 2020 at 18:50):

I wish I never brought this point, and I'd want recommend against overthinking and rationalizing some new semantics to extern "system".

view this post on Zulip Vadim Petrochenkov (Jun 22 2020 at 18:52):

Whatever idea was originally behind "system", it's not currently used like that.
De-facto it's a redundant alias to "stdcall" now and it would be better if it didn't exist, IMO.

view this post on Zulip Vadim Petrochenkov (Jun 22 2020 at 18:53):

"stdcall unwind" and "thiscall unwind" can be internal implementation details and I think the RFC can proceed in the same form as it's written now.

view this post on Zulip Vadim Petrochenkov (Jun 22 2020 at 19:08):

We did...that's how RFC 2699 worked, at least at one point

Although, if the wg is ready to reconsider this (extern "C" unwind fn foo, making ABI orthogonal to the ABI flag), then it would probably be better long term.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:12):

If I recall correctly, the main reason we didn't want to do that is because we want to leave open the possibility of a separate "nopanic" feature (unrelated to extern) somewhere down the line.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:12):

And we didn't want to "accidentally" introduce an equivalent feature.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:14):

I understand not wanting "system" to unwind by default if "C" doesn't, but the fact that we'll need unwinding for "stdcall" and "thiscall" makes me think that we should reconsider the default for "C" as well.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:17):

Unfortunately, the person who was most resistant to unwind-by-default was Centril, who is not currently active in the project or easily contactable. So if we re-opened the discussion, I suspect we could come to a different conclusion than we did before, but only because we would (presumably) no longer have an active advocate for the opposing perspective.

view this post on Zulip Josh Triplett (Jun 22 2020 at 19:19):

I personally have a mild preference for no-unwind-by-default, though not a strong preference.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:19):

That's helpful, I think.

view this post on Zulip Josh Triplett (Jun 22 2020 at 19:19):

Specifically because most code doesn't need unwind, and it has a slight performance hit, and it's easy enough to add unwind to the extern.

view this post on Zulip Josh Triplett (Jun 22 2020 at 19:20):

That holds doubly true if we can, without a substantial performance hit, catch unwind attempts and panic, at least in debug mode.

view this post on Zulip Josh Triplett (Jun 22 2020 at 19:20):

(I doubt it'll be zero-overhead, but we could do it in debug.)

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:21):

I believe that was Centril's primary objection as well, especially w/ panic=abort

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:22):

Note that w/ "C unwind" in the current proposal, there is no UB. I like the idea that Rust is generally "safe by default". Having possible UB for the sake of performance seems very much like the sort of thing Rust typically only permits via opt-in, not opt-out.

view this post on Zulip nikomatsakis (Jun 22 2020 at 19:55):

So, we discussed this some in the meeting. I feel like the obvious fix to this problem is that we introduce additional ABIs ("stdcall unwind", etc), and I think that's...fine. I mean if we're adding "C" and "C unwind" I think honestly I would expect that you would have "unwind" variants of the various other ABIs available (and if there isn't a variant, I would expect that means something like unwinding doesn't work there for some other reason). This seems OK and I'm not inclined to try to change the consensus at this point.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 19:56):

Okay. @acfoltzer , @Amanieu , thoughts?

view this post on Zulip acfoltzer (Jun 22 2020 at 20:22):

I haven't been following this closely (gonna read backscroll now done) but this shouldn't hurt our use case, so I'm fine with it (edit: yep, this sounds fine)

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 22 2020 at 20:46):

Okay, so my TODOs for amending the RFC are:

view this post on Zulip Peter Rabbit (Jun 22 2020 at 23:57):

nikomatsakis said:

it seems likely to be fairly niche

winapi uses system for almost all its extern functions, but I'm used to winapi being treated like a niche thing. :exhausted:

view this post on Zulip nikomatsakis (Jun 23 2020 at 09:37):

@Peter Rabbit I stand corrected!

view this post on Zulip nikomatsakis (Jun 23 2020 at 09:39):

Though I can't tell if that implies that you have an opinion about using "system unwind" to declare intentional unwinding

view this post on Zulip Peter Rabbit (Jun 23 2020 at 12:05):

At least on Windows the cost of unwind tables is unavoidable, so the only real performance optimization is whether we emit drop tables in cases where we don't call anything that can unwind other than C++ so that if a C++ exception unwinds through us we can run our drops. Therefore I lean towards supporting unwinding by default.

For some prior art, cl.exe (VC++ compiler) has a flag to determine the exception handling model. https://docs.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model
/EHsc will assume that extern "C" cannot throw exceptions while /EHs will assume that they can throw. There is no opt in for specific functions. However you can opt out by declaring specific functions as noexcept.

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:06):

@Peter Rabbit So you are arguing for making extern "C" unwind-by-default on Windows specifically.

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:09):

Because it's less of a pessimization than it is on some other plaforms not requiring unwind tables.

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:11):

"Support unwind-by-default on Windows" should apply to extern "system" less than to extern "C" in general, because winapi doesn't usually throw anything we want to run drops on, AFAIR.

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:12):

And winapi is pretty much the only thing that uses stdcall, even if it's large, so it is niche :slight_smile:

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:14):

whether we emit drop tables in cases where we don't call anything that can unwind other than C++

Which is a significant optimization.
Imagine any math stuff from C libraries that doesn't throw and is used in computationally heavy code.

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:16):

/EHsc ftw

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 14:25):

That's the default for projects created with Visual Studio at least.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 23 2020 at 16:30):

Vadim Petrochenkov said:

"Support unwind-by-default on Windows" should apply to extern "system" less than to extern "C" in general, because winapi doesn't usually throw anything we want to run drops on, AFAIR.

I don't understand what you mean by this; do you just mean that winapi doesn't usually throw at all, or that something about the winapi interface makes it less likely for Drop objects to be used in the calling code, or something else?

view this post on Zulip Vadim Petrochenkov (Jun 23 2020 at 17:23):

The former, winapi doesn't throw (C++-style exceptions).

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 23 2020 at 17:33):

Ah, okay.

view this post on Zulip nikomatsakis (Jun 24 2020 at 14:55):

When you say winapi doesn't through, are you saying that all failures are communicated through return codes, and not using SEH ?

view this post on Zulip nikomatsakis (Jun 24 2020 at 14:55):

I feel like having the behavior of whether unwinding is UB or not depend on the platform (windows one way, other platforms another) is surprising and bound to cause portability hazards. I would not want to go that path.

view this post on Zulip nikomatsakis (Jun 24 2020 at 14:56):

But I am interested to know whether winapi would expect to modify all of its ABIs from extern "system" to extern "system unwind" -- this was something we talked about with respect to libc and functions like read (which in some niche cases may use unwinding, but only with forced unwinds).

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 24 2020 at 14:57):

That's explicitly called out in the RFC as the reason we consider forced unwinding over non-POFs to be UB.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 24 2020 at 14:58):

("that" meaning your point about the portability hazard of having UB on some platforms but not others for the same code)

view this post on Zulip Peter Rabbit (Jun 25 2020 at 02:36):

nikomatsakis said:

When you say winapi doesn't through, are you saying that all failures are communicated through return codes, and not using SEH ?

All Windows API failures are communicated through return codes, so winapi would not opt into unwinding. When an SEH exception occurs 99% of the time you just want to crash the process because it indicates a fatal error. 1% of the time it is because you are doing things like reading/writing memory mapped files and the disk backing the file was unmounted and suddenly your read/write causes an SEH exception, in which case you use __try and __catch to catch that exception and gracefully return an error. I'd really love to have the equivalent of __try and __catch in Rust.

The fun thing about SEH exceptions is that they can occur at any instruction, not just function calls or explicit throws. In C++ by default SEH exceptions do not trigger destructors and are not caught by catch(...) (although you can specify /EHa if you really want that).

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 25 2020 at 02:39):

I take it that's what the LLVM docs mean when they refer to SEH as a form of "async exception"?

view this post on Zulip Amanieu (Jun 25 2020 at 03:14):

FYI as currently implemented we only run destructors for C++ exceptions (which panics are based on) and (sometimes) longjmp exception.

view this post on Zulip Amanieu (Jun 25 2020 at 03:14):

I would therefore consider any SEH exception as outside the scope of the RFC.

view this post on Zulip nikomatsakis (Jun 29 2020 at 21:14):

So -- from what I understand -- we are basically set to move forward, with the plan being to add unwind variants of ABIs as we find we need them, correct?

view this post on Zulip nikomatsakis (Jun 29 2020 at 21:15):

@BatmanAoD (Kyle Strand) did you have any further edits to make or shall I fcp merge?

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 29 2020 at 21:15):

I want to add the ABIs we already know we need before fcp.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 29 2020 at 21:16):

I have a few "TODO"s in comments in the document.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 29 2020 at 21:16):

But adding the other strings is the main one.

view this post on Zulip nikomatsakis (Jun 30 2020 at 14:26):

OK. I think it's important to state that we expect to be adding more but not critical that it should be "complete". I think we should probably just list some and state that the set may vary.

view this post on Zulip BatmanAoD (Kyle Strand) (Jun 30 2020 at 23:31):

Do you think it would be acceptable to add new unwind ABIs after the RFC has landed without opening a new RFC? That seems like it would be a pretty minor change

view this post on Zulip nikomatsakis (Jul 01 2020 at 14:14):

I fully expect to do so

view this post on Zulip nikomatsakis (Jul 01 2020 at 14:15):

I think we can say that explicitly, but the point of the RFC (in my view) is to establish the precedent, not to enumerate each use of it

view this post on Zulip BatmanAoD (Kyle Strand) (Jul 04 2020 at 04:04):

I've pushed a few changes

view this post on Zulip BatmanAoD (Kyle Strand) (Jul 06 2020 at 15:20):

@WG-ffi-unwind I completed my "to do" items over the weekend. From my perspective, the RFC is ready for FCP.

view this post on Zulip BatmanAoD (Kyle Strand) (Jul 07 2020 at 22:21):

@WG-ffi-unwind I've made a few more changes, hopefully resolving @RalfJ 's concerns.


Last updated: Jan 26 2022 at 07:32 UTC