Add sub_ptr on pointers (the usize version of offset_from)

[scottmcm]

We have add/sub which are the usize versions of offset, this adds the usize equivalent of offset_from. Like how .add(d) replaced a whole bunch of .offset(d as isize), you can see from the changes here that it's fairly common that code actually knows the order between the pointers and wants a usize, not an isize.

As a bonus, this can do sub nuw+udiv exact, rather than sub+sdiv exact, which can be optimized slightly better because it doesn't have to worry about negatives. That's why the slice iterators weren't using offset_from, though I haven't updated that code in this PR because slices are so perf-critical that I'll do it as its own change.

This is an intrinsic, like offset_from, so that it can eventually be allowed in CTFE. It also allows checking the extra safety condition -- see the test confirming that CTFE catches it if you pass the pointers in the wrong order.

[rust-highfive]

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[Gankra]

Sorry for the drive-by comment (rant?) but just wanted to leave the note somewhere: since i've been poking these APIs, this has been the second-most-brain-melty interface in ptr (align_offset of being the undefeated champion).

• offset_from refuses to say the math expression it's actually expressing (is it x-y or y-x??)
• you don't actually use these APIs when you care about offset, you care about them when you want to know the distance
• _from is totally backwards and makes things feel double or triple negative

Like my guess is that the intent is that x.offset_from(y) is a drop-in for any existing x - y but: the docs don't say that and instead appeal to offset, and any time I was doing x - y I was already struggling to hold the right way around in my brain, and most appealing to "well you want unsigned semantics, so you don't want underflow, so put the big one first". offset_from scuppers this because it's explicitly signed... so yeah having unsigned APIs would be super great for helping with that bit!

This API is (at least in std) pretty universally used for computing array length via pointer distance... and yes you do this with end - start but that's an implementation detail and not actually what you're trying to think about. You really are trying to ask "how many items are in the range start..end", and subtract is the means to that end. Reproducing the "double negative" of putting things backwards is understandable but I think missing an opportunity.

imo things end up a lot clearer if you just make this start.offset_to(end) because it's like, yeah that's what I'm always doing. I offset from the start to the end, which is always positive. And I offset to, not from.

And then yeah you always want it unsigned so... what you really want is unsigned_offset_to. But this is an Objectively Better API In Every Way to offset_from (just as add is objectively better than offset), so it should be shorter (just as add is shorter than offset), and, oh, hey right we have add.

So, just make add_to, IMO. 😸

(I will concede add_to reads funny, because my brain wants to map it to "add x to y" and not "the add to get to y", but I think this is a weird enough name that it will just... be fine in practice and we'll get over it in like a day when all of our code actually makes sense to read again.)

Also CC #95643

[Gankra]

(also yeah byte_unsigned_offset_from is Suffering, byte_add_to is uh, a lot more reasonable lol)

[RalfJung]

_from is totally backwards and makes things feel double or triple negative

FWIW I find the naming very intuitive -- x.offset_from(y) is the offset you need to add to y to reach x, i.e., it is x's "distance when starting from" y. I didn't struggle with operand order when using it. Maybe by brain is wired in weird ways 🤷 .

I do agree that it should explicitly spell out the (x-y) / size in the docs, but that seems like a minor thing.
(EDIT: got the order wrong. That's somewhat telling... but I blame having been confused by previous messages claiming this is more confusing than it actually is. ;)

I agree offset_to would also work. add_to on the other hand sounds very confusing to me, x.add_to(y) should be y+x IMO.

[eddyb]

(I know @Gankra already knows this, but I wanted to state my objection with "add" because it's only a verb, whereas AFAICT offset_from uses "offset" as a noun, not as a verb like the offset method does. So offset_from is more like {quantity}_from than {action}_from)

Personally something like start.distance_to(end), or (start..end).element_len() seems potentially straight-forward enough to be acceptable (with start and end both being pointers - and element_ could be replaced by byte_ for the byte-oriented one).
_{(I do not have the same issue tying offset_from to subtraction, but I've failed to figure out why it seems "natural" to think of "directional distances" in this way, and if it doesn't for other people I don't mind replacing it with anything that's more range-like, which have much "wider" precedent in Rust than raw pointers)}

EDIT: wait I forgot that "addend" is the equivalent noun for "add", so addend_from can be an unsigned version of offset_from, but I don't think that'd be very clear.

[saethlin]

I agree with @Gankra's sort of confusion/irritation at offset_from. I totally understand that there is a logic to what it is, and I could write it down, but trying to write code with it or explain this API to someone feels like doing mental gymnastics. And the sheer number of ways this could be UB makes me really uncomfortable with using it. But I feel like it's supposed to be the way that you do pointer subtraction, so I should use it. Overall it's just upsetting and makes me feel like the standard library (or standard library's API choices?) is getting in my way.

Btw I'm just commenting here because I'm trying to patch all the ptr-int-ptr casts out of bumpalo, and this code feels like it should use {unsigned_}offset_from: https://github.com/fitzgen/bumpalo/blob/9d754c53fc8931b49afd339603c0c09846297c25/src/lib.rs#L276-L287

impl ChunkFooter {
    // Returns the start and length of the currently allocated region of this
    // chunk.
    fn as_raw_parts(&self) -> (*const u8, usize) {
        let data = self.data.as_ptr() as usize;
        let ptr = self.ptr.get().as_ptr() as usize;
        debug_assert!(data <= ptr);
        debug_assert!(ptr <= self as *const _ as usize);
        let len = self as *const _ as usize - ptr;
        (ptr as *const u8, len)
    }
}

A lot of the bumpalo code casts *const u8 to usize to do arithmetic (then converts them back), and I would like to reduce the pool of reasons for doing this.

[saethlin]

Just caught up on the Zulip thread for this.

@scottmcm you commented:

Huh, I should have rg'd sooner -- essentially every use of ptr::offset_of is followed by as usize:

I think add and sub were a huge improvement over offset, because they are both clearer and less characters to type. They're an unambiguous win over code that does offset(usize as isize). Data suggests that people only really want the usize version of ptr - ptr, so I think it would be a shame if we had an API which is not clearer but has a longer name to do the operation people want. (I'm not advocating for add_to in particular, just that sort of direction)

[est31]

Yeah offset absolutely suffers from the verb / noun confusion issue, where the two mean completely different things. To give a data point, when I see v.offset_from(w), I think about the result being something like w + v, not about it being v - w.

distance suffers from the verb/noun issue less, even though usages as a verb exist. As the API is stable since 1.47 it's tough to change it, but of course expansion of the API can provide a good opportunity to revisit the choice and maybe provide a new one with a better name, deprecating the old one, and the added function only using the new name.

Regarding offset_to vs offset_from, I like the fact that the latter is analog to the - operator. I feel that Rust should not make the abstractions even less natural than they have to be due to the provenance issue. I think most people work with a mental model of pointers as numbers, and Rust should respect that.

[Gankra]

I opened #95851 to clarify offset_from's docs

[RalfJung]

Regarding offset_to vs offset_from, I like the fact that the latter is analog to the - operator. I feel that Rust should not make the abstractions even less natural than they have to be due to the provenance issue. I think most people work with a mental model of pointers as numbers, and Rust should respect that.

If we want an operation that works in const, we have to require that both pointers point to the same allocation. Allocations at const time do not have fixed addresses, so computing offsets across allocation boundaries is not meaningful.

This is also slightly helpful for compiler optimizations, for roughly the same reason -- the optimizer can be sure that passing the result of an offset_from to outside code will not leak any information about the allocation base address, which helps analyses. However, arguably, with x.addr() - y.addr(), we now have another way to express this that avoids the "same allocation" pitfall. So if we ignore const, having a fully safe version is possible without optimization impact. (That's assuming that we keep addr with the semantics from #95588, and it is theoretic until the compiler starts treating addr and expose_addr differently.)

[eternaleye]

One thing I'll note is that both end.offset_from(start) and start.offset_to(end) require that you care about the ordering in advance, but many use cases don't care about order - just the (magnitude of) the distance, not the sign.

In such cases, something like x.distance(y) such that start.distance(end) = end.distance(start) is considerably more ergonomic, and the sign can always be recovered afterwards using let negative = x < y.

[tschuett]

You can either recover the sign at runtime or have different return types. The former might be ergonomic and the latter help the compiler.

[scottmcm]

i.e., it is x's "distance when starting from" y.

I like the sub direction, because it's what you'd get doing this with indexes. I guess we could lean on this in the name too, like b.sub_ptr(a) = n <=> a.add(n) = b <=> b.sub(n) = a. (That uses a suffix to describe the argument in the same sort of way as checked_add_unsigned.)

Data suggests that people only really want the usize version of ptr - ptr, so I think it would be a shame if we had an API which is not clearer but has a longer name to do the operation people want.

👍 The same as how .add(d) is shorter than .offset(i) because it's what I always wanted anyway, it makes sense to me to say that this should have a shorter name than offset_from -- and definitely not a much longer one like unsigned_offset_from.

I started making this thinking of it as "this is the specialized one that slice::from_ptr_range and slice::Iter want". But now that I've realized that it's the normal one, the unsigned_offset_from name is completely inappropriate.

also yeah byte_unsigned_offset_from is Suffering

Heh, very much so.

Hmm, offset_from is anticommutative (a.offset_from(b) == -b.offset_from(a)), which probably makes the order fundamentially hard to express well.

Maybe we can take advantage of the "UB in the other order (unless they're equal)" to put the directionality more obviously in the name.

Would it be clearer with b.bytes_after(a) and b.elements_after(a)? Though including elements seems somewhat inconsistent with the add and sub methods that aren't add_bytes and add_elements...

but many use cases don't care about order - just the (magnitude of) the distance, not the sign.

Can you say more about this, @eternaleye? Notably, every single case in the library/ seems to know up-front which is larger than the other, and @saethlin's case above does too. So while an abs_diff-like API certainly seems reasonable, it's not obvious to me that it should be the primary one.

(Although, thinking about this again, a.offset_from(b).unsigned_abs() works fine since the the signed distance isn't allowed to wrap anyway, and thus it's less obvious that a separate method for it is needed, the way abs_diff is.)

[Gankra]

I think @eternaleye was arguing that the order-less API is strictly more useful because it works for both the standard "definitely know" and the strange "no idea (yet)". But I think this operation is low-level enough and the "definitely know" case so common, that we should prefer the ordered impl because it's more optimized (saving like 3 instructions or something, I couldn't think of any way to do cute hardware-specific).

Also we have usize::abs_diff so if anyone really wants it they can do ptr.addr().abs_diff(ptr2.addr()) / size themselves and it will probably be negligible since they're doing something Weird.

[scottmcm]

Thanks, @Gankra; that makes sense. And we definitely need a hyper-optimized version of this operation for use in the bowels of slice iterators, so it might as well be this method. I agree the more unusual cases are best left for something else.

---

I ended up pushing a change to rename to sub_ptr, as the consistency with the add/sub names resonated the strongest with me. This test in particular emphasized it the strongest -- if I look at ptr.add(2).sub_ptr(ptr) it seems perfectly clear to me for that to return 2, just like it would for i + 2 - i.

I also added a section like this to the docs to attempt to further emphasize:

ptr.sub_ptr(origin) == count
origin.add(count) == ptr
ptr.sub(count) == origin

Feedback appreciated on whether this name is good, or whether you're prefer one of the other naming approaches.

[Gankra]

I still think framing it as subtraction is a missed opportunity because subtraction is not what you actually "care" about, but a means to an end (which always stops me in my tracks while I triple check which way I want).

But at least it is very compact and appeals directly to subtraction, so it's a strict ergonomics win with the same intuition as the "normal" way. I would happily let this land to nightly and let the name bikeshed later. 😸

[Gankra]

(disclaimer: I have not reviewed the rest of this in detail, and idk how much the docs want to crib from my other PR)

[bors]

💔 Test failed - checks-actions

[scottmcm]

Rebase from faee850 to e1520f1 is just updating to master.
Rebase from e1520f1 to 003b954 is fixing the codegen test.

There's an alloca in the bors run that doesn't show up locally for me, but it looks like -O1 will fix that: https://rust.godbolt.org/z/rjoxYWErc

@bors r=oli-obk rollup=iffy (codegen tests are always iffy for me)

[bors]

📌 Commit 003b954 has been approved by oli-obk

[bors]

⌛ Testing commit 003b954 with merge 1d2ea98...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1d2ea98 to master...

[rust-timer]

Finished benchmarking commit (1d2ea98): comparison url.

Summary:

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 🎉 relevant improvement found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	0	0	1	0
mean2	N/A	N/A	N/A	-0.2%	N/A
max	N/A	N/A	N/A	-0.2%	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