Skip to content

Windows thread support: Part 1#2231

Merged
bors merged 5 commits intorust-lang:masterfrom
beepster4096:winfred
Aug 18, 2022
Merged

Windows thread support: Part 1#2231
bors merged 5 commits intorust-lang:masterfrom
beepster4096:winfred

Conversation

@beepster4096
Copy link
Copy Markdown
Contributor

@beepster4096 beepster4096 commented Jun 13, 2022 •
edited
Loading

This PR adds support for threads on Windows.

@beepster4096 beepster4096 force-pushed the winfred branch 2 times, most recently from a8f777b to ecdcbd0 Compare June 13, 2022 05:20
@beepster4096
Copy link
Copy Markdown
Contributor Author

beepster4096 commented Jun 13, 2022

Whoops, I messed up the rebase at first.

@beepster4096 beepster4096 mentioned this pull request Jun 13, 2022
Merged
oli-obk
oli-obk reviewed Jun 13, 2022
View reviewed changes
oli-obk
oli-obk requested changes Jun 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to run ./miri bless, too, in order to update the .stderr files. You may have to run this for multiple targets to make sure they're all in sync in their behaviour

RalfJung
RalfJung reviewed Jun 13, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 14, 2022
View reviewed changes
@ChrisDenton
Copy link
Copy Markdown
Member

ChrisDenton commented Jun 14, 2022

If it helps, here are some public resources on HANDLEs:

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 14, 2022
@Dylan-DPC
Rollup merge of rust-lang#98042 - DrMeepster:winfred_std_changes, r=C…
c3c26ec 
…hrisDenton

Fix compat_fn option method on miri

This change is required to make `WaitOnAddress` work with rust-lang/miri#2231
@beepster4096
Copy link
Copy Markdown
Contributor Author

beepster4096 commented Jun 14, 2022

I've rewritten the handle implementation based on what is actually guaranteed by the windows docs

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 14, 2022
@Dylan-DPC
Rollup merge of rust-lang#98042 - DrMeepster:winfred_std_changes, r=C…
e565541 
…hrisDenton

Fix compat_fn option method on miri

This change is required to make `WaitOnAddress` work with rust-lang/miri#2231
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Jun 14, 2022

If it helps, here are some public resources on HANDLEs:

To the extent that these are relevant for this new code in Miri, please add links to them in comments in the code. :)

@bors
Copy link
Copy Markdown
Contributor

bors commented Jun 16, 2022

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

cbeuw
cbeuw reviewed Jun 18, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jun 26, 2022
View reviewed changes
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Jun 26, 2022

Thanks for the PR! Looks like Windows is starting to catch up with our Linux support. :)

I am working on a review, but in the future it'd be helpful to break PRs into smaller chunks. For example, I think this could have started with just thread spawning and joining, without all the concurrency primitives. Smaller PRs are a lot easier to review so any effort you can put into splitting a change into chunks is much appreciated.

RalfJung
RalfJung reviewed Jun 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great overall, very nice!

I hope all the new code you added is actually covered by tests. For thread spawning and the synchronization primitives, I would think the existing sync tests are enough, though I do not know if the stdlib actually uses these primitives.

I did not see a test that would join on a thread with a timeout, so that will have to be added (or the code for that be removed again). Also there are no new failing tests, which is a bit of a concern given that the pthreads primitives have a lot of failing tests to ensure that misuse of the pthread APIs is detected.

@bors
Copy link
Copy Markdown
Contributor

bors commented Jul 8, 2022

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

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Jul 9, 2022

Looking at your commits, I see "basic threading" and "condvar, parking and yielding" are separate commits. If you could split those into separate PRs, that would help a lot with reviewing.

Basically the first PR just needs to get concurrency/{simple,thread_local,tls*}.rs to work. That should be possible without new synchronization code I hope?

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Aug 14, 2022
@beepster4096 beepster4096 force-pushed the winfred branch 2 times, most recently from 8a6c1f8 to f010307 Compare August 14, 2022 04:32
@beepster4096
Copy link
Copy Markdown
Contributor Author

beepster4096 commented Aug 14, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Aug 14, 2022
RalfJung
RalfJung reviewed Aug 17, 2022
View reviewed changes
RalfJung
RalfJung reviewed Aug 17, 2022
View reviewed changes
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Aug 17, 2022

Just two more comment nits, then we are good to go. :) Since review is done, please squash the commits together a bit.

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Aug 17, 2022

