Weak::into_raw #60766
Weak::into_raw #60766
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I'm unfortunately pretty slammed for work right now, @sfackler would you be able to help take a look at this? |
|
Yeah, I can take a look tonight. |
|
Ok, thanks! r? @sfackler (switching bors too) |
src/liballoc/sync.rs
Outdated
| // but the data itself might have been dropped in place by now, so the reference would | ||
| // point to uninitialized memory. That would be invalid reference. But how to get | ||
| // pointer to that memory? | ||
| Some(inner) => &inner.data, |
There was a problem hiding this comment.
@RalfJung has an RFC to cover this kind of issue: rust-lang/rfcs#2582. I think just taking a reference and allowing it to coerce like this code does is the "right" way currently?
There was a problem hiding this comment.
I am afraid there is no right way currently. The RFC also does not provide a syntax that you can already write now that would be deemed right in the future; I tried that but there was explicit opposition claiming there was not enough evidence that this would help.
I'd appreciate a comment in the RFC citing this PR to show that we need to do something.
There was a problem hiding this comment.
Oh, also even with my original RFC you should write &inner.data as *const _; the coercion here might happen after the block ended and that would be too late.
There was a problem hiding this comment.
OK, I think I'll do the reverse trick of what is used to compute the start of the structure. It'll work as long as T is sized, which is currently the case.
|
It does seem a bit unfortunate that these can only be provided for T: Sized, but returning an |
|
(I'm used to adding fixups, not rewriting the history during the review. I'll squash them before merging, once the review is done) |
|
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 |
|
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 |
|
Would anyone have an idea why the Aside from solving these links, what would be the next needed steps to get this merged? I've modified the original issue into a tracking issue (I hope I've done it correctly). |
src/liballoc/rc.rs
Outdated
| /// Consumes the `Weak<T>` and turns it into a raw pointer. | ||
| /// | ||
| /// This converts the weak pointer into a raw pointer, preserving the original weak count. It | ||
| /// can be turned back into the `Weak<T>` with [`from_raw`][Weak::from_raw]. |
There was a problem hiding this comment.
This link is not defined anywhere. You need a
[Weak::from_raw]: some_url_here.html
somewhere in this doc.
My guess is it doesn't even get to check |
The previous commit passed completely green. I haven't touched sync.rs in the latest commit. That's what confuses me. And I believe there's this (nightly only) feature of rustdoc that is able to look them up in the current scope. But maybe I shouldn't be using it in here? |
|
Oh wow, I had no idea. Disregard what I said and ask someone who implemented that new feature. ;) |
That feature doesn't work properly yet so don't bother trying to use it. |
|
Is there something missing from my side, or is everyone too busy to review (which is fine, I just wouldn't like to deadlock, each side thinking it's the other one that needs to do something). |
|
Nope, sorry! Let's get rid of the feature gates test, and then r=me. We don't use those for library features. |
Methods on the Weak to access it as raw pointer to the data.
Methods on the Weak to access it as a raw pointer to the data.
|
OK, I've rewritten the branch without any of the feature flag tests or the related commit. |
|
@bors r+ |
|
📌 Commit 4f1dcb3 has been approved by |
bors
added
S-waiting-on-bors
Weak::into_raw Hello This is my first shot at rust-lang#60728. I'd like to consult it a bit before moving further. The biggest question I have is if this API makes sense. My motivation for it is to be able to store the `Weak` in `AtomicPtr`. For that I don't actually need for the pointer to point to the `T`, any pointer (maybe casted to `usize`) would be good enough, but this mirrors what `Arc` does and could be useful for other things too (like comparing if Arc and Weak point to the same thing without playing with the counts), while some opaque pointer wouldn't. Some secondary questions, if this is deemed desirable are: * The weak pointer may be dangling if it is created by `Weak::new()`. It would make sense to treat this as NULL, but that is incompatible with `T: ?Sized` ‒ both `new()` and `ptr::null()` are available only for sized types. The current implementation is therefore also available only for sized `T`s. It would be possible to allow `?Sized` if the API would be `fn into_raw(self) -> Option<NonNull<T>>` and `fn from_raw(NonNull<T>)`, but that's different API than `Arc` has. What would be preferred? * There's a FIXME in my code about what I suspect could be UB. Is it really UB & how to get the pointer correctly? Is manual offsetting of the pointer the only way? * Am I missing some other necessary thing around the feature gates and such? * Is the documentation understandable? I know writing docs is not my strongest skill :-|. Thinks I'd like to do as part of the PR, but are not yet done: * Turn the referenced issue into tracking issue for the feature flag. * Once the `sync::Weak` is considered reasonable, I'd do the equivalent for `rc::Weak`. * This might call for few more tests than what is currently part of the documentation.
Weak::into_raw Hello This is my first shot at rust-lang#60728. I'd like to consult it a bit before moving further. The biggest question I have is if this API makes sense. My motivation for it is to be able to store the `Weak` in `AtomicPtr`. For that I don't actually need for the pointer to point to the `T`, any pointer (maybe casted to `usize`) would be good enough, but this mirrors what `Arc` does and could be useful for other things too (like comparing if Arc and Weak point to the same thing without playing with the counts), while some opaque pointer wouldn't. Some secondary questions, if this is deemed desirable are: * The weak pointer may be dangling if it is created by `Weak::new()`. It would make sense to treat this as NULL, but that is incompatible with `T: ?Sized` ‒ both `new()` and `ptr::null()` are available only for sized types. The current implementation is therefore also available only for sized `T`s. It would be possible to allow `?Sized` if the API would be `fn into_raw(self) -> Option<NonNull<T>>` and `fn from_raw(NonNull<T>)`, but that's different API than `Arc` has. What would be preferred? * There's a FIXME in my code about what I suspect could be UB. Is it really UB & how to get the pointer correctly? Is manual offsetting of the pointer the only way? * Am I missing some other necessary thing around the feature gates and such? * Is the documentation understandable? I know writing docs is not my strongest skill :-|. Thinks I'd like to do as part of the PR, but are not yet done: * Turn the referenced issue into tracking issue for the feature flag. * Once the `sync::Weak` is considered reasonable, I'd do the equivalent for `rc::Weak`. * This might call for few more tests than what is currently part of the documentation.
Rollup of 11 pull requests Successful merges: - #58975 (Implement `iter::Sum` and `iter::Product` for `Option`) - #60542 (Add Step::sub_usize) - #60555 (Implement nth_back for RChunks(Exact)(Mut)) - #60766 (Weak::into_raw) - #61048 (Feature/nth back chunks) - #61191 (librustc_errors: Move annotation collection to own impl) - #61235 (Stabilize bufreader_buffer feature) - #61249 (Rename Place::local to Place::local_or_deref_local) - #61291 (Avoid unneeded bug!() call) - #61294 (Rename `TraitOrImpl` to `Assoc` and `trait_or_impl` to `assoc`.) - #61297 (Remove LLVM instruction stats and other (obsolete) codegen stats.) Failed merges: r? @ghost
| /// drop(strong); | ||
| /// // But not any more. We can do Weak::as_raw(&weak), but accessing the pointer would lead to | ||
| /// // undefined behaviour. | ||
| /// // assert_eq!(42, unsafe { *Weak::as_raw(&weak) }); |
There was a problem hiding this comment.
Technically the RcBox still exists as long as a weak exists, the only thing that happened here is that it got dropped. But dropping an integer is a NOP. So if you run this in Miri, there's not really UB.
For the Miri test case, I replaced the 42 by a Box::new(42) so drop would actually do something. Then you get UB.
There was a problem hiding this comment.
I see what you mean, but it still feels wrong… I mean, calling some kind of drop_in_place should intuitively turn the memory into some kind of uninitialized again. So I would call this „Not being UB by not noticing“ or something.
But, do you think I should change it a bit (I guess using a String would feel more natural than Box<usize>) just to make sure it is UB?
There was a problem hiding this comment.
That would require making the semantics of drop_in_place special -- which in the future we totally might want to do.
So I guess it is okay to specify this as UB for now, putting it together with other unresolved questions around UB.
There was a problem hiding this comment.
I mean, calling some kind of drop_in_place should intuitively turn the memory into some kind of uninitialized again.
IIUC "use-after-Drop::drop" is not UB in Rust. "use-after-free", "use-after-move", creating an invalid value, etc. are all types of UB, and "use-after-drop" can put the program into an execution path that invokes one of them, but this isn't necessarily the case.
There was a problem hiding this comment.
IMO it would conceptually make sense to consider "use-after-drop" the same as "use-after-move".
There was a problem hiding this comment.
It would probably need to be "use-after-try-drop" (that is, drop is not required to succeed), and miri would need to be able to check it.
There was a problem hiding this comment.
Yeah, I am not sure what should happen if drop panics. I am inclined however to treat it the same.
miri would need to be able to check it.
Definitely. Notice that Miri currently is also not able to check for use-after-move.
There was a problem hiding this comment.
There was a problem hiding this comment.
Now there is: https://github.com/rust-lang/miri/issues/753.
docs: Use String in Rc::into_raw examples It is unclear if accessing an integer after `drop_in_place` has been called on it is undefined behaviour or not, as demonstrated by the discussion in #60766 (review). Avoid these uncertainties by using String which frees memory in its `drop_in_place` to make sure this is undefined behaviour. The message in the docs should be to watch out and not access the data after that, not discussing when one maybe could get away with it O:-).
|
What are the reasons for |
|
Good point. Honestly, the factual reason is because I copy-pasted the function prototypes without thinking. On one hand this is more consistent. On the other, one can use a method in the associated-function syntax too. So maybe changing that to methods on Weak is the way to go. Unless someone protests, I'll get to that in the next few days. |
docs: Use String in Rc::into_raw examples It is unclear if accessing an integer after `drop_in_place` has been called on it is undefined behaviour or not, as demonstrated by the discussion in #60766 (review). Avoid these uncertainties by using String which frees memory in its `drop_in_place` to make sure this is undefined behaviour. The message in the docs should be to watch out and not access the data after that, not discussing when one maybe could get away with it O:-).
…ckler
Make the Weak::{into,as}_raw methods
Because Weak doesn't Deref, so there's no reason for them to be only
associated methods.
As kindly pointed out here rust-lang#60766 (comment) by @chpio.
…ckler
Make the Weak::{into,as}_raw methods
Because Weak doesn't Deref, so there's no reason for them to be only
associated methods.
As kindly pointed out here rust-lang#60766 (comment) by @chpio.
Hello
This is my first shot at #60728. I'd like to consult it a bit before moving further.
The biggest question I have is if this API makes sense. My motivation for it is to be able to store the
WeakinAtomicPtr. For that I don't actually need for the pointer to point to theT, any pointer (maybe casted tousize) would be good enough, but this mirrors whatArcdoes and could be useful for other things too (like comparing if Arc and Weak point to the same thing without playing with the counts), while some opaque pointer wouldn't.Some secondary questions, if this is deemed desirable are:
Weak::new(). It would make sense to treat this as NULL, but that is incompatible withT: ?Sized‒ bothnew()andptr::null()are available only for sized types. The current implementation is therefore also available only for sizedTs. It would be possible to allow?Sizedif the API would befn into_raw(self) -> Option<NonNull<T>>andfn from_raw(NonNull<T>), but that's different API thanArchas. What would be preferred?Thinks I'd like to do as part of the PR, but are not yet done:
sync::Weakis considered reasonable, I'd do the equivalent forrc::Weak.