rust
rust copied to clipboard
rust-lang
Update `catch_unwind` doc comments for `c_unwind`
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: https://github.com/rust-lang/rust/issues/74990, https://github.com/rust-lang/rust/issues/115285, https://github.com/rust-lang/reference/pull/1226
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
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:
- 4a8fe8c1d65797793c874f39bfcd9576f1390184
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
#16 2.933 Building wheels for collected packages: reuse
#16 2.934 Building wheel for reuse (pyproject.toml): started
#16 3.181 Building wheel for reuse (pyproject.toml): finished with status 'done'
#16 3.182 Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132715 sha256=dfa09868353292d98f811d3efdb0d54d07389e808efc71d68e3b93c514bf8bec
#16 3.183 Stored in directory: /tmp/pip-ephem-wheel-cache-cfl4_bq9/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#16 3.185 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#16 3.594 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#16 3.594 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#16 DONE 3.7s
---
Documenting std v0.0.0 (/checkout/library/std)
error: unresolved link to `catch_unwind`
--> std/src/thread/mod.rs:1769:33
|
1769 | /// different runtime) as [`catch_unwind`]; namely, catching such an exception using this
| ^^^^^^^^^^^^ no item named `catch_unwind` in scope
= help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
= note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(rustdoc::broken_intra_doc_links)]`
error: could not document `std`
warning: build failed, waiting for other jobs to finish...
Command cd "/checkout" && env -u MAKEFLAGS -u MFLAGS AR_x86_64_unknown_linux_gnu="ar" CARGO_INCREMENTAL="0" CARGO_PROFILE_RELEASE_CODEGEN_UNITS="1" CARGO_PROFILE_RELEASE_DEBUG="0" CARGO_PROFILE_RELEASE_DEBUG_ASSERTIONS="true" CARGO_PROFILE_RELEASE_OVERFLOW_CHECKS="true" CARGO_PROFILE_RELEASE_STRIP="false" CARGO_TARGET_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std" CC_x86_64_unknown_linux_gnu="sccache cc" CFG_COMPILER_BUILD_TRIPLE="x86_64-unknown-linux-gnu" CFG_COMPILER_HOST_TRIPLE="x86_64-unknown-linux-gnu" CFG_RELEASE_CHANNEL="nightly" CFLAGS_x86_64_unknown_linux_gnu="-ffunction-sections -fdata-sections -fPIC -m64" CXXFLAGS_x86_64_unknown_linux_gnu="-ffunction-sections -fdata-sections -fPIC -m64" CXX_x86_64_unknown_linux_gnu="sccache c++" LIBC_CHECK_CFG="1" RANLIB_x86_64_unknown_linux_gnu="ar s" REAL_LIBRARY_PATH_VAR="LD_LIBRARY_PATH" RUSTBUILD_NATIVE_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/native" RUSTC="/checkout/obj/build/bootstrap/debug/rustc" RUSTC_BOOTSTRAP="1" RUSTC_BREAK_ON_ICE="1" RUSTC_ERROR_METADATA_DST="/checkout/obj/build/tmp/extended-error-metadata" RUSTC_FORCE_UNSTABLE="1" RUSTC_HOST_FLAGS="--cfg=bootstrap -Zunstable-options --check-cfg=cfg(bootstrap)" RUSTC_INSTALL_BINDIR="bin" RUSTC_LIBDIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" RUSTC_LINT_FLAGS="-Wrust_2018_idioms -Wunused_lifetimes -Dwarnings" RUSTC_REAL="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustc" RUSTC_SNAPSHOT="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustc" RUSTC_SNAPSHOT_LIBDIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" RUSTC_STAGE="0" RUSTC_SYSROOT="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-sysroot" RUSTC_VERBOSE="0" RUSTC_WRAPPER="/checkout/obj/build/bootstrap/debug/rustc" RUSTDOC="/checkout/obj/build/bootstrap/debug/rustdoc" RUSTDOCFLAGS="--cfg=bootstrap -Csymbol-mangling-version=legacy -Zunstable-options --check-cfg=cfg(feature,values(any())) -Zunstable-options --check-cfg=cfg(bootstrap) --document-private-items --document-hidden-items -Dwarnings -Wrustdoc::invalid_codeblock_attributes --crate-version 1.82.0-nightly\t(f90e078fc\t2024-08-04) -Zcrate-attr=doc(html_root_url=\"https://doc.rust-lang.org/nightly/\") -Zcrate-attr=warn(rust_2018_idioms) -Z unstable-options --resource-suffix 1.82.0 --markdown-css rust.css --markdown-no-toc --index-page /checkout/src/doc/index.md" RUSTDOC_LIBDIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" RUSTDOC_REAL="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustdoc" RUSTFLAGS="--cfg=bootstrap -Csymbol-mangling-version=legacy -Zunstable-options --check-cfg=cfg(feature,values(any())) -Zunstable-options --check-cfg=cfg(bootstrap) -Zmacro-backtrace -Csplit-debuginfo=off -Cprefer-dynamic -Zvalidate-mir -Zmir-opt-level=3 -Zinline-mir -Zinline-mir-preserve-debug -Clink-args=-Wl,-z,origin -Clink-args=-Wl,-rpath,$ORIGIN/../lib -Cforce-frame-pointers=yes -Zcrate-attr=doc(html_root_url=\"https://doc.rust-lang.org/nightly/\")" RUST_COMPILER_RT_ROOT="/checkout/src/llvm-project/compiler-rt" RUST_TEST_THREADS="4" TERM="xterm" WINAPI_NO_BUNDLED_LIBRARIES="1" __CARGO_DEFAULT_LIB_METADATA="bootstrapstd" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "doc" "--target" "x86_64-unknown-linux-gnu" "--release" "-Zbinary-dep-depinfo" "-j" "4" "--locked" "--color" "always" "--features" " panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/sysroot/Cargo.toml" "--no-deps" "--target-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/doc" "-Zskip-rustdoc-fingerprint" "-p" "alloc" "-p" "core" "-p" "panic_abort" "-p" "panic_unwind" "-p" "proc_macro" "-p" "std" "-p" "test" "-p" "unwind" (failure_mode=Exit) did not execute successfully.
Created at: src/core/builder.rs:1413:33
Executed at: src/core/build_steps/doc.rs:733:22
Build completed unsuccessfully in 0:00:22
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:
- 8e8a1d0e663609cd73bc7e91f48c804ff8c0b98b
@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?
I think @rust-lang/opsem may want to have a look at this as well, since it alters the AM behaviour of a language primitive (that would presumably have a corresponding Minirust primitive), particularily to have less undefined behaviour.
To me this sounds like it needs T-lang approval, because it stabilizes a new promise about a language primitive.
Thinking about this a bit more, I'd say we go for T-opsem + T-libs-api FCP, and nominate for T-lang in case they want to intervene.
The thing we're committing to here is that we'll always be able to handle "incoming" unwinding from other runtimes in some sane way. I know way too little about unwinding runtimes and implementations to say whether that's something that might bite us in the future or not. But of course it'd be great to have a guarantee like that, much better than UB!
Speaking as an expert in unwinding, beyond just "System Unwinds via an unsupported unwinding method", which I presume is just UB per se when it enters rust code, there shouldn't any issues with catching a foreign exception and aborting. Catching an exception and yielding it back to the program is a bit harder and has some subtle gotchas (though it is possible in theory), but that's why both behaviours are allowed, nondeterministically.
"System Unwinds via an unsupported unwinding method", which I presume is just UB per se when it enters rust code
So should we be worried about that? What would even be a real-world example of that?
If someone, for example, called into MinGW code using SJLJ exceptions from MinGW compiled to use SEH, it wouldn't be able to interact with the exception at all (unless you used a compiler that generates both handlers - I plan to have an option like this in lccc's gcc-style CLI, -femit-unwind-handling=sjlj alongside -funwind-method=seh in that particular case, though I don't know if there will be a rustc-style CLI analog for it).
Also, an unwinding proposal by someone named James Renwick that the C++ Embedded/Low Latency/Game Dev/LA WG14 reviewed a couple years ago, used a method of deterministic unwinding (via standard architecture-level control flow) which either would fizzle upon touching a stack frame that doesn't support it (since the frame would be required to test the unwinding flag, and when set either branch to the handler, or return to caller) or be UB to call into in the first place, depending on whether the unwind control block is in TLS or passed via invisible pointer parameter.
@chorman0773
If someone, for example, called into MinGW code using SJLJ exceptions from MinGW compiled to use SEH, it wouldn't be able to interact with the exception at all
Sorry, what's "it" here? The Rust standard library compiled for MinGW?
Is this something we should add to the reference?
@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.
We discussed this in the libs-api meeting and we're happy with this change. Effectively we are committing to being able to handle foreign exceptions in catch_unwind in a way that is not UB. But otherwise this doesn't make any additional guarantees.
Sorry, what's "it" here? The Rust standard library compiled for MinGW?
Yes - assuming its compiled to use SEH rather than SJLJ (should be the default without a switch to change it).
Is this something we should add to the reference?
Maybe, but I'm not sure how best to document this. It's like if you call a 64-bit routine from a 32-bit binary. You've crossed entirely different ambient ABIs, of course things are going to be screwy.
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Amanieu
- [ ] @CAD97
- [x] @JakobDegen
- [ ] @RalfJung
- [ ] @cuviper
- [ ] @digama0
- [ ] @joshtriplett
- [ ] @m-ou-se
- [ ] @saethlin
- [ ] @the8472
- [ ] @thomcc
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
@RalfJung Yes, this should go through a libs-api FCP. Feel free to start one and include T-opsem if you think it is relevant.
Team member @RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Amanieu
- [ ] @BurntSushi
- [x] @CAD97
- [x] @JakobDegen
- [x] @RalfJung
- [ ] @digama0
- [x] @dtolnay
- [ ] @joshtriplett
- [ ] @m-ou-se
- [ ] @saethlin
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
(I carried over @Amanieu 's and @CAD97 's checkbox from the earlier FCP with the wrong team)
@rfcbot fcp cancel @rustbot labels +T-lang
Process-wise, it seems probably better to just include us on the FCP here, so let's do that. I'll carry over the checkboxes.
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Amanieu
- [x] @BurntSushi
- [x] @CAD97
- [x] @JakobDegen
- [x] @RalfJung
- [ ] @digama0
- [x] @dtolnay
- [x] @joshtriplett
- [ ] @m-ou-se
- [x] @nikomatsakis
- [x] @pnkfelix
- [x] @saethlin
- [x] @scottmcm
- [x] @tmandry
- [x] @traviscross
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.