-
Notifications
You must be signed in to change notification settings - Fork 303
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
Inside Rust post: FFI-unwind design meeting #522
Conversation
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)" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 |
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. |
@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 Do you want to wait for more review or should I merge this? |
@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! |
Hmm I don't see this on https://blog.rust-lang.org/inside-rust/ |
* 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
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.