chore: removing unnecessary arc in scoped threads implementation

[tvallotton]

The Arc shouldn't needed because the scoped threads are allowed to borrow from the outer 'scope, which the lifetime of ScopeData should already satisfy.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
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

[ChrisDenton]

Why was an Arc added? What has changed since then?

[tvallotton]

Why was an Arc added? What has changed since then?

I have no idea. I was reading the code, and went to discord to ask why was an Arc being used here for no reason. Someone told me "if you think its unnecessary, remove it and make a PR", so I did. The tests pass so it must be fine.

[ChrisDenton]

Not necessarily. I would do a git blame on the code (there's also a "blame" button in the github UI) and look for the commit that added it.

[bugadani]

#98503 introduced the Arc to fix a data race.

[Noratrieb]

that justification seems fairly clear, so you'd need to show why it's incorrect

[tvallotton]

Idk, it wasn't clear to me when I read it. The way I saw it, the scoped data outlives the place where it is joined, so it seemed reasonable a reference should outlive it.

[Noratrieb]

improving the comment would be a nice improvement then :)

[workingjubilee]

for an example of how the lifetimes involved can be very complex and counterintuitive even in the scoped thread case, see:

• Scoped thread implicit join doesn't wait for thread locals to be dropped #116237

[tvallotton]

improving the comment would be a nice improvement then :)

Yeah, definitely.

[ChrisDenton]

cc @RalfJung it would be good to have a test for #98498 so the Arc doesn't get removed when it's still needed

[Noratrieb]

I'm not sure that such a test that fails outside Miri would be possible. I wonder if Miri catches this, how exactly do you test std under Miri locally?

[RalfJung]

how exactly do you test std under Miri locally?

./x.py miri std :)
But most of std fails because it uses APIs Miri does not support. So ./x.py miri std -- thread:: or so would work.

The reproducer in the original issue (#98498) is an std doctest. rustc CI runs these std doctests in Miri. So CI should catch when this breaks again -- but PR CI doesn't, and also this is a concurrency issue so it may be somewhat random whether it gets detected or not.

[RalfJung]

./x.py miri std -- thread:: on this PR does indeed catch the issue, first try.

...........error: Undefined Behavior: not granting access to tag <3430156> because that would remove [SharedReadOnly for <3471498>] which is strongly protected because it is an argument of call 757088
    --> /home/r/src/rust/rustc.2/library/core/src/ptr/mod.rs:537:1
     |
537  | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <3430156> because that would remove [SharedReadOnly for <3471498>] which is strongly protected because it is an argument of call 757088
     |
     = 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
help: <3430156> was created here, as the root tag for alloc1430378
    --> library/std/src/thread/tests.rs:393:9
     |
393  | /         thread::scope(|s| {
394  | |             for _ in 0..5 {
395  | |                 s.spawn(|| a_bool.load(Ordering::Relaxed));
396  | |             }
...    |
400  | |             }
401  | |         });
     | |__________^
help: <3471498> is this argument
    --> library/std/src/thread/scoped.rs:60:49
     |
60   |     pub(super) fn decrement_num_running_threads(&self, panic: bool) {
     |                                                 ^^^^^
     = note: BACKTRACE (of the first span) on thread `thread::tests::`:
     = note: inside `core::ptr::drop_in_place::<thread::scoped::Scope<'_, '_>> - shim(Some(thread::scoped::Scope<'_, '_>))` at /home/r/src/rust/rustc.2/library/core/src/ptr/mod.rs:537:1: 537:56
note: inside `thread::tests::scope_join_race`
    --> library/std/src/thread/tests.rs:393:9
     |
393  | /         thread::scope(|s| {
394  | |             for _ in 0..5 {
395  | |                 s.spawn(|| a_bool.load(Ordering::Relaxed));
396  | |             }
...    |
400  | |             }
401  | |         });
     | |__________^
note: inside closure
    --> library/std/src/thread/tests.rs:389:21
     |
387  | #[test]
     | ------- in this procedural macro expansion
388  | #[cfg(miri)] // relies on Miri's data race detector
389  | fn scope_join_race() {
     |                     ^
     |
    ::: /home/r/src/rust/rustc.2/library/core/src/macros/mod.rs:1636:5
     |
1636 |     pub macro test($item:item) {
     |     -------------- in this expansion of `#[test]`

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

error: aborting due to 1 previous error

error: test failed, to rerun pass `-p std --lib`

[RalfJung]

Oh in fact that's exactly a regression test for #98498 that triggered the UB. :)
So, the only extra thing we could do here is have a copy of this test in the Miri test suite, then it would be caught already during PR CI and not just with bors. (Or probably it'd make more sense to move the test than to copy it.)

[workingjubilee]

@tvallotton Would you be so kind as to make sure the test in question lands in miri's test suite in this PR?

[RalfJung]

The test runs 100 iterations... I think I made it std-only to avoid unduly slowing down the Miri test suite. The point of that test suite, after all, is to test Miri, not std. :)

[tvallotton]

@tvallotton Would you be so kind as to make sure the test in question lands in miri's test suite in this PR?

I could but if I don't think I can do it today.

[RalfJung]

@rust-lang/miri how do we feel about having this test in our test suite?

Personally I think we should not add this test to our test suite. It takes close to 10s to run on my laptop, making it one of the slowest tests we have. I don't think that's something I want to wait for every time I do ./miri test during Miri development. The Miri test suite should focus on finding bugs in Miri, not std. We carry some tests for things that came up in core/std, but those are cheap and cost basically no time. But for cases where the test is expensive (scope_join_race, arc_drop_dereferenceable_race) I have deliberately not added them to the Miri testsuite.

[workingjubilee]

Oh that's slower than I was expecting, I was hoping it was something it could be feasible to run only a few iterations of but 10s for 100 means that's still 100millis for that test, just for 1 iteration.

[oli-obk]

I don't think it's useful to have it in the miri test suite. As long as it exists elsewhere, so it will eventually fail under miri, and the documentation references the test, we have all the coverage we need

[workingjubilee]

Ah, making sure references exist then sounds like a good idea. We can make sure the comments here reference that test at least.

[saethlin]

I agree that adding this slow test to Miri's test suite is not a net benefit here.

[jhpratt]

r? libs

I don't really have the context here, and it seems as if others know what's necessary. Feel free to claim directly.

[Noratrieb]

I don't think there's much gained from adding this specific test to the Miri test suite to make it run in PR CI. If the problem is that std Miri tests aren't running in PR CI, they should be made to run in PR CI. I don't know if we can afford that, that would be a question for infra.
I'm not sure it's worth it, since I don't think it comes up that often, but it certainly wouldn't hurt the development experience either.

As for this PR, let's improve the comment to be clearer and link the test in it, to make sure someone in the future knows exactly what's going on.

[RalfJung]

Running Miri on core, alloc, std currently takes about 50-60min I think. So that's not something we can easily add to PR CI. std is run on multiple targets, but that will only save 10-20 minutes I think.

[Noratrieb]

@rustbot author

[tvallotton]

Should I just go on and close it?

[Noratrieb]

you're welcome to improve the comment such that you won't be confused next time, but you can also just close it if you don't want to