Add functions core::ptr::dangling/_mut<T>() -> const/mut T #45527

bluss · 2017-10-25T17:03:21Z

Add functions to create a dangling aligned non-null raw pointer. The
use case is to fix all the places we use "1 as *mut T", in
particular in the slice iterators.

The aligned pointer has a value like 0x1, 0x4, 0x8 etc. depending on the
alignment of the pointed-to type. It is useful when a non-null pointer
is required like in references, slices, boxes, and other places.

NOTE: This changes the observable behaviour of slice iterators.
Previously, it would always yield 0x1 pointers (as references) for ZST,
including types like [SomeType; 0]. They now use an aligned non-null
pointer.

Expose std::ptr::dangling/_mut as new unstable functions with intent
on later stabilization, because if this low level trick is needed in
collections, the rest of the Rust ecosystem will need them as well.

rust-highfive · 2017-10-25T17:03:24Z

r? @aturon

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

bluss · 2017-10-25T17:04:35Z

Choices, choices.

We could have two versions, one for const and one for mut? I don't know if we should bother with that.
Should it be a static method on raw pointers instead of a free function?

Gankra · 2017-10-25T17:26:20Z

This seem fine. API should exactly match ptr::null(_mut), imo.

petrochenkov · 2017-10-25T17:35:05Z

empty sounds too benign, IMO, almost like default or null, as if it were something valid and not a hack to workaround non-null/alignment attributes.

bluss · 2017-10-25T17:36:16Z

@petrochenkov I see that, maybe "dangling" is better? empty follows the existing Unique/Shared method names.

petrochenkov · 2017-10-25T17:37:43Z

@bluss
invalid_nonnull_aligned? 😄

bluss · 2017-10-25T17:38:38Z

Is std::ptr::nonnull() too confusing? It could std::ptr::dummy() instead, for the dummiest but simplest name.

Gankra · 2017-10-25T17:54:57Z

I vote dangling if we can't use empty. It's clearly !!!scary!!! but easy to learn and remember.

Mark-Simulacrum · 2017-10-25T18:11:35Z

Marking as relnotes due to the potential breakage with ZST iterators.

bluss · 2017-10-25T18:57:06Z

Rewritten to use std::ptr::dangling/_mut.

kennytm · 2017-10-25T19:19:17Z

src/libcore/ptr.rs

