Make the necessary changes to support concurrency in Miri. #70598

vakaras · 2020-03-31T04:37:27Z

This pull request makes the necessary changes to the Rust compiler to allow Miri to support concurrency:

Move stack from the interpretation context (InterpCx) to machine, so that the machine can switch the stacks when it changes the thread being executed.
Add the callbacks that allow the machine to generate fresh allocation ids for each thread local allocation and to translate them back to original allocations when needed. This allows the machine to ensure the property that allocation ids are unique, which allows using a simpler representation of the memory.

r? @oli-obk

cc @RalfJung

RalfJung · 2020-03-31T08:02:42Z

Oh wow, thanks a lot! I expected this to take way longer. ;)

I'd like to review this, too, before it lands. But it may take a week or two until I find the time.
Some notes for now:

Could you prepare a Miri PR that just makes Miri work again as before with the Machine changes? I'd like to not block "getting Miri to work again" on Implement basic support for concurrency (Linux/macos only) miri#1284.
Could you describe at a high level what the plan is for data races? You didn't mention them, so I guess Miri will not detect data races even when executing concurrent programs. That is a rather big problem, as it means Miri will happily execute such code but miss the UB. At the very least, this needs to be called out in the README; maybe there should even be a warning emitted when the first thread is spawned, or so.
I don't understand what you said here about TLS. I see no reason why any translation should be needed. The different threads just each make their own allocation for their thread-local storage, and have different allocation IDs for that, and all is fine. This is also what happens on real machines: different thread's TLS has different physical addresses. This is important as one thread can take a pointer to its TLS and send that to another thread.

src/librustc_mir/interpret/machine.rs

src/librustc_mir/interpret/memory.rs

vakaras · 2020-03-31T14:59:12Z

Oh wow, thanks a lot! I expected this to take way longer. ;)

I tried to make a minimal first step. There is still a lot of work to do, but my hope is that all of it will be on the Miri side.

