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

Add random failures to compare_exchange_weak #1686

Merged
merged 5 commits into from
Jan 28, 2021

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Jan 24, 2021

In practice this is pretty useful for detecting bugs.

This fails more frequently than realistic (50% (now 80%, controlled by a flag) of the time). I couldn't find any existing code that tries to model this (tsan, cdschecker, etc all seem to have TODOs there). Relacy models it with a 25% or 50% failure chance depending on some settings.

CC @JCTyblaidd who wrote the code this modifies initially, and seems interested in this subject.

src/data_race.rs Outdated Show resolved Hide resolved
tests/run-pass/atomic.rs Outdated Show resolved Hide resolved
tests/run-pass/atomic.rs Outdated Show resolved Hide resolved
@thomcc
Copy link
Member Author

thomcc commented Jan 26, 2021

Unsure if the CI error here is related to me... I'll rebase and hope it fixes itself. All good now.

@thomcc
Copy link
Member Author

thomcc commented Jan 26, 2021

I got a comment elsewhere about it being unfortunate that this triggers even if no other atomic operations are happening that involve the atomic variable. For future reference, and for anybody else with the same concern:

That is realistic, because on (some/most) real hardware these failures are triggered by operations by another thread on the same cache line (which might not even be part of the same allocation, let alone the same atomic). Also, all the descriptions of this in specifications I found put no bounds on when these are allowed to fail, just that they are allowed to fail randomly.

The one caveat here is that under -Zmiri-compare-exchange-weak-failure-rate=1.0 (e.g. explicitly requesting a 100% failure rate) it will fail every time, which I believe is against what's requested by most specs. That said, even this may be useful if testing a code path that tries to do something like backoff and use another algorithm under heavy contention, or whatever, so that seems fine given that it's an explicit request for the behavior.

@thomcc
Copy link
Member Author

thomcc commented Jan 28, 2021

Just to check, this isn't waiting on anything from me right?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 28, 2021

Nope, I just forgot about it. Thanks for the PR and the reminder!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 28, 2021

📌 Commit d310620 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Jan 28, 2021

⌛ Testing commit d310620 with merge a0485c5...

@bors
Copy link
Contributor

bors commented Jan 28, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a0485c5 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants