Skip to content

Inside Rust post: FFI-unwind design meeting #522

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

Conversation

BatmanAoD
Copy link
Member

The meeting date is still TBD. I believe it will either be this coming Monday (2 March) or two weeks after that (16 March). This must be finalized before posting.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

In this table, "UB" stands for "undefined behavior". Some instances of UB can
be detected at runtime, but the code to do so would impose an undesirable
code-size penalty; the group recommends generating such code only for debug
builds. These cases are marked "UB (debug: abort)"
Copy link
Member

Choose a reason for hiding this comment

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

We don't have any cases marked "UB (debug: abort)" in the table.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to me that if we didn't care too much about code size for debug builds, we could make all the UB entries be "debug: abort". What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

...well, no, I guess that's not true for cases where forced unwinding is allowed but destructors won't be invoked.

Copy link
Member Author

Choose a reason for hiding this comment

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

So would it be possible to use abort shims for all of the UB scenarios in the last column, "other foreign unwind"?

Copy link
Member

Choose a reason for hiding this comment

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

We can detect UB in the last column with abort shims around all FFI calls. We can detect UB from forced unwind running destructors by still emitting destructor landing pads but making them abort. In both cases we're pretty much doing codegen as if using panic=unwind, which might be too much of a cost even for debug builds.

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 second-to-last column, though, destructors won't reliably be run, even in debug mode, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I guess there's no reason why they wouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since, as you say, we'd be doing codegen as if it were panic=unwind

@BatmanAoD
Copy link
Member Author

The table is not particularly readable; colors or something might help.

I copied the HTML generated by GitHub because it appears that the blog doesn't use GitHub's extension for generating tables delimited by | characters.

@BatmanAoD
Copy link
Member Author

One other oddity: when I generate the blog site locally, it lists this entry chronologically earlier than the already-published incident report. I don't know how to change this without changing the date of the draft to tomorrow.

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Feb 26, 2020

@BatmanAoD Yes, that is a known issue with the blog, and there is no current workaround. I would just make the date tomorrow, it's tomorrow somewhere.

@BatmanAoD BatmanAoD marked this pull request as ready for review February 26, 2020 21:12
@XAMPPRocky
Copy link
Member

@BatmanAoD Do you want to wait for more review or should I merge this?

@BatmanAoD
Copy link
Member Author

@XAMPPRocky Since you've written and published posts before, if it looks okay to you, then I think we're good to go. Thank you!

@XAMPPRocky XAMPPRocky merged commit e409568 into rust-lang:master Feb 27, 2020
@BatmanAoD BatmanAoD deleted the inside-rust-post-ffi-unwind-design-meeting branch February 27, 2020 19:01
@nikomatsakis
Copy link
Contributor

Hmm I don't see this on https://blog.rust-lang.org/inside-rust/

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Feb 27, 2020

steveklabnik pushed a commit that referenced this pull request Mar 27, 2020
* Add first draft of post, with meeting date TBD

* Simplify description

* Use HTML generated by GitHub for table

* Discuss aborting on UB in debug mode

* Ensure new post is chronologically last

* Use March 2nd as prospective meeting date

* Fix non-inline links
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.

6 participants