-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
implement unsafe pointer methods #43964
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
Tracking Issue: #43941 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly 👍 on this, but there are some issues I'd like to see resolved, either through convincing me of the wording, or changing the wording.
src/libcore/ptr.rs
Outdated
/// Behaviour: | ||
/// | ||
/// * Both the starting and resulting pointer must be either in bounds or one | ||
/// byte past the end of an allocated object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the formatting here - byte should be indented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
/// `isize`. | ||
/// | ||
/// * The offset being in bounds cannot rely on "wrapping around" the address | ||
/// space. That is, the infinite-precision sum, **in bytes** must fit in a usize. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems unnecessary to state, imo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to construct an offset which wraps around the address space but ends up in bounds, if pointers are modeled as "just" usizes. This would satisfy both previous rules, but fail this one. LLVM explicitly says it's UB to do this with GEP inbounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that assumes the pointers are just usizes. I dunno, maybe I've been smoking the object model for too long.
/// | ||
/// Consider using `wrapping_offset` instead if these constraints are | ||
/// difficult to satisfy. The only advantage of this method is that it | ||
/// enables more aggressive compiler optimizations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this paragraph. wrapping_offset
is, imo, a kludge that really shouldn't ever be used. Maybe recommend add
, and use actual usize
pointer arithmetic in add
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no unsigned version of GEP inbounds, it's impossible to ask LLVM to do this without removing the inbounds modifier, which is what wrapping_*
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
/// | ||
/// Consider using `wrapping_offset` instead if these constraints are | ||
/// difficult to satisfy. The only advantage of this method is that it | ||
/// enables more aggressive compiler optimizations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same complaint as before.
where T: Sized, | ||
{ | ||
self.offset(count as isize) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should use usize
based pointer arithmetic, rather than casting to isize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, there is no such thing in LLVM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can I just say that this is so weird to me. Anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a reason I frequently rant about gep
Well, you've convinced me. I'm a go. |
/// that should be dropped. | ||
/// | ||
/// Additionally, it does not drop `src`. Semantically, `src` is moved into the | ||
/// location pointed to by `dst`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src
and dst
should be val
and self
/// Beyond accepting a raw pointer, this is unsafe because it semantically | ||
/// moves the value out of `self` without preventing further usage of `self`. | ||
/// If `T` is not `Copy`, then care must be taken to ensure that the value at | ||
/// `src` is not used before the data is overwritten again (e.g. with `write`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src
should be self
src/libcore/ptr.rs
Outdated
@@ -555,7 +581,8 @@ impl<T: ?Sized> *const T { | |||
} | |||
|
|||
/// Calculates the offset from a pointer using wrapping arithmetic. | |||
/// `count` is in units of T; e.g. a `count` of 3 represents a pointer | |||
/// | |||
/// `count` is in units of T; e.g. an `count` of 3 represents a pointer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an `count`
-> a `count`
friendly ping @gankro to keep this on your radar! |
All comments addressed, and squashed. One thing I noticed myself: I didn't update *mut's offset docs before. That is now fixed. |
I've pinged @sfackler on IRC for review. |
src/libcore/ptr.rs
Outdated
/// # Safety | ||
/// | ||
/// If any of the following conditions are violated, the result is Undefined | ||
/// Behaviour: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think we lean towards US english spelling in docs?
LGTM other than the one spelling nit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me from the documentation whether these methods may or may not be used with unaligned pointers: *offset
methods, *add
and *sub
methods, copy*
methods, *_volatile
methods, drop_in_place
, *_bytes
methods, replace
, swap
.
Should I simply assume that they are safe to use with unaligned pointers, unless explicitly stated?
It'd be nice if the Safety
section of every method mentioned whether unaligned pointers are allowed anyway.
Other than that, LGTM.
} | ||
|
||
/// Swaps the values at two mutable locations of the same type, without | ||
/// deinitializing either. They may overlap, unlike `mem::swap` which is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By 'they may overlap' you mean 'the locations may be equal'?
What happens if you swap the values at, say, two different but overlapping locations (at least one of the pointers will be unaligned)? Is it allowed to call swap
with unaligned pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need alignment issues -- consider x: [u32; 3]
, where I swap [u32; 2] from x[0] and x[1].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the final value of x[1] is actually unspecified, but definitely one of the original values of x[0] or x[2].
@stjepang Unless explicitly stated otherwise, all functions which take pointers assume that they are aligned. (see |
5f4b91a
to
e64b56a
Compare
e64b56a
to
4f24507
Compare
Made changes to appease american pigs ready to merge |
@bors: r=sfackler |
📌 Commit 4f24507 has been approved by |
⌛ Testing commit 4f24507 with merge bd235a61e5047f090f8a37729145b8e56f290904... |
💔 Test failed - status-travis |
@bors: retry
|
The errors in the documentation I pointed out above are still there. |
implement unsafe pointer methods I also cleaned up some existing documentation a bit here or there since I was doing so much auditing of it. Most notably I significantly rewrote the `offset` docs to clarify safety (`*const` and `*mut`'s offset docs had actually diverged).
⌛ Testing commit 4f24507 with merge 6ae1aead5260aadf8598783fe98fb9210487ddbb... |
@bors: retry
|
implement unsafe pointer methods I also cleaned up some existing documentation a bit here or there since I was doing so much auditing of it. Most notably I significantly rewrote the `offset` docs to clarify safety (`*const` and `*mut`'s offset docs had actually diverged).
☀️ Test successful - status-appveyor, status-travis |
Hey @tormol sorry I thought I got everything. Please file an issue for the ones I missed, or a PR to fix it, so we don't forget! |
I also cleaned up some existing documentation a bit here or there since I was doing so much auditing of it. Most notably I significantly rewrote the
offset
docs to clarify safety (*const
and*mut
's offset docs had actually diverged).