miri engine: basic support for pointer provenance tracking #54461

RalfJung · 2018-09-22T10:39:03Z

This enriches pointers with a new member, tag, that can be used to do provenance tracking. This is a new type parameter that propagates up through everything. It defaults to () (no tag), which is also the value used by CTFE -- but miri will use another type.

The only actually interesting piece here, I think, is what I had to do in the memory's get. The problem is that tcx (storing the allocations for statics) uses () for provenance information. But the machine might need another tag. The machine has a function to do the conversion, but if a conversion actually happened, we need to store the result of this somewhere -- we cannot return a pointer into tcx as we usually would.
So I introduced MonoHashMap which uses RefCell to be able to insert new entries even when we just have a shared ref. However, it is important that we can also return shared refs into the map without holding the RefCell opan. This is achieved by boxing the values stored in the map, so their addresses remain stable even when the map's table gets reallocated. This is all implemented in mono_hash_map.rs.

NOTE: This PR also contains the commits from #54380 (comment). Only the last two commits are new.

Mark-Simulacrum · 2018-09-22T12:15:30Z

The mono hash map should almost certainly go in librustc_data_structures

RalfJung · 2018-09-22T12:30:08Z

It is awfully specific in its API -- quirky and limited to what memory needs. Basically, it is in a separate module only because that reduces the amount of code that has to care about the invariant.

eddyb · 2018-09-22T14:10:31Z

So I introduced MonoHashMap

As a general note, I'd like to not merge such changes until multiple people have looked at the problem, and decided that the new data structure is needed.

EDIT: I looked at it - we planned to do this ages ago, but we actually settled on using arenas instead - could you use a TypedArena here, and just have a HashMap with references from the arena as values?

RalfJung · 2018-09-22T15:58:55Z

I considered using an arena, but then when stack frames get popped or boxes dropped, their allocations would not get removed.

I could try recycling allocations from the arena, but at that point I'd also write a new data structure.

eddyb · 2018-09-22T16:02:40Z

Okay, why not use Rc instead of Box then?

RalfJung · 2018-09-22T16:10:09Z

You mean, we should use Rc<Allocation> instead of &Allocation as the return value of Memory::get? Not sure if that would overall be more or less efficient than what I got. I'd guess slightly less so because Rc has to check the refcount even when we have an &mut, which I currently avoid.

eddyb · 2018-09-23T11:44:39Z

@RalfJung Ah, I was thinking in terms of tags there. We should probably discuss your plans there in more detail - what do you expect to use as the non-() type for Tag?
I would hope for something copyable, which would, I believe, remove the need for MonoHashMap?

Oops, I read your PR description again, so this is for allocating &Allocation.

Why not use a slightly different map than Relocation for tags, or just make them optional? Then, instead of (), you use ! (and I guess Pointer<!> would just contain Option<!>).

RalfJung · 2018-09-23T20:58:50Z

Why not use a slightly different map than Relocation for tags, or just make them optional? Then, instead of (), you use ! (and I guess Pointer<!> would just contain Option<!>).

I don't understand how that would help at all? It'd still be Option<!> in tcx and Option<Borrow> in the full miri machine.

I know you stroked that part of your comment, but my plans are described at https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html :D

eddyb · 2018-09-26T06:40:19Z

@RalfJung Sorry, I keep getting confused. Something workable could be keeping a map from AllocId to these tags, outside of the Allocation. That way, you don't have to do anything special when "importing" tcx allocations - they're just guaranteed to have trivial tags.

RalfJung · 2018-09-26T10:08:08Z

That's not enough information. I need to track every pointer, so I'd need another relocation-like map somewhere. I think that would get really messy, this is nicer.

eddyb · 2018-09-28T08:45:18Z

Adding new unsafe code is not nice, IMO. However, if you want to use unsafe code...
For now, if you have a &HashMap<Foo, Box<!>>, you should be able to refer to it as &HashMap<Foo, Box<T>> for any T, as it's guaranteed to be empty anyway.

RalfJung · 2018-09-29T13:25:15Z

Not sure where you think a HashMap<_, Box<!>> would come up here?

We could have a Relocations<Tag> in the allocation, so having two relocations instead of one with pairs, and then hope that the path with () gets optimized away. Not sure how that would help, though.

nikomatsakis · 2018-10-01T18:50:46Z

Having talked out the various options with @RalfJung, I have to say that MonoHashMap seems like a pretty reasonable solution to the existing constraints. Certainly the interface is pretty small and self-contained, and it permits retaining the convenient &-based API.

RalfJung · 2018-10-04T15:21:58Z

@eddyb @oli-obk @nikomatsakis This is currently blocking progress on my main internship project. How do we proceed?

bjorn3

Noticed two typos

bjorn3 · 2018-10-04T15:46:50Z

src/librustc_mir/const_eval.rs

+    fn static_with_default_tag(
+        alloc: &'_ Allocation
+    ) -> Cow<'_, Allocation<Self::PointerTag>> {
+        // We do not use a tag so we can just cheapyl forward the reference


bjorn3 · 2018-10-04T15:48:32Z

src/librustc_mir/interpret/machine.rs

+    /// the appropriate tags on each pointer.
+    ///
+    /// This should avoid copying if no work has to be done! If this returns an owned
+    /// allocation (because a copy had to be done to add the tags), machibe memory will


nikomatsakis · 2018-10-04T16:20:55Z

@RalfJung my vote is to land as is; we can always revisit it

nikomatsakis · 2018-10-04T16:26:10Z

I do agree with @eddyb that it's a good idea to be very cautious with introducing new unsafe abstractions. However, this one feels quite self-contained, and I don't see any other solution to the problem that doesn't involve large scale refactorings (I think there are various approaches one could take if we did want to do large-scale refactoring, but we could also do that as a follow-up, and it's not clear that such a thing would be good).

Alternative solutions I can see:

Use a long-lived arena (e.g., with a lifetime like 'tcx). I believe @RalfJung was concerned about memory for these tags not being freed "promptly", so a long-lived arena may not be a very good idea.
Use indices: big change from using &Allocation.
Use a ref-counted value and return a Cow or some such thing that means the allocation may either be ref-counted or borrowed. Again, big refactoring, probably annoying and messy.
Use a ref-counted value and a shorter arena (e.g., scoped to a particular set of memory operations). This shorter arena can be used just to hold onto a "second ref" to the Rc<Allocation> such that we can always return a &Allocation (sort of like a Cocoa auto-release pool). Again, big refactoring. This feels potentially clean to me but (a) probably nobody but me understands what I mean and (b) it'd require invasive edits and is probably best done as a follow-up anywya.

Other than that, I don't think there are other options.

RalfJung · 2018-10-04T16:54:15Z

Rebased and fixed typos.

src/librustc_mir/interpret/memory.rs

src/librustc/mir/interpret/value.rs

oli-obk · 2018-10-04T15:29:30Z

src/librustc_mir/const_eval.rs

+    fn static_with_default_tag(
+        alloc: &'_ Allocation
+    ) -> Cow<'_, Allocation<Self::PointerTag>> {
+        // We do not use a tag so we can just cheapyl forward the reference


src/librustc_mir/interpret/memory.rs

src/librustc_mir/interpret/snapshot.rs

src/librustc_mir/interpret/memory.rs

RalfJung · 2018-10-05T13:52:25Z

Thanks to @oli-obk I was able to get rid of the unsafe code here. :) The actual map used by memory to manage its allocations is now abstracted away through the Machine trait. The miri tool will still need unsafe, but rustc itself and the CTFE engine do not.

…fancy one

…in the miri engine

RalfJung · 2018-10-10T09:33:23Z

Rebased, and I hope tidy is also happy now.

oli-obk · 2018-10-10T09:51:25Z

src/librustc_mir/interpret/memory.rs

+                write!(msg, "└{0:─^1$}┘ ", target, relocation_width as usize).unwrap();
+                pos = i + self.pointer_size();
+            }
+            trace!("{}", msg);


you'll probably want to do tag-printing here at some point, right?

Yeah I think so, but I am not sure what the best way is to do that (and to avoid printing useless ()).

oli-obk · 2018-10-10T09:54:11Z

src/librustc_mir/interpret/place.rs

        // Downcasts only change the layout
-        assert_eq!(base.extra, None);
+        assert!(base.meta.is_none());


the assert_eq was on purpose, to make sure that base.extra was debug printed if the assertion failed

I wanted to avoid an Eq bound on tags. But I now added that to be able to put them into HashMap so I guess I could change these all back.

oli-obk · 2018-10-10T09:55:46Z

@bors r+

bors · 2018-10-10T09:55:47Z

📌 Commit bc9435d has been approved by oli-obk

bors · 2018-10-10T12:13:13Z

⌛ Testing commit bc9435d with merge 2243fab...

miri engine: basic support for pointer provenance tracking This enriches pointers with a new member, `tag`, that can be used to do provenance tracking. This is a new type parameter that propagates up through everything. It defaults to `()` (no tag), which is also the value used by CTFE -- but miri will use another type. The only actually interesting piece here, I think, is what I had to do in the memory's `get`. The problem is that `tcx` (storing the allocations for statics) uses `()` for provenance information. But the machine might need another tag. The machine has a function to do the conversion, but if a conversion actually happened, we need to store the result of this *somewhere* -- we cannot return a pointer into `tcx` as we usually would. So I introduced `MonoHashMap` which uses `RefCell` to be able to insert new entries even when we just have a shared ref. However, it is important that we can also return shared refs into the map without holding the `RefCell` opan. This is achieved by boxing the values stored in the map, so their addresses remain stable even when the map's table gets reallocated. This is all implemented in `mono_hash_map.rs`. NOTE: This PR also contains the commits from #54380 (comment). Only the [last two commits](https://github.com/rust-lang/rust/pull/54461/files/8e74ee0998a5b11f28d61600dbb881c7168a4a40..HEAD) are new.

bors · 2018-10-10T14:50:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 2243fab to master...

rust-highfive · 2018-10-10T14:52:26Z

📣 Toolstate changed by #54461!

Tested on commit 2243fab.
Direct link to PR: #54461

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

@oli-obk

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

ljedrz · 2018-10-12T10:36:18Z

@RalfJung FYI, this change has worsened the results of max-rss ctfe-family benchmarks by up to 40% (perf link). Is this expected?

RalfJung · 2018-10-12T11:08:20Z

Uh, no, that's odd. It should just have added a bunch of ZST in a couple places.

How does one go about debugging such things?

RalfJung · 2018-10-12T11:11:32Z

Oh wait. I got rid of interning vtables... that could be it. I left a FIXME to add a cache. I guess I should put implementing that cache further up on my list.^^

I am not sure if vtables really should be put into tcx -- my gut feeling is that anything that might happen during an execution of the miri tool, shouldn't affect tcx. The fact that allocations in tcx now have a different type (no provenance) than the ones in miri makes sure we don't accidentally do that. But caching vtables within a miri execution makes a lot of sense, I think.

This comment has been minimized.

Sign in to view

TimNN added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 25, 2018

TimNN assigned eddyb Sep 25, 2018

RalfJung mentioned this pull request Sep 29, 2018

Miri engine cleanup #53671

Merged

This comment has been minimized.

Sign in to view

RalfJung force-pushed the pointer-provenance branch from c82f505 to 5af85da Compare October 2, 2018 08:39

bjorn3 reviewed Oct 4, 2018

View reviewed changes

RalfJung force-pushed the pointer-provenance branch from 79ab37b to 1d65fe6 Compare October 4, 2018 16:54

nikomatsakis reviewed Oct 4, 2018

View reviewed changes

src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved

oli-obk reviewed Oct 5, 2018

View reviewed changes

RalfJung force-pushed the pointer-provenance branch from 2eb29e5 to 1cf357d Compare October 5, 2018 13:51

RalfJung mentioned this pull request Oct 5, 2018

update for ptr provenance rust-lang/miri#468

Merged

RalfJung added 5 commits October 10, 2018 09:58

clarify alloc_map description

0354510

typos and spaces

75ea7c7

abstract mono_hash_map through a trait, only miri actually needs the …

83667d6

…fancy one

more comments for these sublte games we are playing with allocations …

3fb617d

…in the miri engine

vtables are not leaks

9a9dbff

RalfJung force-pushed the pointer-provenance branch from 0293a45 to 9a9dbff Compare October 10, 2018 08:31

This comment has been minimized.

Sign in to view

tidy up

bc9435d

oli-obk reviewed Oct 10, 2018

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2018

bors merged commit bc9435d into rust-lang:master Oct 10, 2018

This was referenced Oct 10, 2018

Report const eval error inside the query #53821

Merged

Rollup of 12 pull requests #54953

Closed

impl Eq+Hash for TyLayout #54936

Merged

miri engine: Fix run-time validation #54955

Merged

oli-obk mentioned this pull request Oct 13, 2018

Deduplicate some code and compile-time values around vtables #55016

Merged

RalfJung deleted the pointer-provenance branch November 9, 2018 15:35

miri engine: basic support for pointer provenance tracking #54461

miri engine: basic support for pointer provenance tracking #54461

Conversation

RalfJung commented Sep 22, 2018 • edited Loading

This comment has been minimized.

Mark-Simulacrum commented Sep 22, 2018

RalfJung commented Sep 22, 2018

eddyb commented Sep 22, 2018 • edited Loading

RalfJung commented Sep 22, 2018 • edited Loading

eddyb commented Sep 22, 2018

RalfJung commented Sep 22, 2018

eddyb commented Sep 23, 2018

RalfJung commented Sep 23, 2018 • edited Loading

eddyb commented Sep 26, 2018

RalfJung commented Sep 26, 2018 via email

eddyb commented Sep 28, 2018

RalfJung commented Sep 29, 2018

nikomatsakis commented Oct 1, 2018

This comment has been minimized.

RalfJung commented Oct 4, 2018

bjorn3 left a comment

Choose a reason for hiding this comment

bjorn3 Oct 4, 2018

Choose a reason for hiding this comment

bjorn3 Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Oct 4, 2018

nikomatsakis commented Oct 4, 2018

RalfJung commented Oct 4, 2018

oli-obk Oct 4, 2018

Choose a reason for hiding this comment

RalfJung commented Oct 5, 2018

This comment has been minimized.

RalfJung commented Oct 10, 2018

oli-obk Oct 10, 2018

Choose a reason for hiding this comment

RalfJung Oct 10, 2018

Choose a reason for hiding this comment

oli-obk Oct 10, 2018

Choose a reason for hiding this comment

RalfJung Oct 10, 2018

Choose a reason for hiding this comment

oli-obk commented Oct 10, 2018

bors commented Oct 10, 2018

bors commented Oct 10, 2018

bors commented Oct 10, 2018

rust-highfive commented Oct 10, 2018

ljedrz commented Oct 12, 2018 • edited Loading

RalfJung commented Oct 12, 2018

RalfJung commented Oct 12, 2018

RalfJung commented Sep 22, 2018 •

edited

Loading

eddyb commented Sep 22, 2018 •

edited

Loading

RalfJung commented Sep 22, 2018 •

edited

Loading

RalfJung commented Sep 23, 2018 •

edited

Loading

bjorn3 Oct 4, 2018 •

edited

Loading

ljedrz commented Oct 12, 2018 •

edited

Loading