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

Implement the os_unfair_lock functions on macOS #3745

Merged
merged 1 commit into from
Jul 14, 2024

Conversation

joboet
Copy link
Member

@joboet joboet commented Jul 13, 2024

These are needed for rust-lang/rust#122408. See the documentation here and the implementation here.

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.

Thanks, this looks quite good! See the comments below.

See the documentation here and the implementation here.

Please add links to this in the code as well.

src/concurrency/sync.rs Outdated Show resolved Hide resolved
src/shims/unix/macos/foreign_items.rs Outdated Show resolved Hide resolved
src/shims/unix/macos/foreign_items.rs Outdated Show resolved Hide resolved
src/shims/unix/macos/foreign_items.rs Outdated Show resolved Hide resolved
libc::os_unfair_lock_assert_not_owner(lock.get());
}

// `os_unfair_lock`s can be moved and leaked:
Copy link
Member

Choose a reason for hiding this comment

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

What if they get moved while being locked?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing, the lock continues to function. We can in all likelihood rely on this, because the lock is unlocked in userspace before entering the kernel, which means the kernel cannot rely on the lock being present when doing the wake, as another thread might lock, unlock and deallocate it immediately. And as the userspace code cannot rely on the lock being fixed either – the structure is too small to contain a pointer – we can do all the moving and leaking we like.

src/provenance_gc.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Looks good, thanks a lot! I also filed #3749 to track adjusting our shared lock implementations so that they can be address-sensitive the way they should be.

Please squash the commits.

@joboet
Copy link
Member Author

joboet commented Jul 14, 2024

I also filed #3749 to track adjusting our shared lock implementations so that they can be address-sensitive the way they should be.

I could change this to use the futex API like the real implementation, which would make this behave exactly the same way.

@RalfJung
Copy link
Member

@bors r+

I could change this to use the futex API like the real implementation, which would make this behave exactly the same way.

That seems a bit awkward to implement inside Miri... also, we'd want to raise UB when doing things that are not guaranteed by the implementation, so having exactly the same implementation does not seem quite right.

@bors
Copy link
Collaborator

bors commented Jul 14, 2024

📌 Commit 7660dde has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 14, 2024

⌛ Testing commit 7660dde with merge d54c18a...

@bors
Copy link
Collaborator

bors commented Jul 14, 2024

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

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