Skip to content

Implement FreeBSD syscall _umtx_op for futex support #4209

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
Merged
merged 1 commit into from
Apr 10, 2025
Merged

Implement FreeBSD syscall _umtx_op for futex support #4209

merged 1 commit into from
Apr 10, 2025

Conversation

LorrensP-2158466
Copy link
Contributor

@LorrensP-2158466 LorrensP-2158466 commented Feb 25, 2025 •
edited
Loading

Links to #3553.

Currently This implements the WAIT and WAKE operations of the _umtx_op syscall.
Enable sync and concurrency tests in ci/ci.sh and add tests that calls _umtx_op directly.

@LorrensP-2158466
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Feb 25, 2025
@LorrensP-2158466 LorrensP-2158466 changed the title Freebsd futex Implement _umtx_op syscall of Freebsd for futex support Feb 25, 2025
@LorrensP-2158466 LorrensP-2158466 changed the title Implement _umtx_op syscall of Freebsd for futex support Implement FreeBSD syscall _umtx_op for futex support Feb 25, 2025
@RalfJung
Copy link
Member

Thanks for the PR! We have a significant backlog currently so unfortunately it could take a while until you get a proper review, sorry.

I have not yet changed ci/ci.sh because I don't exactly know which filters to pass. Currently if I test it locally only the the tests that use libc::cpuset_getaffinity fail.

You should add sync to have it test the libstd synchronization primitives.

Also, please add a test that directly calls the FreeBSD functions.

RalfJung
RalfJung reviewed Mar 27, 2025
View reviewed changes
Copy link
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.

Sorry for the long wait! These PRs are kinda tricky to review. Overall it looks good but I have a lot of questions. :)

When resolving these comments, please do not force-push or amend commits. Instead, make new commits for your new changes. We'll worry about cleaning up the git history later.

@RalfJung
Copy link
Member

RalfJung commented Apr 1, 2025

@rustbot author
Please post @rustbot ready when the PR is ready for the next round of review.

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 1, 2025
@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented Apr 1, 2025 •
edited
Loading

Those are not my commits, I hope I didn't do something wrong.

As for the changes, I:

  • changed the handling of uaddr and uaddr2 to now being correct
  • fixed/updated/added tests to correctly test behaviour of the syscall
  • updated/added lots of extra comments explaining things or referring to the manual

Only thing that I can think of is problem with the Real Time clock that is used with timespec when running in isolation mode.

@LorrensP-2158466
Copy link
Contributor Author

@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 Apr 1, 2025
@RalfJung
Copy link
Member

RalfJung commented Apr 1, 2025 •
edited
Loading

Yeah something is definitely wrong with the PR, I can't review it in this state. Please make sure only your commits are on this branch. Probably a rebase can fix this.

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Apr 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Apr 1, 2025
@LorrensP-2158466
Copy link
Contributor Author

I think i fixed it, only my commits are present on this branch

@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 Apr 2, 2025
@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2025 •
edited
Loading

In the future, please avoid rebasing unless there are conflicts. Github is pretty bad at dealing with changes to the PR, and rebasing throws it off entirely, causing a bunch of extra work on my side.

@LorrensP-2158466
Copy link
Contributor Author

Sorry ):, you said that a rebase would probably fix it, and it sort of did. I had never seen something like that before and could not find anything that resembled that. Hopefully it doesn't cause to much problems.

RalfJung
RalfJung reviewed Apr 3, 2025
View reviewed changes
Copy link
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.

@rustbot author

src/shims/unix/freebsd/foreign_items.rs Outdated
@@ -55,6 +56,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.write_scalar(res, dest)?;
}

// Futex
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Futex
// Synchronization primitives

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 3, 2025
@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2025

Sorry ):, you said that a rebase would probably fix it, and it sort of did. I had never seen something like that before and could not find anything that resembled that. Hopefully it doesn't cause to much problems.

Yeah you must have done a rebase or a merge first to even cause the problem that the rebase then fixed. That first step is what I referred to. :)

@LorrensP-2158466
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: Waiting for the PR author to address review comments label Apr 6, 2025
@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Apr 6, 2025
@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 7, 2025
@LorrensP-2158466
Copy link
Contributor Author

@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 Apr 8, 2025
RalfJung
RalfJung reviewed Apr 9, 2025
View reviewed changes
Copy link
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.

Just some minor nits for the test. :)

@RalfJung
Copy link
Member

RalfJung commented Apr 9, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 9, 2025
@LorrensP-2158466
Copy link
Contributor Author

The tests should be ok, at least on my machine.

@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 Apr 9, 2025
@RalfJung
Copy link
Member

RalfJung commented Apr 9, 2025

This looks great, thanks! Please squash the commits, then we can land this. Please use the --keep-base flag when squashing so that the force-push diff is easier to review. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 9, 2025
@LorrensP-2158466
Copy link
Contributor Author

Sorry, I have never squashed commits before. And I don't want to break te history Again.

How do I do this correctly?

@RalfJung
Copy link
Member

RalfJung commented Apr 9, 2025

It should be something like git rebase --interactive --keep-base origin/master (you may have to replace origin by something else, that depends on how you named the remote that points to the upstream Miri repo). That will open an editor; change all but the first line to say "squash" instead of "pick", save and close. Another editor will show up where you can pick a reasonable commit message for the one remaining commit.

@LorrensP-2158466
Copy link
Contributor Author

I learned something new about git today, I hope this is what you meant :).

@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 Apr 10, 2025
@RalfJung
Copy link
Member

Looks good, thanks :)

@RalfJung RalfJung enabled auto-merge April 10, 2025 08:45
@RalfJung RalfJung added this pull request to the merge queue Apr 10, 2025
Merged via the queue into rust-lang:master with commit 0238eb3 Apr 10, 2025
7 checks passed
@LorrensP-2158466 LorrensP-2158466 deleted the freebsd_futex branch April 11, 2025 10:35
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

@m-ou-se m-ou-se m-ou-se left review comments

@saethlin saethlin saethlin left review comments

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.
5 participants
@LorrensP-2158466 @RalfJung @rustbot @m-ou-se @saethlin