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

Update catch_unwind doc comments for c_unwind #128321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BatmanAoD
Copy link
Member

@BatmanAoD BatmanAoD commented Jul 28, 2024

Updates catch_unwind doc comments to indicate that catching a foreign exception will no longer be UB. Instead, there are two possible behaviors, though it is not specified which one an implementation will choose.

Nominated for t-lang to confirm that they are okay with making such a promise based on t-opsem FCP, or whether they would like to be included in the FCP.

Related: #74990, #115285, rust-lang/reference#1226

@rustbot
Copy link
Collaborator

rustbot commented Jul 28, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 28, 2024
library/std/src/panic.rs Outdated Show resolved Hide resolved
library/std/src/panic.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics.rs Outdated Show resolved Hide resolved
library/std/src/panic.rs Outdated Show resolved Hide resolved
library/std/src/panic.rs Outdated Show resolved Hide resolved
library/std/src/panic.rs Outdated Show resolved Hide resolved
@tgross35
Copy link
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. has-merge-commits PR has merge commits, merge with caution. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Aug 4, 2024
@BatmanAoD
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2024
@BatmanAoD BatmanAoD force-pushed the catch-unwind-doc-update branch 2 times, most recently from 0718c3c to 39acc84 Compare August 11, 2024 21:36
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2024
@BatmanAoD
Copy link
Member Author

@rust-lang/libs I think we need some official sign-off of some sort on this, because it provides a new guarantee that wasn't part of the initial RFC.

If there's agreement that this should be merged, can we also backport it to beta in time for the 1.81 release, so that the documentation will be correct in time for the full stabilization of c_unwind?

@cuviper
Copy link
Member

cuviper commented Aug 14, 2024

New guarantees should be considered by @rust-lang/libs-api.

@Amanieu Amanieu added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 17, 2024
@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 28, 2024
@rfcbot
Copy link

rfcbot commented Aug 28, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@BatmanAoD
Copy link
Member Author

IMO we should "just" document what happens when a foreign unwind (or really, any unwind) leaves main, similar to what we now say for join. But I also don't know where to put this.

I don't see anything about an "entrypoint" in the standard library, but I may just not know what to look for. The other place that might make sense to document this would be, I guess, in the new panic documentation in the Reference, or possibly somewhere else in the Reference?

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

Yeah, the reference docs is the best place I can think of for this right now.

@BatmanAoD BatmanAoD force-pushed the catch-unwind-doc-update branch 2 times, most recently from a9bd7ba to 4ba514c Compare August 31, 2024 19:57
@BatmanAoD
Copy link
Member Author

@RalfJung The other PR mentioned unwinding past main in the "panic" section, but I've explicitly added this information to the fn main documentation as well.

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 4, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 7, 2024
@rfcbot
Copy link

rfcbot commented Sep 7, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@BatmanAoD
Copy link
Member Author

@tgross35 There are now only two commits; do you need me to squash again before merge?

@tgross35
Copy link
Contributor

@tgross35 There are now only two commits; do you need me to squash again before merge?

It's up to whoever approves this, and officially we have no history requirements. But it's always good to avoid making a change in one commit and then immediately undoing/modifying that change in another commit within the same PR, so I would probably squash (history/blame/bisect gets significantly harder to understand otherwise - nbd here, just good practice).

Also, you can drop the Co-authored-by: Trevor Gross ..., that was just a small wording suggestion :)

madsmtm added a commit to madsmtm/objc2 that referenced this pull request Sep 12, 2024
C-unwind was added in Rust 1.71, and allows panicking/unwinding
/exceptions across foreign function interfaces.

Additionally, Rust decided to let handling of foreign unwinds be
implementation defined behavior (instead of undefined), so we can now
mark `throw` as safe, see rust-lang/rust#128321.

This has a cost in that we now have landing pads on every message send;
this is strictly the correct choice, though, so we will have to bear
with it.

Fixes #539.
madsmtm added a commit to madsmtm/objc2 that referenced this pull request Sep 12, 2024
C-unwind was added in Rust 1.71, and allows panicking/unwinding
/exceptions across foreign function interfaces.

Additionally, Rust decided to let handling of foreign unwinds be
implementation defined behavior (instead of undefined), so we can now
mark `throw` as safe, see rust-lang/rust#128321.

This has a cost in that we now have landing pads on every message send;
this is strictly the correct choice, though, so we will have to bear
with it.

Fixes #539.
madsmtm added a commit to madsmtm/objc2 that referenced this pull request Sep 12, 2024
C-unwind was added in Rust 1.71, and allows panicking/unwinding
/exceptions across foreign function interfaces.

Additionally, Rust decided to let handling of foreign unwinds be
implementation defined behavior (instead of undefined), so we can now
mark `throw` as safe, see rust-lang/rust#128321.

This has a cost in that we now have landing pads on every message send;
this is strictly the correct choice, though, so we will have to bear
with it.

Fixes #539.
madsmtm added a commit to madsmtm/objc2 that referenced this pull request Sep 12, 2024
C-unwind was added in Rust 1.71, and allows panicking/unwinding
/exceptions across foreign function interfaces.

Additionally, Rust decided to let handling of foreign unwinds be
implementation defined behavior (instead of undefined), so we can now
mark `throw` as safe, see rust-lang/rust#128321.

This has a cost in that we now have landing pads on every message send;
this is strictly the correct choice, though, so we will have to bear
with it.

Fixes #539.
@Mark-Simulacrum
Copy link
Member

I think https://github.com/rust-lang/rust/pull/128321/files#r1750418130 is still unresolved -- it would make sense to me to clarify that behavior in this PR.

@BatmanAoD
Copy link
Member Author

@Mark-Simulacrum resolved.

@tgross35 That was added automatically by GH. 😆 Removed; thank you.

Documentation comments for `catch_unwind` and `thread::join` to indicate
new behavioral guarantee when catching a foreign exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.