@rustbot author

@bors
Copy link
Copy Markdown
Contributor

bors commented Aug 18, 2022

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

@beepster4096
Copy link
Copy Markdown
Contributor Author

beepster4096 commented Aug 18, 2022

@rustbot ready

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Aug 18, 2022

Awesome, thanks a ton for all your work and patience. :)

@bors r+

@bors
Copy link
Copy Markdown
Contributor

bors commented Aug 18, 2022

📌 Commit c466ac0 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Copy Markdown
Contributor

bors commented Aug 18, 2022

⌛ Testing commit c466ac0 with merge 46da748...

@bors
Copy link
Copy Markdown
Contributor

bors commented Aug 18, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 46da748 to master...

@bors bors merged commit 46da748 into rust-lang:master Aug 18, 2022
workingjubilee pushed a commit to pgcentralfoundation/postgrestd that referenced this pull request Sep 15, 2022
@Dylan-DPC
Rollup merge of #98042 - DrMeepster:winfred_std_changes, r=ChrisDenton
e97ab09 
Fix compat_fn option method on miri

This change is required to make `WaitOnAddress` work with rust-lang/miri#2231
@RalfJung RalfJung mentioned this pull request Oct 28, 2022
Closed
4 tasks
@beepster4096 beepster4096 mentioned this pull request Oct 30, 2022
Merged
bors added a commit that referenced this pull request Nov 6, 2022
@bors
Auto merge of #2638 - DrMeepster:windows-condvars, r=RalfJung
32c90ff 
Implement condvars for Windows

Adds 3 shims for Windows: `SleepConditionVariableSRW`, `WakeConditionVariable`, `WakeAllConditionVariable` to add support for condvars (which fixes #2628).

Salvaged from what was removed from #2231
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Nov 15, 2022
@bors
Auto merge of rust-lang#2638 - DrMeepster:windows-condvars, r=RalfJung
bf0f47b 
Implement condvars for Windows

Adds 3 shims for Windows: `SleepConditionVariableSRW`, `WakeConditionVariable`, `WakeAllConditionVariable` to add support for condvars (which fixes rust-lang#2628).

Salvaged from what was removed from rust-lang#2231
RalfJung
RalfJung reviewed Jan 18, 2026
View reviewed changes
tests/fail/concurrency/windows_join_main.rs

// XXX HACK: This is how miri represents the handle for thread 0.
// This value can be "legitimately" obtained by using `GetCurrentThread` with `DuplicateHandle`
// but miri does not implement `DuplicateHandle` yet.
Copy link
Copy Markdown
Member

@RalfJung RalfJung Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW by now we do support DuplicateHandle so we could fix this. However that function has so many flags, I can't figure out how it would be used.^^

(Also I don't understand why we even need it, couldn't we just use GetCurrentThread directly?)

Copy link
Copy Markdown
Contributor Author

@beepster4096 beepster4096 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCurrentThread returns a pseudo-handle, not a handle to the current thread. If the inner thread waited on that handle, it would actually wait on itself and not the main thread.

Copy link
Copy Markdown
Contributor Author

@beepster4096 beepster4096 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct call would be DuplicateHandle(GetCurrentProcess(), GetCurrentThread(), GetCurrentProcess(), <out ptr>, 0, false, DUPLICATE_SAME_ACCESS).

Copy link
Copy Markdown
Member

@RalfJung RalfJung Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So DuplicateHandle doesn't duplicate that pseudo-handle, it performs a kind of normalization?

What an API...

Copy link
Copy Markdown
Contributor Author

@beepster4096 beepster4096 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it creates a new handle from it that actually needs to be closed. Pseudo-handles are like constant sentinel values really.

I have just spotted that the current miri impl incorrectly passes pseudo-handles straight through though, instead of throwing an unsup error. I think I'll need to work on a PR for that and this test

Copy link
Copy Markdown
Contributor Author

@beepster4096 beepster4096 Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #4818

@beepster4096 beepster4096 mentioned this pull request Jan 18, 2026
Merged
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

@ChrisDenton ChrisDenton ChrisDenton left review comments

@oli-obk oli-obk oli-obk requested changes

+1 more reviewer

@cbeuw cbeuw cbeuw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S-waiting-on-review Status: Waiting for a review to complete

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@beepster4096 @ChrisDenton @RalfJung @bors @oli-obk @cbeuw @rustbot