give some more help for the unusual data races #3145
Conversation
| } else if access.is_read() && other_access.is_read() { | ||
| assert!(involves_non_atomic); | ||
| Some( | ||
| "overlapping atomic and non-atomic accesses must be synchronized, even if both are read-only", |
There was a problem hiding this comment.
@saethlin I have been deliberating how to best express this... is "must be synchronized" clear enough? Would "must be ordered" better? Any other ideas?
There was a problem hiding this comment.
Just to be sure, is this an example of the UB you are talking about?
error: Undefined Behavior: Data race detected between (1) Atomic Load on thread `tokio-runtime-w` and (2) Read on thread `tokio-runtime-w` at alloc31996+0x20. (2) just happened here
--> /home/ben/tokio/tokio/src/loom/std/atomic_u32.rs:26:9
|
26 | core::ptr::read(self.inner.get() as *const u32)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Atomic Load on thread `tokio-runtime-w` and (2) Read on thread `tokio-runtime-w` at alloc31996+0x20. (2) just happened here
|
help: and (1) occurred earlier here
--> /home/ben/tokio/tokio/src/runtime/scheduler/multi_thread/queue.rs:455:28
|
455 | let src_tail = self.0.tail.load(Acquire);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
There was a problem hiding this comment.
I think I prefer "ordered"? At least the way I'd hand-wave the difference between those terms in English is to say that atomic operations provide orderings, then synchronization tools like locks can be built on those. Synchronization sounds like a stronger term to me 🤷
But my biggest question/concern with the diagnostic here is that this being UB will probably be surprising, so I think it would be best if we could preempt people asking questions about why. Is there a short version explanation? Or maybe we could link to rust-lang/unsafe-code-guidelines#345? The Twitter link in the issue is already broken, so the story is already a little confusing. My best reading of that issue is that this is UB because we mostly punt to C++ for our atomic semantics, and C++ seems to make this UB, though it's not clear why, and x86 really wants mixed-size atomics to be UB.
There was a problem hiding this comment.
Is there a short version explanation?
"There's no way to write code that does this in C++ without causing UB, and for now we don't want to invent our own memory model."
I am not sure if C++ has a very good reason for making this UB... x86 says it's disallowed but also Linux and Wine do it so it's not like they will be able to break this.
There was a problem hiding this comment.
AFAIK describing mixed-size accesses in these C++-style models is just very hard / hasn't been figured out yet
That's definitely how it seems to me. It would be unfortunate for us to say that atomic and non-atomic reads can produce a data race, especially if it's known to be permissible in formal models.
We've been not reporting this scenario as UB for a long time, so it would be kind of silly to start defensively reporting UB when we're sure that this will be made well-defined in the future.
There was a problem hiding this comment.
We've been not reporting this scenario as UB for a long time,
We've not supported threads for a long time, and when we did we've not reported any data races for a long time. The fact that we allowed racing atomic and non-atomic accesses was an oversight; I didn't realize that this is not possible in C++.
We have documented for a looooong time that we are using the C++ memory model. Until recently we didn't have any APIs that would let people cause racing atomic and non-atomic accesses (without brute-force operations like transmute). rust-lang/rust#115719 stabilized AtomicX::from_ptr which made this possible for the first time, and as part of stabilization we also clarified the docs to say explicitly that mixing atomic and non-atomic accesses is not allowed. Miri should detect such bugs.
There was a problem hiding this comment.
Are you suggesting we link to the docs? People tend to behave as if the docs are stable, they read them perhaps a few times when they are learning APIs then just use them. Unless we do something to prompt them, I do not think it is reasonable to expect an experienced user to notice that the docs have changed.
I'm not opposing reporting UB here, I just expect this one to be very odd to explain to people. And the more we can head that off when they see the UB report, the better.
There was a problem hiding this comment.
Sure, I can add links to the docs.
There was a problem hiding this comment.
But that is blocked on rust-lang/rust#116762, which puts the relevant docs in a more central place.
|
I'll add a link to the docs once rust-lang/rust#116762 lands. Meanwhile I think this is still an improvement, we can always further improve the message later. @bors r+ |
|
☀️ Test successful - checks-actions |
Fixes #3142