Explicitly pass strong ref as raw pointer to prevent UB in Arc::drop #58611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @aidanhs (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
vertexclique
force-pushed
the
arc-drop-fetch-sub-explicit-impl
branch
from
7d323b8 to
5f36dc2
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
RalfJung
reviewed
Feb 21, 2019
| pub fn fetch_sub_explicit(f: *const $atomic_type, | ||
| val: $int_type, | ||
| order: Ordering) -> $int_type { | ||
| unsafe { atomic_sub((*f).v.get(), val, order) } |
There was a problem hiding this comment.
What's the type of atomic_sub? Won't this create a reference again?
There was a problem hiding this comment.
atomic_sub takes *mut T:
rust/src/libcore/sync/atomic.rs
Line 2176 in 5f36dc2
There was a problem hiding this comment.
Okay, thanks. So this would indeed fix the problem even under the most strict semantics -- but at the cost of duplicating the entire API surface of the Atomic* types.
There was a problem hiding this comment.
True that. See my comment under the PR description:
- I am unsure how this can be done without extending the public api of
AtomicUsize.
edit: basically all Atomic* i meant.
|
Thanks for this PR! However, I think it is too early to implement something here. We first need to do some design work and figure out how we want to solve this problem -- this has some ecosystem wide consequences in terms of which code is or is not UB. |
|
Then, this can stay in here for further reference. |
|
Do you mean we should merge this for further experimentation? Or keep the PR? We don't usually keep PRs open that we do not intend to merge soon. That just clutters the PR list. But you could add a link to #55005, so even if we close it we can still find this proposed solution. |
|
Not keeping the PR but the reference to it. It is up to you to merge this or not. Not my word in here, since you want to go to the stacked borrows direction, and major review needs to be done in code and we need to come under a conclusion for UB or not UB condition. I am adding the reference now. But can't say a word for PR acceptance for experimentation. |
|
r? @RalfJung |
|
I need to fix the doc example of public api of |
|
As discussed before, I don't think we want to land this before we decided that we really want to duplicate the Nevertheless, thanks a lot @vertexclique! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As stated out in #55005 I try to apply the first solution stated by @RalfJung . Which is:
I want to add couple of questions and notes about PR:
Atomic*.ArcInner<T>so while using thefetch_*of strong ref. how we can pass a raw pointer of strong ref? Is it correct way of doing it?Arc::drop? I have no idea.