rename ptr::invalid -> ptr::without_provenance

[RalfJung]

It has long bothered me that ptr::invalid returns a pointer that is actually valid for zero-sized memory accesses. In general, it doesn't even make sense to ask "is this pointer valid", you have to ask "is this pointer valid for a given memory access". We could say that a pointer is invalid if it is not valid for any memory access, but the way this FCP is going, it looks like all pointers will be valid for zero-sized memory accesses.

Two possible alternative names emerged as people's favorites:

1. Something involving dangling, in analogy to NonNull::dangling. To avoid inconsistency with the NonNull method, the address-taking method could be called dangling_at(addr: usize) -> *const T.
2. without_provenance, to be symmetric with the inverse operation ptr.addr_without_provenance() (currently still called ptr.addr() but probably going to be renamed)

I have no idea which one of these is better. I read this comment as expressing a slight preference for something like the second option, so I went for that. I'm happy to go with dangling_at as well.

Cc @rust-lang/opsem

[rustbot]

r? @m-ou-se

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

[scottmcm]

Also, ptr::dangling is a lot like NonNull::dangling except that the address is chosen by the user, so it makes sense to use the same name.

Bikeshed: I might expect that ptr::dangling::<T>() worked to get a *const T just like NonNull::<T>::dangling() gives me a NonNull<T>? So I'm not necessarily convinced that this parallel works that well.

Not that I have a better name in mind. dangling_at or something isn't great.

[CAD97]

I agree that invalid is an unfortunate name given that we're FCP-merge on considering all pointers valid for zero-sized accesses.

I agree that ptr::dangling() is a reasonable name for the functionality, but also that it seems like that name should be a nullary constructor like ptr::NonNull::dangling().

By analogy with ptr::from_exposed_addr, this function could maybe be from_dangling_addr or dangling_with_addr. Or it could be spelled as a two part dangling().with_addr(_), but a direct function serves as a place to attach docs to and to help avoid people from reaching for the single step addr as *mut _ instead.

And just as a note, calling the function from_addr would be a very poor choice, as then the wrong and UB prone ptr::from_addr(ptr.addr()) has no indication that it's a bad idea while also having the nicer names compared to ptr::from_exposed_addr(ptr.expose_addr()).

[CAD97]

CI failure is that hashbrown (cfg(feature = "rustc-dep-of-std")) uses ptr::invalid_mut. Thus when remaining, we'll need to carry a #[deprecated] re-export with the old name for at least long enough to release and update std to using a new version of hashbrown. cc @Amanieu

[RalfJung]

My plan was to fix hashbrown before landing this: rust-lang/hashbrown#481

[RalfJung]

Regarding the name: yes the inconsistency with NonNull::dangling being nullary is a bit annoying. But I thought this wouldn't actually be confusing enough to justify picking a different name; the compiler will easily point out when there are too many / too few arguments.

If we don't want to overload dangling like that, then dangling_with_addr would work, but it is very verbose. dangling_addr would be a bit less consistent but also less verbose. I don't find dangling_at so bad either...

[joboet]

How about loose as a name, as in: "is not tied to an allocated object"?

[RalfJung]

That sounds like a pointer one could use to access any allocated object. Which is not what this is.

[CAD97]

FWIW I've turned around and like dangling as a name just fine again. At worst it's a single added compile-edit cycle to get it wrong, and both ptr::dangling(addr) and ptr::NonNull::dangling() are locally obvious about what they do. layout.dangling() is the odd one out here if any are, honestly.

[RalfJung]

I was considering adding ptr::dangling() and the renaming the variant that takes the address to something longer. But in the end I am fine either way.

Let's see what t-libs-api says.

[RalfJung]

Based on t-libs-api feedback, let's stick to ptr::dangling{,_mut} for now.

@Amanieu this is now blocked on a hashbrown release.

[bors]

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

[RalfJung]

@Amanieu would be nice to get a new release of hashbrown that includes rust-lang/hashbrown#481 so that this PR can move to the next stage. :)

[m-ou-se]

