More tests, fix issue 1643 and detect races with allocation.

[JCTyblaidd]

Fixes #1643 by disabling race detection for V-Table memory, adds race detection between r/w & memory allocation, and adds more tests.

There is one unusual result in dealloc_read_race_stack_drop.rs, where the stack variable is read by thread 0 & thread 2 and so reports a race with thread 0, any ideas for the cause of the read on thread 0? Fixed, bug with reporting the index a read race occured in correctly.

[JCTyblaidd]

Odd, compile-fail/data_race/read_write_race_stack.rs passes on my local machine

[bjorn3]

A few nits

[JCTyblaidd]

Looked through the log & ran ci.sh locally:

It seems to fail if -Z mir-opt-level=3 is passed as a compiler flag, so mir-opt might be making the behaviour defined and causing the test to fail.

All the tests pass for this:

## Running compile-fail tests in tests/compile-fail against miri for target x86_64-unknown-linux-gnu
   Compiler flags: --edition 2018 -Dwarnings -Dunused --sysroot /home/james/.cache/miri/HOST

But the test fails for this:

## Running compile-fail tests in tests/compile-fail against miri for target x86_64-unknown-linux-gnu
   Compiler flags: --edition 2018 -Dwarnings -Dunused --sysroot /home/james/.cache/miri/HOST -Z mir-opt-level=3

[RalfJung]

I wonder if there is a way to make the vtable init writes occur at timestamp 0? That would avoid having to disable the race detector for vtables.

[JCTyblaidd]

The v-tables are marked as immutable so we could just disable the data-race detector for any immutable memory instead of switching on memory type, assuming the memory is never marked mutable again. The alternative is to again expose some state of method that results in the internal switch on data-race detection being temporarily disabled similarly to atomic read/writes internally.

Though the issue doesn't seem to occur for the caller_location memory since it seems to allocate new memory each time it is invoked, which i think exposes different behaviour to rustc:

use std::panic::Location;

fn location() -> &'static Location<'static> {
    Location::caller()
}

fn main() {
    let a = location() as *const _;
    let b = location() as *const _;
    /// Passes on rustc & fails on miri
    assert_eq!(a,b);
}

I would assume both v-tables and caller location memory should be treated similarly as immutable statics initialized at timestamp 0.

[RalfJung]

Though the issue doesn't seem to occur for the caller_location memory since it seems to allocate new memory each time it is invoked, which i think exposes different behaviour to rustc

Note that even for vtables, Miri behavior differs from rustc -- rustc will duplicate vtables when they are used in multiple codegen units, while Miri entirely deduplicates them.

The alternative is to again expose some state of method that results in the internal switch on data-race detection being temporarily disabled similarly to atomic read/writes internally.

The write accesses that initialize the vtable are fine, right?
Could we have a method which takes an AllocId (or a Pointer+Size) and reset all clocks in that memory region to 0? That could then be called after Miri is done lazily initializing the vtable. (Caller locations don't need it I think since they are not shared.)

[JCTyblaidd]

The write accesses that initialize the vtable are fine, right?
Could we have a method which takes an AllocId (or a Pointer+Size) and reset all clocks in that memory region to 0? That could then be called after Miri is done lazily initializing the vtable

Yes this would work and should be easy to add, how do you think this method should be exposed to the v-table initialization code, extra method in Machine?

[RalfJung]

Yes this would work and should be easy to add, how do you think this method should be exposed to the v-table initialization code, extra method in Machine?

Hm, I have not thought about this... yeah a new machine method makes sense. It could be called something like after_static_mem_initialized or so.

[JCTyblaidd]

Reworked to use machine hook to reset data_race state - requires rust-lang/rust#79942 or similar.

[RalfJung]

Is this a bugfix, or what does this do?

[JCTyblaidd]

Bugfix, would return the incorrect vector index as the source of the data-race for VClocks with different sizes.

a race between [1,2] and [1] would return that the race was with index 0 instead of 1.

[RalfJung]

Okay, thanks. Is there a test ensuring that this will not regress?

[JCTyblaidd]

Yes, i added all thread ids to the tests instead of matching on just //~ERROR data race.

The test dealloc_read_race_stack_drop.rs has this case occur and covers the bug by requiring explicit matching on

//~ ERROR Data race detected between Deallocate on Thread(id = 1) and Read on Thread(id = 2)

whereas with the bug it would return:

//~ ERROR Data race detected between Deallocate on Thread(id = 1) and Read on Thread(id = 0, name = "main") 

[RalfJung]

Okay, awesome. :)

[RalfJung]

@JCTyblaidd thanks for the fix and even adjust rustc for this!
@bors r+

[bors]

📌 Commit c13aabc has been approved by RalfJung

[bors]

⌛ Testing commit c13aabc with merge 85e5613...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 85e5613 to master...