Add [T]::as_ptr_range() and [T]::as_mut_ptr_range(). #65806
Conversation
See rust-lang/rfcs#2791 for motivation.
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
|
r? @Centril |
| #[inline] | ||
| pub fn as_ptr_range(&self) -> Range<*const T> { | ||
| let start = self.as_ptr(); | ||
| let end = unsafe { start.add(self.len()) }; |
There was a problem hiding this comment.
Could you please provide a brief safety argument reasoning around the requirements .add states. (Notably, vec.as_ptr().add(vec.len()) is mentioned.) (Same below.)
There was a problem hiding this comment.
Hm, now I'm starting to wonder if it actually is safe. A Vec is never larger than isize::MAX bytes, but that guarantee is not there for [T], is it?
There was a problem hiding this comment.
I changed it to wrapping_add instead.
There was a problem hiding this comment.
Mmh, I'm wondering where this limitation with isize::MAX is coming from. The documentation doesn't seem to mention why that is, only that this is the case. Is this some LLVM specific problem or something? I don't think this is something Rust is inheriting from the C standard.
There was a problem hiding this comment.
cc @rust-lang/lang @rust-lang/wg-unsafe-code-guidelines
Note that slices point to their entire range, so it is important that the length metadata is never too large. In particular, allocations and therefore slices cannot be bigger than
isize::MAXbytes.
The total size of the slice must be no larger than
isize::MAXbytes in memory. See the safety documentation of pointer::offset.
(Assuming these statements are normative.)
There was a problem hiding this comment.
The first mentions allocations (does that mean heap allocations?),
I think it refers to any sort of allocation, including stack allocations (as your array example indicates).
and the second one seems specific to
alloc.
That's a mistake on my part, it is defined in core as well.
So working on the assumption that this is unspecified / may become implementation-defined it seems to me that we have two choices:
-
Use
.wrapping_add(...)-- potentially a foot-gun if you use.containstho not going to happen for real programs because of LLVM limitations and because of the sheer size of the allocation. -
Use
assert!(...)as above. I'm not sure if LLVM would see the assertion and reason that it must always hold and so optimize it away.
We can probably switch between the two behaviors even after stabilizing because they are so pathological. However, it seems to me we should add a note to the documentation as well as the tracking issue and then the libs team can pick between one of them.
Changed it back to
unsafe { add }, with a comment describing why it should be safe.
Haha; Seems we had the opposite take-aways.
There was a problem hiding this comment.
Ah, the Index implementation of slices assumes the same thing: https://doc.rust-lang.org/nightly/src/core/slice/mod.rs.html#2678-2724
There was a problem hiding this comment.
I don't want to wade into the issue of what's normative, but ptr.offset and related methods with restrictions to isize::MAX bytes are used pervasively from practically all safe slice APIs (at least, anything that ever accesses or takes the address of a slice element). Therefore:
- it is impossible in practice to have a safe slice (in the sense that you can do safe operations on it and rest assures they won't cause UB) that spans more than
isize::MAXbytes - we would have to audit and change a whole bunch of code throughout the standard library (or remove the
isize::MAXrestriction fromoffsetand friends) if we ever wanted to support larger slices
So I think the current implementation (unsafe { add } with comment) is perfectly fine.´
I would advise against wrapping_add because it can cause performance degradation in some situations and there's no reason to risk that.
There was a problem hiding this comment.
Yeah that seems like a good conclusion. Let's keep the PR as-is.
@m-ou-se Could you add a note to the second bullet re. this not being normative (at least not yet) as well as link to rust-lang/unsafe-code-guidelines#102 (comment). With that I think we should be good to merge the PR.
(Also let's keep this thread visible since it was an interesting conversation.)
m-ou-se
force-pushed
the
slice-ptr-range
branch
2 times, most recently
from
ca19770 to
29ef4b8
Compare
m-ou-se
force-pushed
the
slice-ptr-range
branch
from
29ef4b8 to
de9b660
Compare
|
Excellent all around; thanks! @bors r+ rollup |
|
📌 Commit de9b660 has been approved by |
bors
added
S-waiting-on-bors
…range, r=Centril Add [T]::as_ptr_range() and [T]::as_mut_ptr_range(). Implementation of rust-lang/rfcs#2791
|
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 |
|
Ah bummer; my bad! -- I forgot the feature gate in the doctests; you'll need to add those. @bors r- (and r=me again with the gates added to the top of the doc-tests) |
bors
added
S-waiting-on-author
|
@m-ou-se: 🔑 Insufficient privileges: Not in reviewers |
|
@bors r+ |
|
📌 Commit 225b245 has been approved by |
bors
added
S-waiting-on-bors
…range, r=Centril Add [T]::as_ptr_range() and [T]::as_mut_ptr_range(). Implementation of rust-lang/rfcs#2791
|
Ugh, looks like it was still broken. I'm still getting used to Is there any way to make |
m-ou-se
force-pushed
the
slice-ptr-range
branch
from
225b245 to
381c442
Compare
|
Not that I'm aware of; but cc @GuillaumeGomez @bors r+ |
|
📌 Commit 381c442 has been approved by |
…range, r=Centril Add [T]::as_ptr_range() and [T]::as_mut_ptr_range(). Implementation of rust-lang/rfcs#2791
Rollup of 7 pull requests Successful merges: - #63810 (Make <*const/mut T>::offset_from `const fn`) - #65705 (Add {String,Vec}::into_raw_parts) - #65749 (Insurance policy in case `iter.size_hint()` lies.) - #65799 (Fill tracking issue number for `array_value_iter`) - #65800 (self-profiling: Update measureme to 0.4.0 and remove non-RAII methods from profiler.) - #65806 (Add [T]::as_ptr_range() and [T]::as_mut_ptr_range().) - #65810 (SGX: Clear additional flag on enclave entry) Failed merges: r? @ghost
…range, r=Centril Add [T]::as_ptr_range() and [T]::as_mut_ptr_range(). Implementation of rust-lang/rfcs#2791
If you are only changing libstd and not the compiler, I think |
Rollup of 6 pull requests Successful merges: - #65705 (Add {String,Vec}::into_raw_parts) - #65749 (Insurance policy in case `iter.size_hint()` lies.) - #65799 (Fill tracking issue number for `array_value_iter`) - #65800 (self-profiling: Update measureme to 0.4.0 and remove non-RAII methods from profiler.) - #65806 (Add [T]::as_ptr_range() and [T]::as_mut_ptr_range().) - #65810 (SGX: Clear additional flag on enclave entry) Failed merges: r? @ghost
To run doctests, you need the std to be built as far as I know. So unfortunately, I'd say no. :-/
Last time I tried some doc-related checks, you couldn't start them at stage 0. If that changed, it'll make me save a lot of time! |
For me they work but give a weird error that one can seemingly ignore. Even if they don't, you can save a lot of time on re-runs by doing |
Implementation of rust-lang/rfcs#2791