CTFE/Miri engine Pointer type overhaul #87123

RalfJung · 2021-07-14T08:58:52Z

This fixes the long-standing problem that we are using Scalar as a type to represent pointers that might be integer values (since they point to a ZST). The main problem is that with int-to-ptr casts, there are multiple ways to represent the same pointer as a Scalar and it is unclear if "normalization" (i.e., the cast) already happened or not. This leads to ugly methods like force_mplace_ptr and force_op_ptr.
Another problem this solves is that in Miri, it would make a lot more sense to have the Pointer::offset field represent the full absolute address (instead of being relative to the AllocId). This means we can do ptr-to-int casts without access to any machine state, and it means that the overflow checks on pointer arithmetic are (finally!) accurate.

To solve this, the Pointer type is made entirely parametric over the provenance, so that we can use Pointer<AllocId> inside Scalar but use Pointer<Option<AllocId>> when accessing memory (where None represents the case that we could not figure out an AllocId; in that case the offset is an absolute address). Moreover, the Provenance trait determines if a pointer with a given provenance can be cast to an integer by simply dropping the provenance.

I hope this can be read commit-by-commit, but the first commit does the bulk of the work. It introduces some FIXMEs that are resolved later.
Fixes rust-lang/miri#841
Miri PR: rust-lang/miri#1851
r? @oli-obk

rust-highfive · 2021-07-14T08:58:55Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

…sion infallible This resolves all the problems we had around "normalizing" the representation of a Scalar in case it carries a Pointer value: we can just use Pointer if we want to have a value taht we are sure is already normalized.

…ecated Scalar methods

compiler/rustc_middle/src/ty/print/pretty.rs

oli-obk · 2021-07-15T11:28:31Z

compiler/rustc_mir/src/const_eval/eval_queries.rs