* Could you prepare a Miri PR that just makes Miri work again as before
with the `Machine` changes? I'd like to not block "getting Miri to work
again" on
[rust-lang/miri#1284](https://github.com/rust-lang/miri/pull/1284).

Currently, each PR I create needs to get an approval from our legal department, which may take up to a week. If you think that it is still worth doing this, please let me know.

* Could you describe at a high level what the plan is for data races? You
didn't mention them, so I guess Miri will not detect data races even when
executing concurrent programs. That is a rather big problem, as it means
Miri will happily execute such code but miss the UB. At the very least,
this needs to be called out in the README; maybe there should even be a
warning emitted when the first thread is spawned, or so.

Data races are currently not supported. I think that adding a vector-clock based data race detection to all memory accesses is likely to significantly degrade the performance. I will add a comment to README and a warning as you suggest.

By the way, do stacked borrows rule out data-races except on UnsafeCell?

* I don't understand what you said here about TLS. I see no reason why
any translation should be needed.

I started learning about TLS only when I started working on this PR, so it is very likely that I do not understand something.

The different threads just each make their own allocation for their
thread-local storage, and have different allocation IDs for that, and all is
fine. This is also what happens on real machines: different thread's TLS has
different physical addresses. This is important as one thread can take a
pointer to its TLS and send that to another thread.

Could you please point me to the code that is supposed to do this thread-local allocation? The reason why I changed canonical_alloc_id to generate new allocation ids for thread locals is that get_raw_mut seems to get an allocation id that is a GlobalAlloc::Static and is the same for all threads. If we used this static id as a key in alloc_map (relevant code), all threads would get the same allocation, which is not what we want. One option would be to make the alloc_map aware of the thread local storage, but this is a big change (this was the first design I tried) and it does not have a property that allocation ids are unique, which, for example, breaks stacked borrows.

Is my reasoning clear and do you see what I missed?

bjorn3 · 2020-03-31T15:05:54Z

By the way, do stacked borrows rule out data-races except on UnsafeCell?

With raw pointers it is possible to create data-races even with stacked borrows.

RalfJung · 2020-03-31T15:06:38Z

Currently, each PR I create needs to get an approval from our legal department, which may take up to a week. If you think that it is still worth doing this, please let me know.

oO

The thing is, I want to avoid a situation where Miri does not work with rustc master. The Miri-side PR will likely take a while to review as it is much bigger, and new shims just tend to be a lot of effort (plus there is usually some first-time-contributor effect). If we cannot land this one here without breaking Miri or blocking on that big review, we have to wait until both PRs are completely reviewed before landing anything. This might mean more rebasing for you on the rustc side.

I have a preference for being able to land independent changes, but if you are fine with doing the rebases and not landing the rustc PR until both PRs are fully reviewed, I could live with that.

I hope you don't need to wait another week for each adjustment we request in this PR?

By the way, do stacked borrows rule out data-races except on UnsafeCell?

If you also exclude raw pointers (as pointed out by @bjorn3), I think so, but have not formally proven it.

Could you please point me to the code that is supposed to do this thread-local allocation? The reason why I changed canonical_alloc_id to generate new allocation ids for thread locals is that get_raw_mut seems to get an allocation id that is a GlobalAlloc::Static and is the same for all threads. If we used this static id as a key in alloc_map (relevant code), all threads would get the same allocation, which is not what we want. One option would be to make the alloc_map aware of the thread local storage, but this is a big change (this was the first design I tried) and it does not have a property that allocation ids are unique, which, for example, breaks stacked borrows.

You clearly know more about TLS than I do; I didn't even know "thread-local statics" were a thing. When I referred to thread-local storage I was thinking of the APIs we are already shimming in Miri via shims/tls.rs.
So, LLVM has some support for marking a static as "thread-local", and we now have to emulate that somehow in Miri? Well that will be fun.^^ In that case I agree something needs to happen on the machine level. Is there good documentation somewhere about how thread-local statics work?

bjorn3 · 2020-03-31T15:19:27Z

So, LLVM has some support for marking a static as "thread-local", and we now have to emulate that somehow in Miri?

Yes

#![feature(thread_local)]

#[thread_local]
static A: u8 = 0;

pub fn get_a_ref() -> *const u8 {
    &A
}

target triple = "x86_64-unknown-linux-gnu"

@_ZN10playground1A17hfc698d88543494bfE = internal thread_local constant <{ [1 x i8] }> zeroinitializer, align 1, !dbg !0

; playground::get_a_ref
; Function Attrs: nonlazybind uwtable
define i8* @_ZN10playground9get_a_ref17h42f05c46255e0587E() unnamed_addr #0 !dbg !11 {
start:
  ret i8* getelementptr inbounds (<{ [1 x i8] }>, <{ [1 x i8] }>* @_ZN10playground1A17hfc698d88543494bfE, i32 0, i32 0, i32 0), !dbg !15
}

In cg_clif the logic for tls can be found at https://github.com/bjorn3/rustc_codegen_cranelift/blob/c2e35604474aa25479dcca231fbc393a941234da/src/constant.rs#L64 to ensure that the const pointing to the static is directly codegened as taking the reference of the static rather than loading the from an allocation that contains the pointer. And at https://github.com/bjorn3/rustc_codegen_cranelift/blob/c2e35604474aa25479dcca231fbc393a941234da/src/constant.rs#L251 the static allocation is marked as thread local.

Is there good documentation somewhere about how thread-local statics work?

One of the things I used while implementing TLS for Cranelift was https://akkadia.org/drepper/tls.pdf. It describes the ABI level of TLS for ELF executables.

vakaras · 2020-03-31T20:27:59Z

I have a preference for being able to land independent changes, but if you are fine with doing the rebases and not landing the rustc PR until both PRs are fully reviewed, I could live with that.

I am fine with doing rebases.

src/librustc_mir/interpret/machine.rs

src/librustc_mir/interpret/memory.rs

src/librustc_mir/interpret/machine.rs

bors · 2020-04-01T23:46:57Z

