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

regression: the type (dyn Any + 'static) may contain interior mutability #125193

Closed
BoxyUwU opened this issue May 17, 2024 · 11 comments · Fixed by #125392
Closed

regression: the type (dyn Any + 'static) may contain interior mutability #125193

BoxyUwU opened this issue May 17, 2024 · 11 comments · Fixed by #125392
Labels
P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-libs Relevant to the library team, which will review and decide on the PR/issue.
Milestone

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented May 17, 2024

[INFO] [stdout] error[E0277]: the type `(dyn Any + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
[INFO] [stdout]   --> src/main.rs:13:1
[INFO] [stdout]    |
[INFO] [stdout] 13 | / lucet_hostcalls! {
[INFO] [stdout] 14 | |     #[no_mangle]
[INFO] [stdout] 15 | |     pub unsafe extern "C" fn two(&mut _vmctx,) -> *mut AssertUnwindSafe<Box<dyn Future<Output = u64>>> {
[INFO] [stdout] 16 | |         async fn body() -> u64 {
@BoxyUwU BoxyUwU added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 17, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 17, 2024
@BoxyUwU BoxyUwU added this to the 1.79.0 milestone May 17, 2024
@lqd
Copy link
Member

lqd commented May 17, 2024

I'll need to re-check later but this seems to bisect to #123203 somehow.

@inquisitivecrystal
Copy link
Contributor

I'll need to re-check later but this seems to bisect to #123203 somehow.

The UI test changes from that PR appear to back this up, as confusing as it is.

@lqd
Copy link
Member

lqd commented May 17, 2024

I'm not sure what's going on in that crate, if it built successfully by mistake, or how that PR interacts with it, so let's cc the PR author @jkarneges and reviewer @Amanieu for validation.

@lukas-code
Copy link
Member

After #123203, Context no longer implements UnwindSafe and RefUnwindSafe, because the context now contains a &mut dyn Any which doesn't implement those traits. So this is an issue with the standard library and not the crate being built.

Repro: playground

use std::panic::UnwindSafe;
use std::task::Context;

fn unwind_safe<T: UnwindSafe>() {}

fn main() {
    unwind_safe::<Context<'_>>(); // test UnwindSafe
    unwind_safe::<&Context<'_>>(); // test RefUnwindSafe
}

@rustbot label -T-compiler +T-libs +S-has-mcve

@rustbot rustbot added S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 17, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 18, 2024
@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 18, 2024
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

Noticing this comment on #123303 which says that was meant to be confined on nightly.

Unsure about the fallout of this regression but for now marking as critical for tracking purposes.

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 21, 2024
@Amanieu
Copy link
Member

Amanieu commented May 21, 2024

Revert up: #125377

@jkarneges
Copy link
Contributor

jkarneges commented May 21, 2024

Isn't Context already not unwind safe?

@workingjubilee
Copy link
Member

@jkarneges That line was added by your PR, so while what you say is technically true, talking about what "already happened" when we are traveling back and forth through time via version control may be more confusing than helpful.

@jkarneges
Copy link
Contributor

That line was already there. I added the line below it though. Original PR: https://github.com/rust-lang/rust/pull/123203/files#diff-c5319e149ad3e9d38c996d031a756a8437a533ea9f8bbb2cdfdb8f4c4a423b82

@workingjubilee
Copy link
Member

workingjubilee commented May 22, 2024

Apologies, I guess I can't count numbers today?

Hm, so, the full error of the regression is this:

error[E0277]: the type `(dyn Any + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  --> src/main.rs:13:1
   |
13 | / lucet_hostcalls! {
14 | |     #[no_mangle]
15 | |     pub unsafe extern "C" fn two(&mut _vmctx,) -> *mut AssertUnwindSafe<Box<dyn Future<Output = u64>>> {
16 | |         async fn body() -> u64 {
...  |
59 | |     }
60 | | }
   | | ^
   | | |
   | |_`(dyn Any + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   |   required by a bound introduced by this  @call
   |
   = help: within `Context<'_>`, the trait `RefUnwindSafe` is not implemented for `(dyn Any + 'static)`, which is required by `{closure@/home/jubilee/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lucet-runtime-internals-0.4.1/src/hostcall_macros.rs:54:56: 54:63}: UnwindSafe`
   = note: required because it appears within the type `&mut (dyn Any + 'static)`
note: required because it appears within the type `core::task::wake::ExtData<'_>`
  --> /rustc/a26981974230110fa8fb15e1cf04d05b9a2103f9/library/core/src/task/wake.rs:225:6
note: required because it appears within the type `Context<'_>`
  --> /rustc/a26981974230110fa8fb15e1cf04d05b9a2103f9/library/core/src/task/wake.rs:236:12
   = note: required for `*mut Context<'_>` to implement `UnwindSafe`

Note that it's trying to make a pointer implement UnwindSafe. And for what it's worth, stable does indeed document Context<'_> is UnwindSafe. What implements !UnwindSafe though, also on stable, is &mut T.

Thus unfortunately that line you pointed to, as opposed to the one I thought you were referring to, is effectively just documenting that all &mut T are !UnwindSafe, and that will include Context<'_> regardless of whether it is is UnwindSafe.

And confusingly, *mut T is UnwindSafe if T is RefUnwindSafe, unlike &mut T.

@workingjubilee
Copy link
Member

I have opened #125392 as an alternative.

@bors bors closed this as completed in 5126d4b May 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 23, 2024
Rollup merge of rust-lang#125392 - workingjubilee:unwind-a-problem-in-context, r=Amanieu

Wrap Context.ext in AssertUnwindSafe

Fixes rust-lang#125193

Alternative to rust-lang#125377

Relevant to rust-lang#123392

I believe this approach is justifiable due to the fact that this function is unstable API and we have been considering trying to dispose of the notion of "unwind safety". Making a more long-term decision should be considered carefully as part of stabilizing `fn ext`, if ever.

r? `@Amanieu`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue May 24, 2024
… r=Amanieu

Wrap Context.ext in AssertUnwindSafe

Fixes rust-lang/rust#125193

Alternative to rust-lang/rust#125377

Relevant to rust-lang/rust#123392

I believe this approach is justifiable due to the fact that this function is unstable API and we have been considering trying to dispose of the notion of "unwind safety". Making a more long-term decision should be considered carefully as part of stabilizing `fn ext`, if ever.

r? `@Amanieu`
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.