+                // We know `offset` is relative to the allocation, so we can use `into_parts`.
+                let (data, start) = match ecx.scalar_to_ptr(a.check_init().unwrap()).into_parts() {


could into_parts assert this flag internally? Or does miri also have a need for it?

Yeah Miri needs to access the offset to compare between Pointer<AllocId> (address is relative) and Pointer<Tag> (address is absolute).

FWIW, I actually did think of a way to make this more type-safe: the Provenance trait could have an associated type Offset, and a function wrap_offset(Size) -> Offset. Then into_parts would return Offset, so code generic in Tag would actually not know the type Offset and this could not use it wrong.

Then we might go even further and make the Pointer::offset field be of type Offset... though at that point we cannot define generic pointer arithmetic methods any more so we'd need some more methods in the Provenance trait.

But I am not sure if that's worth it. I had one bug caused by mis-interpreting a relative offset as absolute, and this would not have caught it (since the bug was about pointers stored inside an Allocation, where the offset is part of the byte array anyway).

oli-obk · 2021-07-15T11:41:21Z

compiler/rustc_middle/src/mir/interpret/pointer.rs

+    // so ptr-to-int casts are not possible (since we do not know the global physical offset).
+    const OFFSET_IS_ADDR: bool = false;
+
+    fn fmt(ptr: &Pointer<Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result {


Why is there a custom formatting function? Couldn't this be an impl on Pointer<AllocId> directly, allowing all aggregates containing Pointer to derive their Debug impls?

I don't see how. The Tag type affects how the Pointer itself is rendered:

Pointer<AllocId>: alloc123+0x13

Pointer<Tag>: 0x12defg60[alloc123]<Stacked Borrows info>

I'm mainly unhappy about the roundabout formatting stuff that requires one to essentially write the derived impl by hand.

Yeah I am unhappy about that as well.^^ Open to suggestions for how to do this better.

can miri implement Display for Pointer since Tag is a miri-internal type?

Doesn't that only work for "fundamental" types?

Oh also that would not help -- the generic code in rustc_mir::interpret needs to know for sure that it can debug-print pointers. So we need something generic.

Is it possible to add Pointer<Self::PointerTag>: Debug as a requirement to the Machine trait?^^

Is it possible to add Pointer<Self::PointerTag>: Debug as a requirement to the Machine trait?^^

Not in a useful way (you'd have to repeat the bound at every usage site of Machine).

but you could keep your fmt function and only implement Debug manually on Pointer<Tag: Provenance> and then you should be able to derive Debug everywhere else, as long as you add the Tag: Provenance bound to all the types instead of only on the impls. It's not nice, but nicer than doing manual trivial impls everywhere I think?

thinking about this again, it's not too bad, so we can merge and figure out improvements later

compiler/rustc_middle/src/mir/interpret/mod.rs

oli-obk · 2021-07-15T11:49:58Z

The design lgtm in general, I'm mainly unhappy about the roundabout formatting stuff that requires one to essentially write the derived impl by hand.

… different meanings there)

RalfJung · 2021-07-15T21:49:47Z

I pushed some more commits, mostly because changes were needed to get Miri to work again.

One change is rather unfortunate: I had to give the init_allocation_extra machine hook access to &Memory (and not just &MemoryExtra, and this necessitated a less optimal implementation of Memory::get_mut. With a better borrow checker we could recover the original cost for the happy case (where the allocation is in the local memory), but there'd still be extra cost for when we have to reach for the global memory. I also hat to make MonoHashMap::get in Miri less efficient to avoid RefCell panics.
The reason Miri needs &Memory is that it needs to query the size and alignment of allocations. So one possible alternative would be to make dead_alloc_map actually contain all allocations, not just the dead ones; and to give the init_allocation_extra machine hook access to that map as well. This would be a rather gross hack since Rust does not have good support for representing "a struct with some of its fields borrowed away".

An extremely coarse benchmark (time ./miri test pass) showed no regressions in Miri performance; let's make sure there is no rustc regression either:
@bors try
@rust-timer queue

compiler/rustc_middle/src/mir/interpret/allocation.rs

oli-obk · 2021-07-16T17:17:48Z

r=me if you want, unless you are already working on the things we discussed and want to get them into this PR

RalfJung · 2021-07-16T17:24:54Z

Yeah I am already experimenting now.^^

RalfJung · 2021-07-16T17:54:14Z

This leads to an unnecessary Tag: Debug bound (because we can't control the where clause if the generated Debug, which is the entire problem of course), but I still think it's a win overall.

I did not yet do this for Scalar and ScalarMaybeUninit though, just to the higher-level types. Those two have custom Debug impls anyway.

RalfJung · 2021-07-16T17:55:40Z

Ah, with that last commit this PR actually has a net negative line count. :)

I wish the derive macro would support adding extra where clauses...

oli-obk · 2021-07-17T11:04:08Z

@bors r+

bors · 2021-07-17T11:04:09Z

📌 Commit efbee50 has been approved by oli-obk

bors · 2021-07-17T15:26:33Z

⌛ Testing commit efbee50 with merge c78ebb7...

bors · 2021-07-17T17:56:40Z

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

rust-highfive · 2021-07-17T17:57:08Z

📣 Toolstate changed by #87123!

Tested on commit c78ebb7.
Direct link to PR: #87123

💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

@eddyb

Tested on commit rust-lang/rust@c78ebb7. Direct link to PR: <rust-lang/rust#87123> 💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk). 💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

adjust Miri to Pointer type overhaul This is the Miri side of rust-lang/rust#87123. This was a lot more work than I expected... lucky enough it is also (hopefully) the last large-scale refactoring I will do.^^ Fixes #224

rust-highfive assigned oli-obk Jul 14, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2021

This comment has been minimized.

Sign in to view

RalfJung force-pushed the miri-provenance-overhaul branch from c7436a8 to b7822c4 Compare July 14, 2021 09:25

This comment has been minimized.

Sign in to view

RalfJung force-pushed the miri-provenance-overhaul branch from b7822c4 to cb61d35 Compare July 14, 2021 11:05

RalfJung added 9 commits July 14, 2021 18:17

consistently treat None-tagged pointers as ints; get rid of some depr…

626605c

…ecated Scalar methods

use NonZeroU64 for AllocId to restore old type sizes

71c166a

fix cranelift

172abb2

adjust tests

4759090

test dynamic check for ptr-int-casts (unleashed)

4ff353c

more precise message for the ptr access check on deref

ae950a2

bless mir-opt, codegen, and remaining ui tests

1f21ab1

remove unused error variant

8932aeb

RalfJung force-pushed the miri-provenance-overhaul branch from cb61d35 to 8932aeb Compare July 14, 2021 16:18

oli-obk reviewed Jul 15, 2021

View reviewed changes

compiler/rustc_middle/src/ty/print/pretty.rs Show resolved Hide resolved

oli-obk reviewed Jul 15, 2021

View reviewed changes

compiler/rustc_middle/src/mir/interpret/mod.rs Show resolved Hide resolved

RalfJung added 3 commits July 15, 2021 17:14

adjustions and cleanup to make Miri build again

f4b61ba

enable Miri to fix the bytes in an allocation (since ptr offsets have…

adbe755

… different meanings there)

make check_ptr_access_align work on function pointer allocations

bd874a9

This comment has been minimized.

Sign in to view

tweak pointer out-of-bounds error message

4e28065

RalfJung force-pushed the miri-provenance-overhaul branch from 310f41f to 4e28065 Compare July 15, 2021 20:47

RalfJung mentioned this pull request Jul 15, 2021

adjust Miri to Pointer type overhaul rust-lang/miri#1851

Merged

This comment has been minimized.

Sign in to view

get rid of incorrect erase_for_fmt

7c720ce

RalfJung force-pushed the miri-provenance-overhaul branch from 6b1ef0d to 7c720ce Compare July 16, 2021 08:10

RalfJung mentioned this pull request Jul 16, 2021

const-eval: loading or overwriting parts of a pointer is not supported #87184

Closed

bjorn3 reviewed Jul 16, 2021

View reviewed changes

compiler/rustc_middle/src/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

add some comments regarding the two major quirks of our memory model

a5299fb

RalfJung force-pushed the miri-provenance-overhaul branch from a091a22 to a5299fb Compare July 16, 2021 11:16

RalfJung force-pushed the miri-provenance-overhaul branch from 8c7bb33 to c3ed171 Compare July 16, 2021 17:52

avoid manual Debug impls by adding extra Provenance bounds to types

efbee50

I wish the derive macro would support adding extra where clauses...

RalfJung force-pushed the miri-provenance-overhaul branch from c3ed171 to efbee50 Compare July 16, 2021 18:02

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 17, 2021

bors merged commit c78ebb7 into rust-lang:master Jul 17, 2021

rustbot added this to the 1.55.0 milestone Jul 17, 2021

rust-highfive mentioned this pull request Jul 17, 2021

miri no longer builds after rust-lang/rust#87123 #87222

Closed

RalfJung deleted the miri-provenance-overhaul branch July 17, 2021 18:06

bors mentioned this pull request Jul 17, 2021

don't evaluate constants if borrowck fails #86325

Closed

RalfJung mentioned this pull request Mar 2, 2022

Always use -Z symbolic-alignment-checks for *unleaked* allocations rust-lang/miri#1988

Open

		// We know `offset` is relative to the allocation, so we can use `into_parts`.
		let (data, start) = match ecx.scalar_to_ptr(a.check_init().unwrap()).into_parts() {

CTFE/Miri engine Pointer type overhaul #87123

CTFE/Miri engine Pointer type overhaul #87123

Uh oh!

Conversation

RalfJung commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jul 14, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk commented Jul 15, 2021

Uh oh!

This comment has been minimized.

RalfJung commented Jul 15, 2021

Uh oh!

This comment has been minimized.

Uh oh!

oli-obk commented Jul 16, 2021

Uh oh!

RalfJung commented Jul 16, 2021

Uh oh!

RalfJung commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Jul 16, 2021

Uh oh!

oli-obk commented Jul 17, 2021

Uh oh!

bors commented Jul 17, 2021

Uh oh!

bors commented Jul 17, 2021

Uh oh!

bors commented Jul 17, 2021

Uh oh!

rust-highfive commented Jul 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

RalfJung commented Jul 14, 2021 •

edited

Loading

RalfJung commented Jul 16, 2021 •

edited

Loading