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

Data race false positive on once_cell #1643

Closed
matklad opened this issue Dec 7, 2020 · 5 comments · Fixed by #1644
Closed

Data race false positive on once_cell #1643

matklad opened this issue Dec 7, 2020 · 5 comments · Fixed by #1644
Labels
A-concurrency Area: affects our concurrency (multi-thread) support C-bug Category: This is a bug.

Comments

@matklad
Copy link
Member

matklad commented Dec 7, 2020

Hi!

I am excited about the new data race detector, but, unfortunatelly, it fails on once_cell. I think this is a false positive -- I've managed to minimize the test case to do almost literally nothing, and commenting (or uncommenting) a no-op fn is what causes the bug. Here's the repro: matklad/once_cell@93717d9

The test is modified from std: https://github.com/rust-lang/rust/blob/45f638bd8620592b7c50e9b483c2aa8ef9715f5d/library/std/src/sync/once/tests.rs#L17

matklad added a commit to matklad/once_cell that referenced this issue Dec 7, 2020
@RalfJung
Copy link
Member

RalfJung commented Dec 7, 2020

@JCTyblaidd could you have a look at this?

@RalfJung RalfJung added C-bug Category: This is a bug. A-concurrency Area: affects our concurrency (multi-thread) support labels Dec 7, 2020
@RalfJung
Copy link
Member

RalfJung commented Dec 7, 2020

The error message is curious:

error: internal compiler error: /rustc/0f6f2d681b39c5f95459cd09cb936b6ceb27cd82/compiler/rustc_mir/src/interpret/validity.rs:918:17: Unexpected error during validation: Data race detected

So this corresponds not to a "normal" memory access but to an access that is implicitly performed by validation. This is probably related to the initialize_inner call.

I noticed the call has an &AtomicUsize as argument. It is possible that validation is doing something that is considered a non-atomic access to this data, which would then probably be a data race.

However, validation doesn't descent through references, so this is odd.

@JCTyblaidd
Copy link
Contributor

Really odd, managed to reduce to

use std::thread::spawn;
pub(crate) struct OnceCell;

impl OnceCell {
    #[cold]
    pub(crate) fn initialize(&self) {
        initialize_inner(&mut || false)
    }
}

fn initialize_inner(_init: &mut dyn FnMut() -> bool) {}

fn main() {
    static O: OnceCell = OnceCell;
    let j1 = spawn(move || {
        O.initialize();
    });
    let j2 = spawn(move || {
        O.initialize();
    });
    j1.join().unwrap();
    j2.join().unwrap();
}

There is no normal memory access associated so my guess is this is some internal miri memory that the race is detected for, looking into it.

@JCTyblaidd
Copy link
Contributor

Reduced to:

use std::thread::spawn;

fn initialize() {
    initialize_inner(&mut || false)
}

fn initialize_inner(_init: &mut dyn FnMut() -> bool) {}

fn main() {
    let j1 = spawn(initialize);
    let j2 = spawn(initialize);
    j1.join().unwrap();
    j2.join().unwrap();
}

It seems to be a data-race in the VTable, are v-tables generated lazily?

@JCTyblaidd
Copy link
Contributor

Looked in rustc_mir/src/interpret/traits.rs, i think its that the vtable is generated lazily in thread 1 and then read by thread 2, since they share the same vtable object and hence memory, so we need to somehow disable race detection when vtables are generated.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 12, 2020
Add post-init hook for static memory for miri.

Adds a post-initialization hook to treat memory initialized using the interpreter as if it was initialized in a static context.

See: rust-lang/miri#1644 & rust-lang/miri#1643
JohnTitor added a commit to JohnTitor/rust that referenced this issue Dec 13, 2020
Add post-init hook for static memory for miri.

Adds a post-initialization hook to treat memory initialized using the interpreter as if it was initialized in a static context.

See: rust-lang/miri#1644 & rust-lang/miri#1643
@bors bors closed this as completed in 85e5613 Dec 13, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 6, 2021
update Miri

update Miri to include fix for rust-lang/miri#1643
Cc `@rust-lang/miri` r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: affects our concurrency (multi-thread) support C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants