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.

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

Add support for FUTEX_{WAIT,WAKE}_BITSET #2054

Merged
merged 7 commits into from
Apr 6, 2022

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Apr 6, 2022

FUTEX_WAIT_BITSET and FUTEX_WAKE_BITSET are extensions of FUTEX_WAIT and FUTEX_WAKE that allow tagging each waiting thread with up to 32 'labels', and then only wake up threads that match certain labels. The non-bitset operations behave like their bitset was fully set (u32::MAX), meaning that they'll wait for anything, and wake up anything.

The only other difference is that FUTEX_WAIT_BITSET uses an absolute timeout instead of an relative timeout like FUTEX_WAIT.

Often, FUTEX_WAIT_BITSET is used not for its bitset functionality, but only for its absolute timeout functionality. It is then used with a bitset of u32::MAX.

This adds support for only that use case to Miri, as that's all std currently needs. Any other bitset is still unsupported.

Update: This adds full support for both these syscalls.

assert_eq!(libc::syscall(
libc::SYS_futex,
&futex as *const i32,
libc::FUTEX_WAIT_BITSET,
Copy link
Member Author

Choose a reason for hiding this comment

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

This test also passes if I put FUTEX_WAIT here, as it seems that the monotonic clock starts at zero in the tests. It'd be a better test if it was run with a monotonic clock that does not start at zero.

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, our monotone clock is relative to the time_anchor which is initialized via Instant::now() when the interpreter starts.

I guess we could add an arbitrary offset to that? It also should make a difference if the interpreter has already been running for a bit (e.g. because of a previous timeout test).

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 6, 2022

This adds support for only that use case to Miri, as that's all std currently needs. Any other bitset is still unsupported.

It was pretty easy to add full support for these syscalls. I've updated the PR.

@m-ou-se m-ou-se changed the title Add support for FUTEX_WAIT_BITSET(bitset=MAX). Add support for FUTEX_{WAIT,WAKE}_BITSET Apr 6, 2022
@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

It was pretty easy to add full support for these syscalls. I've updated the PR.

Lol, I think I know that feeling. ;) Thanks a ton!

@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

Could you put 306ba8357fb36212b7d30efb9eb9e41659ac1445 into the rust-version file? Then CI will tell us if this also makes Mutex work again in Miri.

@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

I like green CI. :)
@bors r+

@bors
Copy link
Collaborator

bors commented Apr 6, 2022

📌 Commit 4fdda31 has been approved by RalfJung

@bors
Copy link
Collaborator

bors commented Apr 6, 2022

⌛ Testing commit 4fdda31 with merge 0e2def5...

@bors
Copy link
Collaborator

bors commented Apr 6, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 0e2def5 to master...

@bors bors merged commit 0e2def5 into rust-lang:master Apr 6, 2022
@m-ou-se m-ou-se deleted the futex-wait-bitset branch April 6, 2022 23:03
libc::FUTEX_WAIT_BITSET,
123,
&timeout,
0,
Copy link
Member

Choose a reason for hiding this comment

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

This turns out to be a subtle bug, since 0 becomes 0i32 but this argument should be pointer-sized. #2058 makes Miri detect that problem.

(It'd probably still work on x86 due to details of the calling convention, but e.g. if arguments are passed on the stack then va_arg would probably do the wrong thing here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants