Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Miri engine refactoring #55915

Merged
merged 34 commits into from
Nov 25, 2018
Merged

Miri engine refactoring #55915

merged 34 commits into from
Nov 25, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 13, 2018

next small step of #55293

r? @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2018
This was referenced Nov 13, 2018
Closed
Closed
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk oli-obk mentioned this pull request Nov 13, 2018
Merged
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk oli-obk mentioned this pull request Nov 13, 2018
Merged
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the miri_engine_refactoring branch 2 times, most recently from d063639 to 700eda3 Compare November 15, 2018 13:45
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 16, 2018

this is ready for review now

RalfJung
RalfJung reviewed Nov 16, 2018
View reviewed changes
src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Nov 16, 2018
View reviewed changes
src/librustc_mir/interpret/traits.rs
@@ -61,16 +61,16 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
let drop = self.memory.create_fn_alloc(drop).with_default_tag();
self.memory
.get_mut(vtable.alloc_id)?
.write_ptr_sized(tcx, vtable, ptr_align, Scalar::Ptr(drop).into())?;
.write_ptr_sized(tcx, vtable, Scalar::Ptr(drop).into())?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment that we do not bother with alignment checks here because anyway this is just generating static data.

Actually, with Allocation having so many methods, couldn't we do more of these preparations (writing all the stuff) before adding the allocation to the memory? We might even not add it to the memory at all but intern it immediately? But that's stuff for a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems doable and probably even allows us to generalize this function so it can be used from codegen, too (without needing an eval context)

@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 16, 2018

So... I remember the exact issue now. The Place for e.g. (u32, u32, u64) is 8 byte aligned. But when I try to access the second u32, we need 4 byte alignment checks.

I presume from your comments that it's a bug if the ptr_align is 8 in try_read_immediate_from_mplace function, so the caller of the function is at fault?

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 24, 2018

@bors r=RalfJung rebased

@bors
Copy link
Contributor

bors commented Nov 24, 2018

📌 Commit b853252 has been approved by RalfJung

@bors 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 Nov 24, 2018
@bors
Copy link
Contributor

bors commented Nov 24, 2018

⌛ Testing commit b853252 with merge c52cd9476b78265b00215eb0d78a4bb1b1a5904d...

@bors
Copy link
Contributor

bors commented Nov 24, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 24, 2018
@RalfJung
Copy link
Member 

Build completed successfully in 3:01:33
Command exited with code 259

Hu? Cc @rust-lang/infra

@kennytm
Copy link
Member

kennytm commented Nov 24, 2018

@bors retry

@bors 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 Nov 24, 2018
@kennytm kennytm mentioned this pull request Nov 24, 2018
Merged
bors added a commit that referenced this pull request Nov 24, 2018
@bors
Auto merge of #56201 - kennytm:revert-55935, r=alexcrichton
edaac35 
Revert "appveyor: Use VS2017 for all our images"

This reverts commit 008e5dc (#55935)

We suspect this causes the spurious failure in #55906 (comment) and #55915 (comment).

r? @alexcrichton
bors added a commit that referenced this pull request Nov 25, 2018
@bors
Auto merge of #55915 - oli-obk:miri_engine_refactoring, r=RalfJung
2dd94c1 
Miri engine refactoring

next small step of #55293

r? @RalfJung
@bors
Copy link
Contributor

bors commented Nov 25, 2018

⌛ Testing commit b853252 with merge 2dd94c1...

@bors
Copy link
Contributor

bors commented Nov 25, 2018

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

@bors bors merged commit b853252 into rust-lang:master Nov 25, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #55915!

Tested on commit 2dd94c1.
Direct link to PR: #55915

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

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Nov 25, 2018
@rust-highfive
📣 Toolstate changed by rust-lang/rust#55915!
77e1a6e 
Tested on commit rust-lang/rust@2dd94c1.
Direct link to PR: <rust-lang/rust#55915>

🎉 miri on windows: build-fail → test-pass (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
🎉 miri on linux: build-fail → test-pass (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
This was referenced Nov 25, 2018
Merged
Merged
@nnethercote
Copy link
Contributor

Some nice perf wins for this, up to 5% across multiple benchmarks.

@oli-obk oli-obk deleted the miri_engine_refactoring branch November 30, 2018 08:35
bors added a commit that referenced this pull request Dec 5, 2018
@bors
Auto merge of #55922 - oli-obk:slice_pat_ice, r=zackmdavis
21cb46a 
Fix ICE in `const` slice patterns

fixes #55911

based on #55915

New commits start at eabc1551e0d0953f1951020afb5919ab4c129cf5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oli-obk @rust-highfive @RalfJung @bors @kennytm @nnethercote