We discussed this in last week's libs-api meeting, but didn't reach a conclusion yet.

It'd be useful to know the common use cases for this function.

Some things that came up in the meeting:

• Inconsistency with NonNull::dangling() isn't great.
• If ptr::dangling() took no arguments, the proposed functionality would be ptr::dangling().with_addr().
  • That's equivalent to ptr::null().with_addr().
  • Depending on the expected use cases, that might be too verbose or unclear.
• Many use cases involve stuffing non-pointer data (some integer or flag) into a pointer or AtomicPtr.
  • For those use cases, the name "invalid" might be fine as is.

[m-ou-se]

I'll note that the methods to go from a pointer to a usize are called .addr() and .expose_addr(), while the opposites are called ptr::invalid() and ptr::from_exposed_addr().

Purely by looking at these names, from_addr would be the most consistent name for ptr::invalid().

That's not very clear though, which might suggest that .addr() should also have a different name, depending on what we end up calling ptr::invalid.

(Edit March 2024: I now think addr is fine. I see no need for symmetry here. See #121588 (comment))

[RalfJung]

That's not very clear though, which might suggest that .addr() should also have a different name, depending on what we end up calling ptr::invalid.

Yeah, addr probably needs a rename. Some alternative names have been mentioned on Zulip, though none of them make me really excited: so far we got lossy_addr/addr_lossy and bare_addr/addr_bare. So the corresponding inverses would be from_lossy_addr (which makes little sense) and from_bare_addr (which makes sense but doesn't warn about the fact that this pointer cannot be used to access any memory).

To me it seems more important to warn about the fact that this pointer cannot be used to access any memory than to be consistent with the inverse function -- ideally we'd do both but I can't think of a way to do that.

[dtolnay]

it seems more important to warn about the fact that this pointer cannot be used to access any memory than to be consistent with the inverse function

👍 to this.

I share the sentiment of not feeling great about either of lossy_addr or bare_addr.

Ralf's comment in rust-lang/unsafe-code-guidelines#478 (comment) caught my eye: I sometimes call them "(raw) integer pointers", but that doesn't really fit the pattern here.

I wonder if there is a way to lean into the interpretation of these being integers dressed as pointers, not so much pointers referring to some (invalid/dangling) "address".

What I mean is going even further than ptr.numerical_addr() / ptr::from_num{ber,erical_addr}, as these names still overly emphasize an address-ness which is not really there. If Ralf calls them an "integer pointer", maybe they're rather a "pointy integer".

pubmod ptr {pubfnpointy_integer<T>(value:usize) - > *constT;pubfnpointy_integer_mut<T>(value:usize) - > *mutT;}

The thing to figure out would be whether a "pointy integer" is the thing you get back from ptr::pointy_integer(value) (integer dressed as a pointer), or the thing you get back from ptr.expose_addr() (pointer dressed as an integer). To me, using "address" to describe the latter makes sense, and that leaves "pointy integer" unambiguously describing the former.

[RalfJung]

It'd be useful to know the common use cases for this function.

• implementing the ptr::null and NonNull::dangling functions -- basically this function is the source of all dangling pointers
• creating pointers that are just sentinel values and will never be dereferenced
• creating a pointer "out of thin air" that is valid for zero-sized reads, e.g. when implementing an iterator that needs a special code path for the ZST case

[bors]

💔 Test failed - checks-actions

[RalfJung]

@bors r=m-ou-se

[bors]

📌 Commit b58f647 has been approved by m-ou-se

It is now in the queue for this repository.

[bors]

⌛ Testing commit b58f647 with merge 3406ada...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 3406ada to master...

[rust-timer]

Finished benchmarking commit (3406ada): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[2.4%, 4.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.2%	[-4.2%, -4.2%]	1
Improvements ✅ (secondary)	-3.4%	[-3.5%, -3.4%]	2
All ❌✅ (primary)	0.9%	[-4.2%, 4.5%]	3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	38
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	38

Bootstrap: 649.698s -> 651.719s (0.31%)
Artifact size: 310.95 MiB -> 310.97 MiB (0.01%)