Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
"C unwind" RFC #28
Changes from 31 commits
5a40727
8b33f10
6f3460f
26742cb
887bbbf
3acfbbf
8aaed63
ea8c5c8
e829c46
52de42d
8726cf5
c594939
59c1a12
c6230c6
788e0f1
837ecd0
0496f2e
ca322d7
81731df
bda9e54
1c2f729
5a4017a
b1df600
c2c04e5
c52638a
14c7094
0f387c2
e637631
b125bd4
59dd1ee
14036c6
ac8f840
533c679
fc13b34
c94f315
c70c886
25f1cfe
5d54d93
f2eb906
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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
.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.
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?
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 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 implementationabort
s.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.
I've added some verbiage about our usage of "TBD".
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 do you think we should say:
catch_unwind
?This does imply that panic=abort would have to intercept the C++ exception and 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.
Yes, I think that's accurate. I think we already had the
panic=abort
requirement?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.
I think it might be helpful to have a list of scenarios and a description of what works and what doesn't.
catch_unwind
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.
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.
I guess there is a similar table in the "detailed design" section.
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.
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.
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.
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.
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.
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... 😄
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, 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.
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.
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:
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.
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.
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 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