Fix data race in thread::scope()

[danielhenrymantilla]

Fixes #98498, based on @RalfJung's suggestions (taking the non-Arc approach; for an Arc-based alternative, see #98503).

r? @m-ou-se

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[RalfJung]

See #98503 for the Arc alternative. :)

This here avoids a pointer indirection, but it also clones once per thread like my approach (when the thread finishes). And it needs unsafe code. 🤷 not sure what is preferable.^^

I'll test this PR in Miri... once it actually builds:

error[E0412]: cannot find type `UnsafeCell` in this scope
  --> /home/r/src/rust/rustc/library/std/src/thread/scoped.rs:44:18
   |
44 |     main_thread: UnsafeCell<Thread>,
   |                  ^^^^^^^^^^ not found in this scope
   |

(After fixing this it complains that UnsafeCell is non-Sync)

[danielhenrymantilla]

Yeah, I have to fix these things: I submitted the PR from the web UI 😅

@rustbot author

[danielhenrymantilla]

@rustbot review

[RalfJung]

Hm, I am still seeing Stacked Borrows violations with this PR:

error: Undefined Behavior: deallocating while item is protected: [SharedReadOnly for <45060> (call 13916)]
  --> atomic.rs:11:1
   |
11 | / std::thread::scope(|s| {
12 | |     for t in &some_bools[..5] {
13 | |         s.spawn(move || assert_eq!(t.load(Ordering::Relaxed), true));
14 | |     }
...  |
18 | |     }
19 | | });
   | |__^ deallocating while item is protected: [SharedReadOnly for <45060> (call 13916)]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
           
   = note: inside `main` at atomic.rs:11:1

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

[RalfJung]

Oh, I think what is happening is that the padding between the fields is not in an UnsafeCell and hence Stacked Borrows does not allow it to be deallocated while a reference points to it... :/

[danielhenrymantilla]

Oh no. Ok, at this point, the sketches I have to tackle padding too, make the PR become way more unwieldy¹, to my taste, than your now definitely simpler Arc one. I'm thus gonna close this unless we were to somehow become convinced of some actual benefits from this Arc-less approach that could make it worth the code complexity.

Footnotes

1. See https://github.com/rust-lang/rust/pull/98017#discussion_r906825431 for more info ↩