☔ The latest upstream changes (presumably #70672) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2020-04-02T07:47:12Z

If we implement #70685 before this PR the interactions will become much clearer, but we can also punt that to the future and merge this PR. I'm fine either way

bors · 2020-04-03T07:39:29Z

☔ The latest upstream changes (presumably #70726) made this pull request unmergeable. Please resolve the merge conflicts.

src/librustc_mir/const_eval/error.rs

src/librustc_mir/const_eval/machine.rs

oli-obk · 2020-04-16T08:01:00Z

Also I'd prefer if you could await my review before landing this. :)

Sorry, I assumed since the last comments were on documentation and these were addressed, that this was ready to go.

src/librustc_mir/interpret/operand.rs

src/librustc_mir/interpret/machine.rs

…_const and add an additional comment.

src/librustc_mir/interpret/operand.rs

…nts.

src/librustc_mir/const_eval/machine.rs

RalfJung

This is great, thank you so much for working on this and doing some drive-by fixes (frame_idx).

r=me with the last nit fixed.

…nforce_ method group.

RalfJung · 2020-04-16T18:01:38Z

@bors r=oli-obk,RalfJung

bors · 2020-04-16T18:01:39Z

📌 Commit cbd9222 has been approved by oli-obk,RalfJung

bors · 2020-04-19T12:58:04Z

⌛ Testing commit cbd9222 with merge 61ec4ad1640bdbdcfc01ab4be1a9f20445d6c823...

Dylan-DPC-zz · 2020-04-19T13:24:00Z

@bors retry yield

bors · 2020-04-19T23:02:11Z

⌛ Testing commit cbd9222 with merge c6b55ee...

bors · 2020-04-20T02:17:50Z

☀️ Test successful - checks-azure
Approved by: oli-obk,RalfJung
Pushing c6b55ee to master...

Move the stack to the evaluator. (no-op PR for 70598) The changes to Miri to make it to compile with Rustc PR rust-lang/rust#70598.

jonhoo · 2020-04-20T12:38:22Z

Oh, this looks really exciting from the title of the PR. Does this mean that thread::spawn will now "just work", or are the more missing pieces?

RalfJung · 2020-04-20T13:12:59Z

Yes it will just work (once rust-lang/miri#1284 lands). :) UB checks for synchronization primitives are incomplete, and there is no data race detection, but the code runs at least.

Just check the testcases in rust-lang/miri#1284. :D

jonhoo · 2020-04-20T13:49:15Z

That's amazing! I can't wait to try this for evmap and for flurry. Or for that matter, for tokio 🎉

Implement basic support for concurrency (Linux/macos only) Changes (most new code is in `src/threads.rs` and `src/shims/foreign_items/posix.rs`): 1. Move the stack from `Machine` to a newly created `Thread` struct. 2. Add a `ThreadSet` struct that manages the threads. 3. Change `canonical_alloc_id` to create a unique allocation id for each thread local and thread (the responsible struct is `ThreadLocalStorage`) 4. Change the code to execute the thread local destructors immediately when a thread terminates. 5. Add the most basic round-robin scheduler. This pull request depends on [these changes to the compiler](rust-lang/rust#70598).

rust-highfive assigned oli-obk Mar 31, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2020

vakaras mentioned this pull request Mar 31, 2020

Implement basic support for concurrency (Linux/macos only) rust-lang/miri#1284

Merged

oli-obk reviewed Mar 31, 2020

View reviewed changes

src/librustc_mir/interpret/machine.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 31, 2020

View reviewed changes

src/librustc_mir/interpret/machine.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 31, 2020

View reviewed changes

src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

vakaras force-pushed the add-threads-cr3 branch 2 times, most recently from 89adcb6 to ce0a8ab Compare March 31, 2020 20:54

This comment has been minimized.

Sign in to view

vakaras force-pushed the add-threads-cr3 branch from ce0a8ab to 3d2d8ea Compare March 31, 2020 22:11

bjorn3 reviewed Mar 31, 2020

View reviewed changes

src/librustc_mir/interpret/machine.rs Outdated Show resolved Hide resolved

oli-obk reviewed Apr 1, 2020

View reviewed changes

src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved

oli-obk reviewed Apr 1, 2020

View reviewed changes

src/librustc_mir/interpret/machine.rs Outdated Show resolved Hide resolved

vakaras force-pushed the add-threads-cr3 branch from 3d2d8ea to 20e4c62 Compare April 2, 2020 00:50

oli-obk mentioned this pull request Apr 2, 2020

Make thread local accesses an Rvalue in MIR #70685

Closed

vakaras force-pushed the add-threads-cr3 branch from 20e4c62 to e1223aa Compare April 6, 2020 15:34

RalfJung reviewed Apr 13, 2020

View reviewed changes

src/librustc_mir/const_eval/error.rs Show resolved Hide resolved

RalfJung reviewed Apr 13, 2020

View reviewed changes

src/librustc_mir/const_eval/machine.rs Show resolved Hide resolved

oli-obk assigned RalfJung Apr 16, 2020

RalfJung reviewed Apr 16, 2020

View reviewed changes

src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 16, 2020

View reviewed changes

src/librustc_mir/interpret/machine.rs Outdated Show resolved Hide resolved

Rename Machine::eval_maybe_thread_local_static_const to adjust_global…

844eead

…_const and add an additional comment.

RalfJung reviewed Apr 16, 2020

View reviewed changes

src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved

Move the explanation why adjust_global_const is called for all consta…

d41f8bf

…nts.

RalfJung reviewed Apr 16, 2020

View reviewed changes

src/librustc_mir/const_eval/machine.rs Outdated Show resolved Hide resolved

RalfJung approved these changes Apr 16, 2020

View reviewed changes

Move stack access methods in the Machine implementations out of the e…

cbd9222

…nforce_ method group.

vakaras force-pushed the add-threads-cr3 branch from b132a21 to cbd9222 Compare April 16, 2020 17:01

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2020

bors merged commit c6b55ee into rust-lang:master Apr 20, 2020

bors mentioned this pull request Apr 20, 2020

Reading from the return place is fine #71005

Merged

bors added a commit to rust-lang/miri that referenced this pull request Apr 20, 2020

Auto merge of #1335 - vakaras:add-threads-noop, r=RalfJung

22cebbe

Move the stack to the evaluator. (no-op PR for 70598) The changes to Miri to make it to compile with Rustc PR rust-lang/rust#70598.

bors added a commit to rust-lang/miri that referenced this pull request Apr 20, 2020

Auto merge of #1335 - vakaras:add-threads-noop, r=RalfJung

d474bf8

Move the stack to the evaluator. (no-op PR for 70598) The changes to Miri to make it to compile with Rustc PR rust-lang/rust#70598.

bors added a commit to rust-lang/miri that referenced this pull request Apr 20, 2020

Auto merge of #1335 - vakaras:add-threads-noop, r=RalfJung

8d9db57

Move the stack to the evaluator. (no-op PR for 70598) The changes to Miri to make it to compile with Rustc PR rust-lang/rust#70598.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the necessary changes to support concurrency in Miri. #70598

Make the necessary changes to support concurrency in Miri. #70598

vakaras commented Mar 31, 2020

RalfJung commented Mar 31, 2020

vakaras commented Mar 31, 2020

bjorn3 commented Mar 31, 2020

RalfJung commented Mar 31, 2020 •

edited

Loading

bjorn3 commented Mar 31, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vakaras commented Mar 31, 2020

This comment has been minimized.

bors commented Apr 1, 2020

oli-obk commented Apr 2, 2020

bors commented Apr 3, 2020

oli-obk commented Apr 16, 2020

RalfJung left a comment

RalfJung commented Apr 16, 2020

bors commented Apr 16, 2020

bors commented Apr 19, 2020

Dylan-DPC-zz commented Apr 19, 2020

bors commented Apr 19, 2020

bors commented Apr 20, 2020

jonhoo commented Apr 20, 2020

RalfJung commented Apr 20, 2020 •

edited

Loading

jonhoo commented Apr 20, 2020

Make the necessary changes to support concurrency in Miri. #70598

Make the necessary changes to support concurrency in Miri. #70598

Conversation

vakaras commented Mar 31, 2020

RalfJung commented Mar 31, 2020

vakaras commented Mar 31, 2020

bjorn3 commented Mar 31, 2020

RalfJung commented Mar 31, 2020 • edited Loading

bjorn3 commented Mar 31, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vakaras commented Mar 31, 2020

This comment has been minimized.

bors commented Apr 1, 2020

oli-obk commented Apr 2, 2020

bors commented Apr 3, 2020

oli-obk commented Apr 16, 2020

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Apr 16, 2020

bors commented Apr 16, 2020

bors commented Apr 19, 2020

Dylan-DPC-zz commented Apr 19, 2020

bors commented Apr 19, 2020

bors commented Apr 20, 2020

jonhoo commented Apr 20, 2020

RalfJung commented Apr 20, 2020 • edited Loading

jonhoo commented Apr 20, 2020

RalfJung commented Mar 31, 2020 •

edited

Loading

RalfJung commented Apr 20, 2020 •

edited

Loading