Document that slices cannot be larger than isize::MAX bytes
#53784
Conversation
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ rollup |
|
📌 Commit f2cd6accf2cf85f5129641800d4fb5796a0b85ca has been approved by |
bors
added
S-waiting-on-bors
|
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 |
|
@bors r- Documentation link seems to be broken |
bors
added
S-waiting-on-author
tbu-
force-pushed
the
pr_doc_slice_isize_max
branch
from
f2cd6ac
to
5d59ede
Compare
|
cc @rust-lang/wg-unsafe-code-guidelines |
|
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 |
tbu-
force-pushed
the
pr_doc_slice_isize_max
branch
from
5d59ede
to
ff6fd5f
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 |
tbu-
force-pushed
the
pr_doc_slice_isize_max
branch
from
ff6fd5f
to
e2c3206
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 |
|
☔ The latest upstream changes (presumably #53928) made this pull request unmergeable. Please resolve the merge conflicts. |
tbu-
force-pushed
the
pr_doc_slice_isize_max
branch
from
e2c3206
to
075a256
Compare
Copy the documentation over to `slice::from_raw_parts_mut`.
tbu-
force-pushed
the
pr_doc_slice_isize_max
branch
from
075a256
to
7b77508
Compare
| #[inline] | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| pub unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] { | ||
| debug_assert!(data as usize % mem::align_of::<T>() == 0, "attempt to create unaligned slice"); | ||
| debug_assert!(len * mem::size_of::<T>() <= isize::MAX as usize, | ||
| "attempt to create slice covering half the address space"); |
There was a problem hiding this comment.
Are we sure that "exactly as large as half the address space" works correctly? That length (in bytes) does already not fit into a signed integer, so computing the address one-past-the-end (e.g. for iteration) would overflow.
There was a problem hiding this comment.
This seems like it'd be best written as len < isize::MAX as usize / mem::size_of::<T>()
There was a problem hiding this comment.
@tbu- not clearer, but it fixes overflow (and also happens at compile-time, as opposed to run-time)
|
Fixed the edge case |
|
@ubsan I think I fixed it now, can you take a look again? The ZST case is checked independently because division by 0 panics, and the |
|
I would vastly prefer |
src/libcore/slice/mod.rs
Outdated
| /// [`from_raw_parts`] for more details. | ||
| /// slices as with [`from_raw_parts`]. The total size of the slice must be no | ||
| /// larger than `isize::MAX` **bytes** in memory. See the safety documentation | ||
| /// of [`pointer::offset`]. |
There was a problem hiding this comment.
This looks a bit strange, with two "See ..." sentences right after one another. Also you made this new remark a new paragraph in the other method. Maybe remove the last sentence of this paragraph? Or else restore this paragraph to its original form, and add a new paragraph for the size limit.
|
@rkruppe So the max size would be "2 less than half the address space"? I somehow don't feel good about that, this just looks like we don't know what we are doing... |
More so than if it was one less than half? But it's true, I for one don't really know what we're doing here 😅 or rather, I could probably (re-)learn it but I do not sufficiently trust myself to put the results in the docs of an unsafe function. |
|
cc @gankro |
Well, yes. Because half the address space is the smallest offset not representable in an |
|
Curiously, the "does the type fit the architecture" check rejects even EDIT: The largest possible one seems to be isize::MAX as usize / 16 / 16 / 16 / 16. Which is a power of two. I'd have expected one less than a power of two as the largest possible... |
|
There's a bit of fuzziness at the upper boundaries of sizes as different compiler components can make incredibly reasonable but technically limiting assumptions to pack sizes into bitfields. I expect it'd be pretty hard to accurately find the exact upper bound of type size that isn't miscompiled/ICEd. But yeah isize::MAX bytes should be possible for a heap allocated slice. |
|
So the limit I found originates from rust/src/librustc_target/abi/mod.rs Lines 164 to 182 in b80cb47 1 << 15 as size which it should not...
EDIT: Ah no, everyone compares with EDIT2: Oh, my local calculate just rounds differently. D'oh. |
|
So based on this indeed I think the max value should be |
|
I am inclined to accept once my last doc nit is resolved. |
|
Ping from triage @tbu-: It looks like some small changes have been requested to your PR. |
kennytm
added
S-waiting-on-review
|
LGTM! @bors r+ |
|
📌 Commit e370b1c has been approved by |
bors
added
S-waiting-on-bors
…Jung Document that slices cannot be larger than `isize::MAX` bytes Fixes rust-lang#53676.
Rollup of 13 pull requests Successful merges: - #53784 (Document that slices cannot be larger than `isize::MAX` bytes) - #54308 (Better user experience when attempting to call associated functions with dot notation) - #54488 (in which we include attributes in unused `extern crate` suggestion spans) - #54544 (Indicate how to move value out of Box in docs.) - #54623 (Added help message for `impl_trait_in_bindings` feature gate) - #54641 (A few cleanups and minor improvements to rustc/infer) - #54656 (Correct doc for WorkQueue<T>::pop().) - #54674 (update miri) - #54676 (Remove `-Z disable_ast_check_for_mutation_in_guard`) - #54679 (Improve bug! message for impossible case in Relate) - #54681 (Rename sanitizer runtime libraries on OSX) - #54708 (Make ./x.py help <cmd> invoke ./x.py <cmd> -h on its own) - #54713 (Add nightly check for tool_lints warning)
Fixes #53676.