Make pointer offset methods/intrinsics const

[josephlr]

Implements #71499 using the implementations from miri.

I added some tests what's allowed and what's UB. Let me know if any other cases should be added.

CC: @RalfJung @oli-obk

[rust-highfive]

r? @matthewjasper

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

[oli-obk]

Please also move https://github.com/rust-lang/miri/blob/147ea8f400de3ca529abcb5eb7b65f84a4896ae9/src/operator.rs#L98 to the miri engine, so that you don't have to repeat its logic. Miri can then call the engine method in the future.

[josephlr]

@oli-obk after this PR lands I was just going to remove all of the Miri pointer_offset_inbounds code. Will that not be possible?

EDIT: I think I understand why pointer_offset_inbounds has to stay. It looks like (non arithmetric) pointer offsets get generated in two different places. One by calls to intrinsic::offset and one by calls to drop arrays in librustc_mir/util/elaborate_drops.rs. So we need a common implementation so miri can handle the second case.

I'll move the function over.

[RalfJung]

I made the implementation a single function for both offset and arith_offset.

I think this greatly decreased readability. Checked and unchecked arithmetic are very different operations, I am not sure if it is worth sharing the bit of code they have in common.

But we'll see again after you moved pointer_offset_inbounds to librustc_mir/interpret/operator.rs.

[JohnCSimon]

Ping from triage
@josephlr - can you please post your status on this PR? Thanks!

[josephlr]

Ping from triage
@josephlr - can you please post your status on this PR? Thanks!

Status: Tried getting #[feature(const_compare_raw_pointers)] to work with this, but that was a dead-end. Working on cleaning up the tests and moving pointer_offset_inbounds.

[josephlr]

@RalfJung @oli-obk this is ready again for review, sorry for the delay.

Please also move https://github.com/rust-lang/miri/blob/147ea8f400de3ca529abcb5eb7b65f84a4896ae9/src/operator.rs#L98 to the miri engine, so that you don't have to repeat its logic. Miri can then call the engine method in the future.

I moved miri's EvalContextExt::pointer_offset_inbounds method to rustc's InterpCx::ptr_offset_inbounds. The name is slightly different so that miri won't break when this PR hits nightly.

BTW: rust-lang/miri#1412 contains the miri cleanup

I made the implementation a single function for both offset and arith_offset.

I think this greatly decreased readability. Checked and unchecked arithmetic are very different operations, I am not sure if it is worth sharing the bit of code they have in common.

Looking at this again, you're correct, keeping them separate adds ~3 lines and makes the code much more readable.

[RalfJung]

Aside from the nit I raised, Miri implementation and test suite LGTM. For the glue and libcore changes I'll defer to @oli-obk.

[josephlr]

@RalfJung Fixed the nits. I had to add //~NOTE annotations instead of ERROR annotations, as the const-eval errors are errors with kind=Some(Note) and not errors with kind=Some(Error).

EDIT: Note that this is what src/test/ui/consts/offset_from_ub.rs does as well.

[RalfJung]

2020-05-15T20:55:09.2903132Z tidy error: /checkout/src/test/ui/consts/offset_ub.rs:12: line longer than 100 chars
2020-05-15T20:55:09.2903954Z tidy error: /checkout/src/test/ui/consts/offset_ub.rs:16: line longer than 100 chars

In tests I usually add the ignore-line-length thing for tidy rather than breaking the lines...^^

[RalfJung]

UB detection shouldn't depend on the target pointer size... so ideally we'd make this consistent.

The pointer offset methods in librustc_middle/mir/interpret/pointer.rs are all written to realize this.

[RalfJung]

In fact the two error messages are almost the same. So probably we should just adjust one of the error sites to use the same message as the other.

[RalfJung]

@josephlr you can run the 32bit test suite yourself by passing --target i686-unknown-linux-gnu (at least on Linux this works well if you have gcc-multilib installed).

[josephlr]

@josephlr you can run the 32bit test suite yourself by passing --target i686-unknown-linux-gnu (at least on Linux this works well if you have gcc-multilib installed).

Ya I was able get the tests working (and I can see the error message issues). I think the right approach is to be more strict in the order of the UB checks.

Right now we:

1. Compute offset_bytes, checking for overflow of i64
2. Compute the offset_pointer via ptr_signed_offset, which checks that the final offset fits in a target-sized pointer.

On 32-bit, if offset_bytes overflows an i32 but not an i64, we catch the UB in step (2) instead of step (1). I think we need to make the check in (1) more strict, making sure that offset_bytes doesn't overflow a target isize. Then everything should just work.

[RalfJung]

On 32-bit, if offset_bytes overflows an i32 but not an i64, we catch the UB in step (2) instead of step (1). I think we need to make the check in (1) more strict, making sure that offset_bytes doesn't overflow a target isize. Then everything should just work.

We could do that, but it still seems odd to have almost but not quite identical error messages in both cases. IMO it would make more sense to just say "overflowing in-bounds pointer arithmetic" either way.

[josephlr]

On 32-bit, if offset_bytes overflows an i32 but not an i64, we catch the UB in step (2) instead of step (1). I think we need to make the check in (1) more strict, making sure that offset_bytes doesn't overflow a target isize. Then everything should just work.

We could do that, but it still seems odd to have almost but not quite identical error messages in both cases. IMO it would make more sense to just say "overflowing in-bounds pointer arithmetic" either way.

Agreed, while I implemented the checks as described above, the error is always PointerArithOverflow. Everything looks good now, and the tests seem to pass on 64 and 32 bit.

[RalfJung]

Awesome!
@bors r=oli-obk,RalfJung

[bors]

📌 Commit 7d5415b has been approved by oli-obk,RalfJung