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.

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

Miri: refactor handling of "uncanonical" AllocIds #71194

Closed
RalfJung opened this issue Apr 16, 2020 · 3 comments · Fixed by #74775
Closed

Miri: refactor handling of "uncanonical" AllocIds #71194

RalfJung opened this issue Apr 16, 2020 · 3 comments · Fixed by #74775
Assignees
Labels
A-miri Area: The miri tool C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 16, 2020

There are (at least) two situations where Miri uses a different AllocId for allocations than would be used in already created "global" allocations:

  • extern static are supported by mapping them to another, existing allocation.
  • TLS statics need a different AllocId for each thread.

Currently both of these use different systems, and the extern static system has some disadvantages like not erroring on this program:

extern {
    static mut TEST: i32;
}

fn main() {
    let _x = unsafe { &TEST };
}

We should use the same system for both, and that system should be to do whatever adjustments we need to do when converting Pointer<()> (an untagged pointer, in global tcx memory) to Pointer<M::PointerTag>.

With #70685 resolved we might not need this for TLS statics any more, depending on the solution. But it would still be good to do this for extern static. Either way, as part of this we should also cleanup the TLS static handling in Miri, which currently special-cases ConstValue::Scalar being converted into Miri machine values -- we should do the right thing for all pointers in constants/globals.

Cc @oli-obk

@RalfJung RalfJung added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-miri Area: The miri tool labels Apr 16, 2020
@RalfJung RalfJung self-assigned this Apr 16, 2020
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 16, 2020
@RalfJung
Copy link
Member Author

Also I noted const_eval and eval_const_to_op are somewhat confusingly named in Miri. And const_eval has a suboptimal interface; @oli-obk can we avoid having to pass the ty and somehow compute that from the gid?

@RalfJung
Copy link
Member Author

RalfJung commented Apr 16, 2020

Also I wonder what it would take to avoid layering violations like https://github.com/rust-lang/miri/pull/1284/files#r409513235 (Cc @vakaras). Maybe Miri's const_eval should not already do eval_const_to_op (then it would not need a ty any more either) and instead return both a ConstValue and the AllocId backing that value (and that AllocId would be guaranteed to be a GlobalAlloc::Memory in tcx.alloc_map)?

Also proposed new name for eval_const_to_op would be just const_to_op... this function, conceptually, is more conversion than evaluation; the fact that it has to evaluate anything is just due to the lazy nature of rustc's query system. That is unlike, e.g., eval_place_to_op with evaluates a mir::Place to an interpret::Place.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 26, 2020

Also I noted const_eval and eval_const_to_op are somewhat confusingly named in Miri. And const_eval has a suboptimal interface; @oli-obk can we avoid having to pass the ty and somehow compute that from the gid?

we can (tcx.type_of + substituting), but that may be expensive.

@bors bors closed this as completed in 4a90e36 Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-miri Area: The miri tool C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants