implement unsafe pointer methods #43964

Gankra · 2017-08-17T21:47:28Z

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).

rust-highfive · 2017-08-17T21:47:39Z

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

Gankra · 2017-08-17T21:47:57Z

Tracking Issue: #43941

Gankra · 2017-08-17T21:48:14Z

cc @ubsan and @nagisa on the changes to offset's docs

strega-nil

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.

strega-nil · 2017-08-18T07:37:50Z

src/libcore/ptr.rs

+    /// Behaviour:
+    ///
+    /// * Both the starting and resulting pointer must be either in bounds or one
+    /// byte past the end of an allocated object.


the formatting here - byte should be indented.

Good catch.

strega-nil · 2017-08-18T07:39:13Z

src/libcore/ptr.rs

+    ///   `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.


this seems unnecessary to state, imo?

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.

Yeah, but that assumes the pointers are just usizes. I dunno, maybe I've been smoking the object model for too long.

strega-nil · 2017-08-18T07:40:15Z

src/libcore/ptr.rs

+    ///
+    /// 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.


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 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.

strega-nil · 2017-08-18T07:41:42Z

src/libcore/ptr.rs

+    ///
+    /// 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.


same complaint as before.

strega-nil · 2017-08-18T07:42:11Z

src/libcore/ptr.rs

+        where T: Sized,
+    {
+        self.offset(count as isize)
+    }


This seems like it should use usize based pointer arithmetic, rather than casting to isize.

As above, there is no such thing in LLVM.

can I just say that this is so weird to me. Anyways.

There is a reason I frequently rant about gep

strega-nil · 2017-08-19T01:10:48Z

Well, you've convinced me. I'm a go.

tormol · 2017-08-21T12:30:56Z

src/libcore/ptr.rs

+    /// that should be dropped.
+    ///
+    /// Additionally, it does not drop `src`. Semantically, `src` is moved into the
+    /// location pointed to by `dst`.


src and dst should be val and self

tormol · 2017-08-21T12:33:44Z

src/libcore/ptr.rs

+    /// 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`,


src should be self

ollie27 · 2017-08-21T22:06:39Z

src/libcore/ptr.rs

@@ -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


an `count` -> a `count`

carols10cents · 2017-08-28T12:38:24Z

friendly ping @gankro to keep this on your radar!

Gankra · 2017-08-28T16:38:40Z

All comments addressed, and squashed.

One thing I noticed myself: I didn't update *mut's offset docs before. That is now fixed.

aidanhs · 2017-09-07T02:09:03Z

I've pinged @sfackler on IRC for review.

sfackler · 2017-09-07T05:10:29Z

src/libcore/ptr.rs

+    /// # Safety
+    ///
+    /// If any of the following conditions are violated, the result is Undefined
+    /// Behaviour:


Nit: I think we lean towards US english spelling in docs?

sfackler · 2017-09-07T05:14:12Z

LGTM other than the one spelling nit.

ghost

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.

ghost · 2017-09-10T15:40:09Z

src/libcore/ptr.rs

+    }
+
+    /// Swaps the values at two mutable locations of the same type, without
+    /// deinitializing either. They may overlap, unlike `mem::swap` which is


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?

You don't need alignment issues -- consider x: [u32; 3], where I swap [u32; 2] from x[0] and x[1].

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].

strega-nil · 2017-09-10T19:41:35Z

@stjepang Unless explicitly stated otherwise, all functions which take pointers assume that they are aligned. (see read vs read_unaligned)

Gankra · 2017-09-10T20:10:08Z

Made changes to appease american pigs

ready to merge

alexcrichton · 2017-09-14T22:55:51Z

@bors: r=sfackler

bors · 2017-09-14T22:55:51Z

📌 Commit 4f24507 has been approved by sfackler

bors · 2017-09-15T11:40:43Z

⌛ Testing commit 4f24507 with merge bd235a61e5047f090f8a37729145b8e56f290904...

bors · 2017-09-15T15:02:07Z

💔 Test failed - status-travis

alexcrichton · 2017-09-15T15:43:08Z

@bors: retry

known spurious Spurious failure in test_process_output_fail_to_start #43283

tormol · 2017-09-15T20:41:59Z

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).

bors · 2017-09-16T12:49:47Z

⌛ Testing commit 4f24507 with merge 6ae1aead5260aadf8598783fe98fb9210487ddbb...

alexcrichton · 2017-09-16T15:28:27Z

@bors: retry

travis got wedged...

bors · 2017-09-16T19:55:04Z

⌛ Testing commit 4f24507 with merge 277476c...

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).

bors · 2017-09-16T22:20:47Z

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 277476c to master...

Gankra · 2017-09-17T01:09:56Z

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!

rust-highfive assigned sfackler Aug 17, 2017

strega-nil suggested changes Aug 18, 2017

View reviewed changes

tormol reviewed Aug 21, 2017

View reviewed changes

ollie27 reviewed Aug 21, 2017

View reviewed changes

arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 22, 2017

Gankra force-pushed the unsafe-reform branch from edb9efd to 5f4b91a Compare August 28, 2017 16:44

arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2017

sfackler reviewed Sep 7, 2017

View reviewed changes

ghost reviewed Sep 10, 2017

View reviewed changes

Gankra force-pushed the unsafe-reform branch from 5f4b91a to e64b56a Compare September 10, 2017 20:05

implement unsafe pointer methods

4f24507

Gankra force-pushed the unsafe-reform branch from e64b56a to 4f24507 Compare September 10, 2017 20:09

alexcrichton mentioned this pull request Sep 16, 2017

Rollup of 27 pull requests #44621

Closed

frewsxcv mentioned this pull request Sep 16, 2017

ptr::swap docs needs clarfication #44479

Closed

bors merged commit 4f24507 into rust-lang:master Sep 16, 2017

This was referenced Sep 16, 2017

Rollup of 19 pull requests #44634

Merged

More align_offset things #44537

Merged

alexcrichton mentioned this pull request Jan 3, 2018

Regression: issue #43941 (pointer methods) causes code in nvml crate to no longer compile #47159

Closed

implement unsafe pointer methods #43964

implement unsafe pointer methods #43964

Conversation

Gankra commented Aug 17, 2017

rust-highfive commented Aug 17, 2017

Gankra commented Aug 17, 2017

Gankra commented Aug 17, 2017

strega-nil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Aug 28, 2017

Gankra commented Aug 28, 2017

aidanhs commented Sep 7, 2017

Choose a reason for hiding this comment

sfackler commented Sep 7, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil commented Sep 10, 2017

Gankra commented Sep 10, 2017

alexcrichton commented Sep 14, 2017

bors commented Sep 14, 2017

bors commented Sep 15, 2017

bors commented Sep 15, 2017

alexcrichton commented Sep 15, 2017

tormol commented Sep 15, 2017 • edited Loading

bors commented Sep 16, 2017

alexcrichton commented Sep 16, 2017

bors commented Sep 16, 2017

bors commented Sep 16, 2017

Gankra commented Sep 17, 2017

tormol commented Sep 15, 2017 •

edited

Loading