Skip to content
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

Merged
merged 1 commit into from
Sep 16, 2017
Merged

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 17, 2017

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
Copy link
Collaborator

r? @sfackler

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

@Gankra
Copy link
Contributor Author

Gankra commented Aug 17, 2017

Tracking Issue: #43941

@Gankra
Copy link
Contributor Author

Gankra commented Aug 17, 2017

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

Copy link
Contributor

@strega-nil strega-nil left a 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.

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

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.
Copy link
Contributor

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)
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

@strega-nil
Copy link
Contributor

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`.
Copy link
Contributor

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`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src should be self

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an `count` -> a `count`

@arielb1 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
@carols10cents
Copy link
Member

friendly ping @gankro to keep this on your radar!

@Gankra
Copy link
Contributor Author

Gankra commented Aug 28, 2017

All comments addressed, and squashed.

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

@arielb1 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
@aidanhs
Copy link
Member

aidanhs commented Sep 7, 2017

I've pinged @sfackler on IRC for review.

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

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?

@sfackler
Copy link
Member

sfackler commented Sep 7, 2017

LGTM other than the one spelling nit.

Copy link

@ghost ghost left a 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
Copy link

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@strega-nil
Copy link
Contributor

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

@Gankra
Copy link
Contributor Author

Gankra commented Sep 10, 2017

Made changes to appease american pigs

ready to merge

@alexcrichton
Copy link
Member

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Sep 14, 2017

📌 Commit 4f24507 has been approved by sfackler

@bors
Copy link
Contributor

bors commented Sep 15, 2017

⌛ Testing commit 4f24507 with merge bd235a61e5047f090f8a37729145b8e56f290904...

@bors
Copy link
Contributor

bors commented Sep 15, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@tormol
Copy link
Contributor

tormol commented Sep 15, 2017

The errors in the documentation I pointed out above are still there.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017
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
Copy link
Contributor

bors commented Sep 16, 2017

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

@alexcrichton
Copy link
Member

@bors: retry

  • travis got wedged...

@bors
Copy link
Contributor

bors commented Sep 16, 2017

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

bors added a commit that referenced this pull request Sep 16, 2017
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
Copy link
Contributor

bors commented Sep 16, 2017

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

@bors bors merged commit 4f24507 into rust-lang:master Sep 16, 2017
This was referenced Sep 16, 2017
@Gankra
Copy link
Contributor Author

Gankra commented Sep 17, 2017

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.