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.

Sign up for GitHub

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

Jump to bottom

Implement basic support for concurrency (Linux/macos only) #1284

Merged
merged 77 commits into from
Apr 30, 2020
Merged

Implement basic support for concurrency (Linux/macos only) #1284

merged 77 commits into from
Apr 30, 2020

Conversation

vakaras
Copy link
Contributor

@vakaras vakaras commented Mar 31, 2020 •
edited
Loading

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.

@vakaras vakaras mentioned this pull request Mar 31, 2020
Merged
@RalfJung RalfJung mentioned this pull request Mar 31, 2020
Merged
@RalfJung
Copy link
Member

Thanks a lot, the high-level picture sounds great. :D It may be a week or two until I get around to review this, though.

In particular, #1157 is ahead of this in the queue, so I expect you will have to rebase across it. And it sounds relevant; indeed this PR has to do something reasonable with the synchronization primitives on all platforms that can now spawn threads (and it should add suitable assertions to other platforms to make sure we don't accidentally spawn threads but then fail to correctly implement the locks).

@RalfJung
Copy link
Member

Another shim that might need adjustment is the one that returns the stack base address. I suppose it would be weird if all threads have the same base...

bjorn3
bjorn3 suggested changes Mar 31, 2020
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
src/threads.rs Outdated Show resolved Hide resolved
src/shims/tls.rs Outdated Show resolved Hide resolved
src/shims/tls.rs Outdated Show resolved Hide resolved
src/threads.rs Outdated Show resolved Hide resolved
@vakaras
Copy link
Contributor Author

vakaras commented Mar 31, 2020

I forgot to mention that this PR also depends on rust-lang/rust#70597. Without that PR, Miri complains about an aliasing violation.

bjorn3
bjorn3 reviewed Apr 2, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
tests/run-pass/concurrency/thread_locals.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Apr 2, 2020

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

@bors
Copy link
Collaborator

bors commented Apr 6, 2020

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

bjorn3
bjorn3 reviewed Apr 13, 2020
View reviewed changes
src/shims/threads.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/eval.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/shims/threads.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/threads.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 13, 2020
View reviewed changes
src/threads.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 29, 2020
View reviewed changes
src/shims/tls.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 29, 2020
View reviewed changes
src/shims/tls.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 29, 2020
View reviewed changes
src/shims/tls.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 29, 2020
View reviewed changes
src/shims/tls.rs Outdated Show resolved Hide resolved
@vakaras
Copy link
Contributor Author

vakaras commented Apr 29, 2020

(Please don't force-push, that makes it much harder to review only changes to this PR -- now I basically have to read the entire diff from scratch again.)

I am probably missing some feature of git: is there a way to rebase the PR without using git push --force-with-lease?

RalfJung
RalfJung reviewed Apr 29, 2020
View reviewed changes
src/thread.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 29, 2020
View reviewed changes
src/thread.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

I am probably missing some feature of git: is there a way to rebase the PR without using git push --force-with-lease?

I don't think so -- I guess I was saying "please don't rebase". Let's wait until reviews is done before resolving conflicts.

RalfJung
RalfJung reviewed Apr 30, 2020
View reviewed changes
src/thread.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Apr 30, 2020
View reviewed changes
src/shims/thread.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

These are the last things, that I just noticed when randomly looking at a few changed files again. Once these are fixed, I'll r+. :-)

@RalfJung
Copy link
Member

All right, no point in turning around every line of code 5 times. Let's land this. @vakaras thank you so much for contributing this and working with us through the review process. This is probably the biggest single addition to Miri ever, and certainly the biggest first-time contribution. :D

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 30, 2020

📌 Commit 48da0cf has been approved by RalfJung

@bors
Copy link
Collaborator

bors commented Apr 30, 2020

⌛ Testing commit 48da0cf with merge 351d46d...

@bors
Copy link
Collaborator

bors commented Apr 30, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 351d46d to master...

@bors bors merged commit 351d46d into rust-lang:master Apr 30, 2020
@bors bors mentioned this pull request Apr 30, 2020
Merged
@vakaras
Copy link
Contributor Author

vakaras commented Apr 30, 2020

Perfect. ☺ I will go and rebase #1362 so that it is ready for review. Should be ready for review by tomorrow.

@jonhoo
Copy link

jonhoo commented Apr 30, 2020

I'm so wildly excited for this! Can't wait to enable it on things like crossbeam, evmap, and flurry 🎉

bors added a commit that referenced this pull request May 25, 2020
@bors
Auto merge of #1362 - vakaras:add-sync-primitives-cr1, r=RalfJung
bfe030d 
Add sync primitives

This is a follow up PR for #1284 that adds support for the missing synchronization primitives.

Sorry for flooding with PRs, but my internship is coming to an end and I need to get things out.

Fixes #1419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@bjorn3 bjorn3 bjorn3 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vakaras @RalfJung @bors @jonhoo @bjorn3