Miri: run liballoc tests with threads #71737
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| } | ||
| a.store(true, SeqCst); | ||
| }); | ||
|
|
||
| while !a2.load(SeqCst) { | ||
| let n = Arc::weak_count(&a2); | ||
| assert!(n < 2, "bad weak count: {}", n); | ||
| #[cfg(miri)] // The Miri scheduler won't reschedule if we do not yield. | ||
| thread::yield_now(); | ||
| } |
There was a problem hiding this comment.
@vakaras this demonstrates an issue with the current Miri scheduling policy: I had to adjust the code to avoid deadlocks. IMO we should introduce something where threads also get descheduled occasionally even when they do not yield.
There was a problem hiding this comment.
I had to adjust the code to avoid deadlocks
If I understand correctly, Miri did not terminate in this case, right?
IMO we should introduce something where threads also get descheduled occasionally even when they do not yield.
I have to agree with you.
There was a problem hiding this comment.
If I understand correctly, Miri did not terminate in this case, right?
Yes. It just ran and ran and probably was just sitting in this loop forever.
RalfJung
force-pushed
the
miri-test-threads
branch
from
8e3a3d1 to
68b454e
Compare
|
The changes seem... OK... to me, but the fact that it introduced what is effectively an infinite loop seems like a downside. I don't really have enough vision to decide what the risks are here. |
|
It did not introduce the infinite loop, the loop was already there. @shepmaster I can gate the |
update miri In particular this includes the change to yield on `spin_loop_hint`, which is needed for rust-lang#71737. r? @ghost Cc @rust-lang/miri Fixes rust-lang#71963
|
@shepmaster I am confused by your last message here (see above), could you clarify? |
This feels like a case of semantics. The PR enabled Miri, which caused Miri to run code forever. The fix ( My bigger question is more along the lines of "does this PR make that underlying infinite loop possible in other cases?". That is, by merging this, are we going to get "strange" failures in other code? |
No. Real schedulers make liveness guarantees, i.e. they guarantee that "threads will eventually get scheduled". It might still be inefficient to spin (though yielding is probably worse on a true multicore system, which is why I made the hint Miri-only). However, Miri's scheduler currently does not guarantee liveness (rust-lang/miri#1388). And anyway this is a weird spin loop -- it is not part of some synchronization but actually trying to catch bugs.
The PR does not add an infinite loop, so how should that be possible? If anything the PR removes the possibility of an infinite loop. I am very confused by this question.
I think I'll make the |
RalfJung
force-pushed
the
miri-test-threads
branch
from
68b454e to
7bea58e
Compare
Before this PR, the test suite did not spin infinitely in that test ("Miri did not terminate in this case, right?" "Yes. It just ran and ran and probably was just sitting in this loop forever."). Thus, the first version of the PR introduced/exposed/demonstrated/caused existing code to never terminate. Running some kind of "normal code" in Miri results in non-termination — is it possible that this non-termination will show up in other testcases? |
"introduced" and "demonstrated" are very different things though? The version of this code in master spins forever in Miri, due to rust-lang/miri#1388. That was just never a problem because this code never got run in Miri before. This PR doesn't introduce any new spinning. It give the Miri scheduler the hint it needs to avoid the endless spinning.
Yes it is. And then we'll deal with it when it comes up. Miri tests don't run as part of regular CI, they run externally, so if this ever happens (seems unlikely given that this is the only case in the entire test suite so far) it'll not concern anyone but me. |
Ah, this is a key part of understanding — I didn't want to unduly hamper a rustc contributor due to a current limitation of a Miri feature. @bors r+ rollup |
|
📌 Commit 7bea58e has been approved by |
bors
added
S-waiting-on-bors
…aster Miri: run liballoc tests with threads Miri now supports threads, so we can run these tests. :)
…aster Miri: run liballoc tests with threads Miri now supports threads, so we can run these tests. :)
Rollup of 5 pull requests Successful merges: - rust-lang#71737 (Miri: run liballoc tests with threads) - rust-lang#71928 (Add strikethrough support to rustdoc) - rust-lang#72048 (Visit move out of `_0` when visiting `return`) - rust-lang#72096 (Make MIR typeck use `LocalDefId` and fix docs) - rust-lang#72128 (strings do not have to be valid UTF-8 any more) Failed merges: r? @ghost
Miri now supports threads, so we can run these tests. :)