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 provenance cleanup #96165

Merged
merged 5 commits into from
Apr 20, 2022
Merged

Miri provenance cleanup #96165

merged 5 commits into from
Apr 20, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 18, 2022 •
edited
Loading

Reviewing #95826 by @carbotaniuman made me realize that we could clean things up a little here.

@carbotaniuman please let me know if you're okay with landing this (it will create a lot of conflicts with your PR), or if you'd prefer incorporating the ideas from this PR into yours. I think we want to end up in a situation where the function you called ptr_reify_alloc returns just two things, a concrete tag and an offset. Getting an AllocId from a concrete tag should be infallible like now. However a concrete tag and Tag don't have to be the same type.

r? @oli-obk

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 18, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2022
@RalfJung
Copy link
Member Author

Getting an AllocId from a concrete tag should be infallible like now. However a concrete tag and Tag don't have to be the same type.

Or maybe it returns 3 things: AllocId, offset, and TagExtra. But either way there should be no redundancy in what it returns.

I feel like this triple approach might be better for #95826 since it avoids having to extract the AllocId from a ConcreteTag. If you want to do that @carbotaniuman let me know, otherwise I can do it in this PR -- the first commit remains the same, but the second commit would instead change the type of the third component to a new associated type that needs to be added to the machine. The hardest part about this is naming that new associated type... TagExtra isn't that great.

@RalfJung RalfJung marked this pull request as draft April 18, 2022 02:58
@RalfJung RalfJung force-pushed the miri-provenance-cleanup branch from 3b7816f to 5f7e4cf Compare April 18, 2022 02:58
@carbotaniuman
Copy link
Contributor

I like these changes, but I can't tell how the difference between the two and three element approaches will affect my code.

However a concrete tag and Tag don't have to be the same type.

Can you clarify this? It looks like according to these changes that all Pointer<M::PointerTag> can infallibly get an AllocId, which isn't the semantics that are needed.

@RalfJung RalfJung force-pushed the miri-provenance-cleanup branch from 5f7e4cf to 0ef5ae3 Compare April 18, 2022 14:00
@RalfJung RalfJung marked this pull request as ready for review April 18, 2022 14:06
@RalfJung
Copy link
Member Author

Can you clarify this? It looks like according to these changes that all Pointer<M::PointerTag> can infallibly get an AllocId, which isn't the semantics that are needed.

I have added it to this PR.
On top of this PR I think you can leave the signature of ptr_get_alloc_id (and the other similar methods) unchanged. You can make Provenance::get_alloc_id and Machine::ptr_get_alloc fallible and add the hooks for ptr2int and int2ptr casts and that should be all the changes that are needed. It should be fairly local.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
RalfJung
RalfJung commented Apr 18, 2022
View reviewed changes
compiler/rustc_const_eval/src/interpret/machine.rs
fn ptr_get_alloc(
ecx: &InterpCx<'mir, 'tcx, Self>,
ptr: Pointer<Self::PointerTag>,
) -> (AllocId, Size);
) -> (AllocId, Size, Self::TagExtra);
Copy link
Member Author

@RalfJung RalfJung Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Technically, this is currently still redundant -- we could make this ptr_get_offset and have it return just a Size, and get all the other data directly from the PointerTag.

But in preparation for @carbotaniuman's PR, I think we should leave this the way it is. After that PR, Provenance::get_alloc_id should be called only for diagnostics and when Provenance::OFFSET_IS_ADDR == false, and ptr_get_alloc should be used throughout the machine to extract information from provenance. (And that's what we already do, so it would be silly to change it back and forth.)

@RalfJung RalfJung force-pushed the miri-provenance-cleanup branch from 0ef5ae3 to c9e568f Compare April 18, 2022 14:14
@RalfJung RalfJung mentioned this pull request Apr 18, 2022
Merged
@RalfJung RalfJung force-pushed the miri-provenance-cleanup branch 2 times, most recently from 8939f71 to 41ed646 Compare April 18, 2022 15:57
@RalfJung RalfJung force-pushed the miri-provenance-cleanup branch from 41ed646 to c83241a Compare April 18, 2022 16:30
@oli-obk
Copy link
Contributor

oli-obk commented Apr 19, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Apr 19, 2022

📌 Commit 55f0977 has been approved by oli-obk

@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 Apr 19, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 19, 2022
@Dylan-DPC
Rollup merge of rust-lang#96165 - RalfJung:miri-provenance-cleanup, r…
4da2306 
…=oli-obk

Miri provenance cleanup

Reviewing rust-lang#95826 by `@carbotaniuman` made me realize that we could clean things up a little here.

`@carbotaniuman` please let me know if you're okay with landing this (it will create a lot of conflicts with your PR), or if you'd prefer incorporating the ideas from this PR into yours. I think we want to end up in a situation where the function you called `ptr_reify_alloc` returns just two things, a concrete tag and an offset. Getting an `AllocId` from a concrete tag should be infallible like now. However a concrete tag and `Tag` don't have to be the same type.

r? `@oli-obk`
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 19, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 19, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2022
@bors
Auto merge of rust-lang#96224 - Dylan-DPC:rollup-h2h3j93, r=Dylan-DPC
a2df8ba 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#95740 (asm: Add a kreg0 register class on x86 which includes k0)
 - rust-lang#95813 (Remove extra space before a where clause)
 - rust-lang#96029 (Refactor loop into iterator; simplify negation logic.)
 - rust-lang#96162 (interpret: Fix writing uninit to an allocation)
 - rust-lang#96165 (Miri provenance cleanup)
 - rust-lang#96205 (Use futex locks on emscripten.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9d9d591 into rust-lang:master Apr 20, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 20, 2022
bors added a commit to rust-lang/miri that referenced this pull request Apr 20, 2022
@bors
Auto merge of #2071 - RalfJung:provenance, r=RalfJung
c9039de 
adjust for provenance cleanup

This is the Miri side of rust-lang/rust#96165
@RalfJung RalfJung deleted the miri-provenance-cleanup branch April 26, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
7 participants
@RalfJung @rust-highfive @carbotaniuman @rust-log-analyzer @oli-obk @bors @rustbot