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

Use pointers instead of &self in Latch::set #1011

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jan 19, 2023

Latch::set can invalidate its own &self, because it releases the
owning thread to continue execution, which may then invalidate the latch
by deallocation, reuse, etc. We've known about this problem when it
comes to accessing latch fields too late, but the possibly dangling
reference was still a problem, like rust-lang/rust#55005.

The result of that was rust-lang/rust#98017, omitting the LLVM attribute
dereferenceable on references to !Freeze types -- those containing
UnsafeCell. However, miri's Stacked Borrows implementation is finer-
grained than that, only relaxing for the cell itself in the !Freeze
type. For rayon, that solves the dangling reference in atomic calls, but
remains a problem for other fields of a Latch.

This easiest fix for rayon is to use a raw pointer instead of &self.
We still end up with some temporary references for stuff like atomics,
but those should be fine with the rules above.

rayon-core/src/latch.rs Outdated Show resolved Hide resolved
Comment on lines 383 to 413
L: Latch,
{
#[inline]
fn set(&self) {
L::set(self);
unsafe fn set(this: *const Self) {
L::set(&**this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This impl makes me uncomfortable. I previously tried to implement this PR and I think I came up with almost exactly the same patch as you have here, but I wasn't sure what to do with this. Of course I couldn't come up with a case where this actually causes an issue, but implementing this for &Latch seems like an invitation to create the references we're trying to avoid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a new LatchRef to replace this -- let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

That is very cool. I like it.

`Latch::set` can invalidate its own `&self`, because it releases the
owning thread to continue execution, which may then invalidate the latch
by deallocation, reuse, etc. We've known about this problem when it
comes to accessing latch fields too late, but the possibly dangling
reference was still a problem, like rust-lang/rust#55005.

The result of that was rust-lang/rust#98017, omitting the LLVM attribute
`dereferenceable` on references to `!Freeze` types -- those containing
`UnsafeCell`. However, miri's Stacked Borrows implementation is finer-
grained than that, only relaxing for the cell itself in the `!Freeze`
type. For rayon, that solves the dangling reference in atomic calls, but
remains a problem for other fields of a `Latch`.

This easiest fix for rayon is to use a raw pointer instead of `&self`.
We still end up with some temporary references for stuff like atomics,
but those should be fine with the rules above.
@cuviper cuviper marked this pull request as ready for review January 20, 2023 20:48
SOF3 added a commit to SOF3/dynec that referenced this pull request Jan 21, 2023
SOF3 added a commit to SOF3/dynec that referenced this pull request Jan 21, 2023
@cuviper
Copy link
Member Author

cuviper commented Jan 21, 2023

bors r+

@bors bors bot merged commit 8cee824 into rayon-rs:master Jan 21, 2023
@cuviper cuviper deleted the latch-set-ptr branch February 25, 2023 17:58
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.

2 participants