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

"C unwind" RFC #28

Merged
merged 39 commits into from
May 27, 2020
Merged

"C unwind" RFC #28

merged 39 commits into from
May 27, 2020

Conversation

BatmanAoD
Copy link
Member

@BatmanAoD BatmanAoD commented Apr 2, 2020

Based on "option 2" from the Internals post

rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Show resolved Hide resolved
@BatmanAoD BatmanAoD marked this pull request as ready for review April 3, 2020 03:29
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave a quick read. Nice job, thanks for whipping this up. Here are some initial thoughts.

rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
@bjorn3
Copy link
Member

bjorn3 commented Apr 26, 2020

Can the personality function detect if a function requires cleanup? In that case it would be nice to abort the process on forced-unwinding of drop frames on at least some systems.

@BatmanAoD
Copy link
Member Author

@bjorn3 Amanieu might be able to answer that, but even if it can't, the destructors themselves could do that. It's sort of expensive to insert that logic everywhere, though, so we'd only want to do that in debug mode.

rfcs/0000-c-unwind-abi.md Show resolved Hide resolved
caught in the caller frames. I.e., a C++ exception may be thrown,
cross into Rust via an `extern "C unwind"` function declaration, safely unwind
the Rust frames, and cross back into C++ (where it may be caught) via a Rust
`"C unwind"` function definition.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, at present, we were saying that it was still UB for a C++ exception to propagate across Rust frames, right?

Or, at minimum, we've not defined what the interaction with catch_unwind should be.

Though I think I would be ok with saying "catch-unwind will abort" and then discussing the possibility of adding some new mechanism later.

This would basically mean that one can "propagate C++ exceptions" in the manner described here, but only if you carefully control the Rust code in between and you know that it will not attempt to use catch_unwind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be curious to hear what @rust-lang/lang thinks about this, as well as @Amanieu -- how far do we want to go in terms of defining the interaction between "foreign" unwinding like C++ and Rust frames?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of simplicity for the initial specification, I would prefer to leave the interaction with catch_unwind TBD (formally UB) for now, even if the initial implementation aborts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some verbiage about our usage of "TBD".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do you think we should say:

  • propagating a C++ exception is OK (with "C unwind" and "panic=unwind")
  • but not if there is catch_unwind?

This does imply that panic=abort would have to intercept the C++ exception and abort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's accurate. I think we already had the panic=abort requirement?

may be "sandwiched" between Rust frames, so that Rust `panic`s may safely
unwind the C++ frames, if the Rust code declares both the C++ entrypoint and
the Rust entrypoint using `"C unwind"`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be helpful to have a list of scenarios and a description of what works and what doesn't.

Scenario Works
C++ exception propagating across Rust frames ✔️
C++ exception propagating across catch_unwind ❌ -- Currently UB, TBD
Rust panic propagating across a "C unwind" frame ✔️

the challenge here though is how to really do this table, the textual descriptions aren't that great. Also I think it'd be worth being specific about showing the ABI details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is a similar table in the "detailed design" section.

Copy link
Contributor

@nikomatsakis nikomatsakis Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I personally would like to see a list of scenarios we considered and which ones work and which ones don't. At least, as a user I would find that very useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To some extent I am doing the work I think we'd need to do to document this properly in the reference or other docs, but I think that is indeed the role of this section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I'm not sure I see a significant difference between what you're asking for and what's provided in the current draft. Though I do think that green/red checkmarks in the tables would be pretty... 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I don't think the draft actually lists out scenarios very thoroughly, and arranging them a table makes them easier to understand. It's more a matter of degree than anything.

rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved

[inside-rust-proposals]: https://blog.rust-lang.org/inside-rust/2020/02/27/ffi-unwind-design-meeting.html#three-specific-proposals

## Reasons for the current proposal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say that we should list out the design constraints that led us here. I guess that this section goes in that direction. This was my list:

  • Able to fully remove all unwinding machinery from panic=abort builds.
  • Able to change Rust panics to an alternative implementation beyond native system unwinding if desired, by inserting "translation shims" at "C unwind" call sites. This is possible under either implementation, but such shims would be much more widespread with a single ABI.
  • Repair the soundness hole when defining safe Rust functions with "C" ABI that panic (by aborting or translating the panic into a foreign exception). All designs did this equally well, either by aborting or by simply propagating the panic naturally.
  • Permit a Rust panic to propagate across foreign frames and re-enter Rust code.
  • Support longjmp/forced exceptions with "C" ABI, as many common C functions may unwind in this way. This proposal handles this case so long as there are no destructors, which was deemed adequate, as these features are highly platform dependent and not really suitable for widespread use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, several of these are listed in the blog post; perhaps we should link to that? In this section, I focused on differences compared to the other options we discussed in the design-meeting, rather than on our non-negotiable constraints.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could link to the blog post, although I think it's good for the RFC to be reasonably self-contained. Or we could just copy some of that material into a different section. Honestly, I think I would typically put these sorts of constraints into the introduction

Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for writing this up. This looks great to me, and would address all of our needs with Lucet.

rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
rfcs/0000-c-unwind-abi.md Outdated Show resolved Hide resolved
@BatmanAoD
Copy link
Member Author

Merging, though this is not quite ready to be submitted to the rfcs repo.

@BatmanAoD BatmanAoD merged commit 86b4697 into rust-lang:master May 27, 2020
@BatmanAoD BatmanAoD deleted the c-unwind-rfc branch May 27, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants