Give OpTy access to locals for priroda #55179

bjorn3 · 2018-10-18T12:17:44Z

r? @oli-obk

oli-obk · 2018-10-18T12:18:28Z

@bors r+ rollup

bors · 2018-10-18T12:18:30Z

📌 Commit 7403d55 has been approved by oli-obk

@oli-obk

Make OpTy field op public for priroda r? @oli-obk

RalfJung · 2018-10-19T06:35:59Z

Oh, why does it need that? I wanted to have something like an abstraction here :(

oli-obk · 2018-10-19T07:03:38Z

We need a way to go from a stackframe ID and a local ID to an OpTy. This seemed the easiest way. Since you have legit concerns here, let's do it right by adding a convenience function for the specific use case.

@bors r-

kennytm · 2018-10-19T08:50:00Z

@bors rollup-

oli-obk · 2018-10-19T12:17:32Z

r? @RalfJung

bjorn3 · 2018-10-19T12:19:19Z

Got some errors locally. Fixing...

bjorn3 · 2018-10-19T12:23:36Z

Fixed

RalfJung · 2018-10-19T17:38:08Z

src/librustc_mir/interpret/operand.rs

+        let local_ty = self.monomorphize(local_ty, frame.instance.substs);
+        let layout = self.layout_of(local_ty)?;
+        Ok(OpTy { op, layout })
+    }


Why don't you use stack()? locals is public, so that should give you all you need?

That gives an Operand while I need an OpTy which cant be made outside rustc_mir.

Oh I see. In that case, please use layout_of_local, and please change eval_place_to_operand to use your new method.

layout_of_local expects a frame id, while this takes an arbitrary Frame. I can do the other thing though.

And you don't have a frame ID where you need this?

You can change layout_of_local to take &Frame as well.

And you don't have a frame ID where you need this?

Currently I do have it, but in the future I want to do some caching for stepping back in time, which could involve printing frames not on the stack. (I am not sure how to do the caching yet, just anticipating.)

You can change layout_of_local to take &Frame as well.

+1

bjorn3 · 2018-10-19T18:53:09Z

@oli-obk suggested adding another thing to this PR, which I will do tomorrow.

bjorn3 · 2018-10-20T09:11:36Z

This is ready for review.

src/librustc_mir/interpret/memory.rs

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

src/librustc_mir/interpret/operand.rs

+        &self,
+        frame: &super::Frame<'mir, 'tcx, M::PointerTag>,
+        local: mir::Local,
+        layout: Option<TyLayout<'tcx>>,


Uh, I forgot we already know the layout there sometimes. Exposing this is not nice.

I do not have a good idea for how to avoid it, though.

Layout is also passed to eval_place_to_op which calls this function now.

src/librustc_mir/interpret/operand.rs

RalfJung · 2018-10-21T07:43:10Z

@bors r-

Sorry I forgot something: Could you add an assert! to make sure nobody accesses the return place? Also, a doc comment explaining that Option<TyLayout> would be nice.

RalfJung · 2018-10-21T07:43:23Z

@bors delegate+

r=me with that fixed.

bors · 2018-10-21T07:43:24Z

✌️ @bjorn3 can now approve this pull request

bors · 2018-10-21T12:11:55Z

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

Also make `layout_of_local` accept any `Frame`

bjorn3 · 2018-10-21T14:17:25Z

@bors r+

bors · 2018-10-21T14:17:26Z

📌 Commit b178553 has been approved by bjorn3

RalfJung · 2018-10-21T15:08:00Z

This should have been

@bors r=RalfJung

bors · 2018-10-21T15:08:01Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Replace CanonicalVar with DebruijnIndex #52984

bors · 2018-10-21T15:08:02Z

📌 Commit b178553 has been approved by RalfJung

bjorn3 · 2018-10-21T16:34:30Z

Didn't know I had to do it that way.

RalfJung · 2018-10-21T16:39:54Z

It's okay, now you do. :)

"r+" means "I did the review and approve". "r=someone" means "I solemnly swear that someone did the review and approved".

bors · 2018-10-21T20:07:41Z

⌛ Testing commit b178553 with merge 424a749...

@oli-obk

Give OpTy access to locals for priroda r? @oli-obk

bors · 2018-10-21T22:41:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: RalfJung
Pushing 424a749 to master...

rust-highfive assigned oli-obk Oct 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2018

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 Oct 18, 2018

kennytm mentioned this pull request Oct 18, 2018

Rollup of 9 pull requests #55181

Closed

kennytm added a commit to kennytm/rust that referenced this pull request Oct 18, 2018

Rollup merge of rust-lang#55179 - bjorn3:miri_public_op_field, r=oli-obk

cb9f6be

Make OpTy field op public for priroda r? @oli-obk

kennytm mentioned this pull request Oct 19, 2018

Rollup of 7 pull requests #55194

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 19, 2018

rust-highfive assigned RalfJung and unassigned oli-obk Oct 19, 2018

bjorn3 mentioned this pull request Oct 19, 2018

Rustup oli-obk/priroda#12

Merged

RalfJung reviewed Oct 19, 2018

View reviewed changes

RalfJung reviewed Oct 20, 2018

View reviewed changes

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

RalfJung reviewed Oct 20, 2018

View reviewed changes

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

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 21, 2018

bjorn3 added 7 commits October 21, 2018 15:49

Make OpTy field op public for priroda

ea51e32

Add method to get OpTy for local from arbitrary frame

3e62ba1

Fix errors

7d406c9

Use read_local_of_frame in eval_place_to_op

3742892

Also make `layout_of_local` accept any `Frame`

Add alloc_map accessor

825b55e

Rename alloc_map_ref to alloc_map

f46e3ba

Rename read_local_of_frame to access_local

c32cf25

bjorn3 force-pushed the miri_public_op_field branch from c571a95 to c32cf25 Compare October 21, 2018 14:00

Address review comments

b178553

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 21, 2018

bors added a commit that referenced this pull request Oct 21, 2018

Auto merge of #55179 - bjorn3:miri_public_op_field, r=RalfJung

424a749

Give OpTy access to locals for priroda r? @oli-obk

bors merged commit b178553 into rust-lang:master Oct 21, 2018

bjorn3 deleted the miri_public_op_field branch October 22, 2018 08:15

bjorn3 mentioned this pull request Jun 10, 2019

Add a method to create an arbitrary OpTy from outside rustc_mir #61718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give OpTy access to locals for priroda #55179

Give OpTy access to locals for priroda #55179

bjorn3 commented Oct 18, 2018

oli-obk commented Oct 18, 2018

bors commented Oct 18, 2018

RalfJung commented Oct 19, 2018

oli-obk commented Oct 19, 2018

kennytm commented Oct 19, 2018

oli-obk commented Oct 19, 2018

bjorn3 commented Oct 19, 2018

bjorn3 commented Oct 19, 2018

RalfJung Oct 19, 2018

bjorn3 Oct 19, 2018

RalfJung Oct 19, 2018

bjorn3 Oct 20, 2018

RalfJung Oct 20, 2018

bjorn3 Oct 20, 2018

bjorn3 commented Oct 19, 2018

bjorn3 commented Oct 20, 2018

RalfJung Oct 20, 2018

bjorn3 Oct 20, 2018

RalfJung commented Oct 21, 2018

RalfJung commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

bjorn3 commented Oct 21, 2018

bors commented Oct 21, 2018

RalfJung commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

bjorn3 commented Oct 21, 2018

RalfJung commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

Give OpTy access to locals for priroda #55179

Give OpTy access to locals for priroda #55179

Conversation

bjorn3 commented Oct 18, 2018

oli-obk commented Oct 18, 2018

bors commented Oct 18, 2018

RalfJung commented Oct 19, 2018

oli-obk commented Oct 19, 2018

kennytm commented Oct 19, 2018

oli-obk commented Oct 19, 2018

bjorn3 commented Oct 19, 2018

bjorn3 commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented Oct 19, 2018

bjorn3 commented Oct 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Oct 21, 2018

RalfJung commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

bjorn3 commented Oct 21, 2018

bors commented Oct 21, 2018

RalfJung commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

bjorn3 commented Oct 21, 2018

RalfJung commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018