+///
+/// This function is safe to call, but the raw pointer it returns does not
+/// point to any value.
+pub fn dangling<T>() -> *const T {


Can this function be const, just like null()?

Ah great idea, align_of is const so it should be no problem.

bluss · 2017-10-25T19:34:56Z

Renaming Unique::empty() / Shared::empty() to the new "dangling" names should probably happen as well, but it could be another PR?

bors · 2017-10-27T23:15:01Z

☔ The latest upstream changes (presumably #45285) made this pull request unmergeable. Please resolve the merge conflicts.

durka · 2017-10-29T18:35:59Z

Please add #[rustc_const_unstable] attributes (we should add this to the unmarked-api lint).

durka · 2017-10-29T18:36:43Z

Or maybe we can hold off adding those until the functions themselves are stabilized?

nagisa · 2017-11-02T17:30:03Z

src/libcore/ptr.rs

+/// # Safety
+///
+/// This function is safe to call, but the raw pointer it returns does not
+/// point to any value.


nit: This is not necessarily true. For types with very large alignments, this may point to outside the 0th page (the page that’s guaranteed to be unmapped on most systems) and actually point to something (e.g. manually mapped 1st page). I would reword this to not mention value validity and rather mention that dereferencing the pointer is invalid, or something along the lines.

My first draft was that dereferencing the pointer is invalid. While that's true in the "read memory" sense, in the higher level Rust semantic, dereferencing a pointer to ZST is fine.

😄 I'm having trouble finding the right words that navigate all of this.

I think any integer_constant as *const T pointer is invalid to use by these rules, since it's not based on anything http://llvm.org/docs/LangRef.html#pointer-aliasing-rules

So it must be treated as "truly dangling" or use some highly implementation defined way to access an actual value through that pointer.

In that sense, it seems best to just say it does not point to any value. If it's equal to a bona fide pointer to a value, that's incidental?

I think any integer_constant as *const T pointer is invalid to use by these rules

This isn’t exactly true, because pretty much any low level code that depends on stuff at well known addresses would be invalid (embedded/low level/OS code relies heavily on that, for example). I believe this rule from the page you’ve linked applies here:

An integer constant other than zero or a pointer value returned from a function not defined within LLVM may be associated with address ranges allocated through mechanisms other than those provided by LLVM. Such ranges shall not overlap with any ranges of addresses allocated by mechanisms provided by LLVM.

This rule does not forbid creation and use of such address ranges, it only forbids overlap with address ranges generated by concepts understood by LLVM (alloca, malloc, ...).

That makes sense. I have pushed a change that has no Safety section, because I have no useful advice apart from what's already in the other doc.

bluss · 2017-11-02T18:37:23Z

Updated to simplify docs after discussion with @nagisa. We can't say so much more than non-null and aligned about the return value.

nagisa · 2017-11-03T15:55:11Z

I have reviewed the functional changes, but am uncomfortable taking a decision whether the breakage this potentially causes is acceptable. I feel that it is, but people "higher up" might feel otherwise.

bluss · 2017-11-04T13:09:39Z

Box of Zst has already made the corresponding breaking change, and this catches up with that in the slice iterator.

Gankra · 2017-11-04T15:53:45Z

Yeah when I changed Vec I think I shook the ecosystem loose from expecting the old heap::EMPTY (they now check <= alignof).

shepmaster · 2017-11-11T01:06:44Z

Randomly reassigning to...

r? @alexcrichton

alexcrichton · 2017-11-12T18:03:13Z

I think I may not be quite up to speed on this area, but mind explaining again the motivation behind these functions? For example is the current iteration today incorrect? Memory unsafe?

bluss · 2017-11-12T18:21:16Z

Shared/Unique (So Rc, Box, Vec etc) had the corresponding change of pointer value in #41064. @gankro would know most.

My idea is that this is something we're gradually making more correct so that we end up more consistent. For example that even ZST pointer values are aligned. As usual, it's hard to find any memory safety issues with any pointer to ZST.

I think that if Box<[T; 0]> has a pointer that is aligned for [T; 0], then std::slice::Iter<[T; 0]> should emit references (&[T; 0]) that match that. This PR changes the slice iterator in that way.

alexcrichton · 2017-11-14T21:12:32Z

I feel like internal changes are fine, but I'm just wary to increase the API surface area of the standard library because it "makes us feel good" rather than having a technical driver under the hood?

scottmcm · 2017-11-14T22:23:44Z

For example is the current iteration today incorrect? Memory unsafe?

I think that, technically, we're currently relying on UB. This code

unsafe { std::ptr::read(1 as *const [i64;0]); }

gets trans'd as

call void @llvm.memcpy.p0i8.p0i8.i64(i8* %3, i8* %4, i64 0, i32 8, i1 false)

specifying an alignment to the llvm intrinsic that's a lie.

Probably the pass ordering means that the 0-byte memcpy is DCE'd before any kind of value propagation can assume the low three bits of the pointer are zero, but I'm pretty sure LLVM would be allowed to replace this code with unreachable since "if the call to this intrinsic has an alignment value that is not 0 or 1, then the caller guarantees that both the source and destination pointers are aligned to that boundary".

Gankra · 2017-11-14T22:36:53Z

It can be considered a bug that accesses to ZSTs ever make it to llvm.

nagisa · 2017-11-15T05:55:45Z

Why? LLVM documentation is phrased in a way that does not invalidate loading a ZST type through any pointer. Sure there's some inefficiency in the IR we generate as they are not strictly necessary, but I wouldn't call it a bug.

…

On Nov 15, 2017 00:37, "Alexis Beingessner" ***@***.***> wrote: It can be considered a bug that accesses to ZSTs *ever* make it to llvm. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45527 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0oEDA_72wPikxd7HqfwgZ7xA4kpwks5s2hYggaJpZM4QGVzA> .

alexcrichton · 2017-11-17T19:34:22Z

@scottmcm thanks for the info! That definitely sounds bad and like possible UB. I agree with @gankro though in that we probably shouldn't be generating that in the first place?

In that sense I'm still a little unclear on the motivation here in the sense that there doesn't seem to be a fundamental technical reason for why we would want such functions.

bluss · 2017-11-18T00:34:51Z

Moving towards aligned references is something we should do, now that it has begun. The soft thing about that is that it's a gradual process, tightening the screws in various places before we are completely in shape.

The motivation for the functions was the rather generic one: Expose std::ptr::dangling/_mut as new unstable functions with intent on later stabilization, because if this low level trick is needed in collections, the rest of the Rust ecosystem will need them as well. I sounds like it's too much of a bother to do it that way. In this case we can easily abstain since users can reimplement the function instead.

scottmcm · 2017-11-18T01:45:01Z

I do think it makes sense for the rule to be "reading through a pointer requires that it's aligned", without a "unless the size is zero" exception. And whether the memcpy is right or not, it feels like a ZST load ought to trans as something, if only an assume(p != 0).

Whether there's a need for these methods to be stabilized is a fair question, though. Something similar to dangling() can even be done as [].as_ptr() these days...

alexcrichton · 2017-11-18T08:14:22Z

@bluss I'm afraid that doesn't answer my question though? I still don't know why we want to move towards aligned raw pointers in the first place?

@scottmcm but this still doesn't feel like a technical argument in favor of more API surface area in the standard library? It would seem natural to me that a ZST does absolutely nothing in trans (no instructions emitted) so in that sense I'm still unsure on the motivation to add new stable surface area to the standard library.

bluss · 2017-11-18T12:48:44Z

@alexcrichton I see. It's not about raw pointers, it's to be used by all the other ones (Unique, Shared, &, etc).

alexcrichton · 2017-11-19T16:23:58Z

Ok I think that makes sense to me! So we'll be handing out safe references from those types and those require valid alignment? (in that &T should always be aligned for any T).

Right now though you can't pull a "safe" reference without using unsafe, though?

bors · 2017-11-21T05:41:29Z

☔ The latest upstream changes (presumably #45039) made this pull request unmergeable. Please resolve the merge conflicts.

Add functions to create a dangling aligned non-null raw pointer. The use case is to fix all the places we use "1 as *mut T" is used, in particular in the slice iterators. The aligned pointer has a value like 0x1, 0x4, 0x8 etc. depending on the alignment of the pointed-to type. It is useful when a non-null pointer is required like in references, slices, boxes, and other places. NOTE: This changes the observable behaviour of slice iterators. Previously, it would always yield 0x1 pointers (as references) for ZST, including types like `[SomeType; 0]`. They now use an aligned non-null pointer. Expose std::ptr::dangling/_mut as new unstable functions with intent on later stabilization, because if this low level trick is needed in collections, the rest of the Rust ecosystem will need them as well.

bluss · 2017-11-21T17:56:39Z

(Rebased & bonus squashing into logical parts)

bluss · 2017-11-23T23:05:23Z

Closing with intent on picking up again after the slice iterator implementation change is worked out. (#46223)

rust-highfive assigned aturon Oct 25, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2017

Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 25, 2017

bluss force-pushed the ptr-empty branch from d8a1550 to e40e64a Compare October 25, 2017 18:55

bluss changed the title ~~Add function core::ptr::empty<T>() -> *mut T~~ Add functions core::ptr::dangling/_mut<T>() -> *const/*mut T Oct 25, 2017

bluss force-pushed the ptr-empty branch from e40e64a to 1aedc69 Compare October 25, 2017 18:56

bluss force-pushed the ptr-empty branch from 1aedc69 to ac02c53 Compare October 25, 2017 18:59

kennytm reviewed Oct 25, 2017

View reviewed changes

bluss force-pushed the ptr-empty branch from e02c27f to 7b2da5e Compare October 25, 2017 20:38

oli-obk mentioned this pull request Oct 26, 2017

Lint casting integers to pointers rust-lang/rust-clippy#2177

Open

bluss mentioned this pull request Oct 26, 2017

Tracking issue for core::ptr::dangling/_mut (feature ptr_dangling) #45557

Closed

bluss force-pushed the ptr-empty branch from 43e7936 to 20fd833 Compare October 28, 2017 08:16

nagisa reviewed Nov 2, 2017

View reviewed changes

shepmaster added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 11, 2017

rust-highfive assigned alexcrichton and unassigned aturon Nov 11, 2017

bluss added 3 commits November 21, 2017 18:40

std: use ptr::dangling_mut() in thread locals and related code

65a9cb6

Add test for aligned references from slice iterators

481db9f

bluss force-pushed the ptr-empty branch from bc8738b to 481db9f Compare November 21, 2017 17:41

bluss closed this Nov 23, 2017

oli-obk mentioned this pull request Dec 20, 2017

Lint casting integers to dangling pointers rust-lang/rust-clippy#2280

Closed

kennytm mentioned this pull request Jan 9, 2018

Rename std::ptr::Shared to NonNull and stabilize it #46952

Merged

ishitatsuyuki mentioned this pull request Apr 14, 2018

Zero Page Optimization rust-lang/rfcs#2400

Closed

Add functions core::ptr::dangling/_mut<T>() -> *const/*mut T #45527

Add functions core::ptr::dangling/_mut<T>() -> *const/*mut T #45527

Conversation

bluss commented Oct 25, 2017 • edited Loading

rust-highfive commented Oct 25, 2017

bluss commented Oct 25, 2017

Gankra commented Oct 25, 2017

petrochenkov commented Oct 25, 2017

bluss commented Oct 25, 2017 • edited Loading

petrochenkov commented Oct 25, 2017

bluss commented Oct 25, 2017

Gankra commented Oct 25, 2017

Mark-Simulacrum commented Oct 25, 2017

bluss commented Oct 25, 2017

kennytm Oct 25, 2017

Choose a reason for hiding this comment

bluss Oct 25, 2017

Choose a reason for hiding this comment

bluss commented Oct 25, 2017

bors commented Oct 27, 2017

durka commented Oct 29, 2017

durka commented Oct 29, 2017

nagisa Nov 2, 2017

Choose a reason for hiding this comment

bluss Nov 2, 2017

Choose a reason for hiding this comment

bluss Nov 2, 2017

Choose a reason for hiding this comment

bluss Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

bluss Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

nagisa Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

bluss Nov 2, 2017

Choose a reason for hiding this comment

bluss commented Nov 2, 2017

nagisa commented Nov 3, 2017

bluss commented Nov 4, 2017

Gankra commented Nov 4, 2017

shepmaster commented Nov 11, 2017

alexcrichton commented Nov 12, 2017

bluss commented Nov 12, 2017

alexcrichton commented Nov 14, 2017

scottmcm commented Nov 14, 2017

Gankra commented Nov 14, 2017

nagisa commented Nov 15, 2017 via email

alexcrichton commented Nov 17, 2017

bluss commented Nov 18, 2017

scottmcm commented Nov 18, 2017

alexcrichton commented Nov 18, 2017

bluss commented Nov 18, 2017

alexcrichton commented Nov 19, 2017

bors commented Nov 21, 2017

bluss commented Nov 21, 2017

bluss commented Nov 23, 2017

Add functions core::ptr::dangling/_mut<T>() -> const/mut T #45527

Add functions core::ptr::dangling/_mut<T>() -> const/mut T #45527

bluss commented Oct 25, 2017 •

edited

Loading

bluss commented Oct 25, 2017 •

edited

Loading

bluss Nov 2, 2017 •

edited

Loading

bluss Nov 2, 2017 •

edited

Loading

nagisa Nov 2, 2017 •

edited

Loading