Miri engine cleanup #53671
Miri engine cleanup #53671
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
RalfJung
force-pushed
the
miri-refactor
branch
2 times, most recently
from
0627c37
to
395a1a7
Compare
|
☔ The latest upstream changes (presumably #53609) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@eddyb pointed out that the way validation handles user-defined types was wrong. I am working on fixing that. Good news is that the |
RalfJung
force-pushed
the
miri-refactor
branch
from
000b906
to
823335e
Compare
|
Done! I will stopp adding stuff to this PR now, I promise. :) (If it ends up green.) |
|
☔ The latest upstream changes (presumably #53567) made this pull request unmergeable. Please resolve the merge conflicts. |
| // This should always be (), but getting it from the sig seems | ||
| // easier than creating a layout of (). | ||
| let dest = PlaceTy::null(&self, self.layout_of(fn_sig.output())?); | ||
| let ty = self.tcx.mk_tup((&[] as &[ty::Ty<'tcx>]).iter()); // return type is () |
There was a problem hiding this comment.
this is just self.tcx.mk_nil()
There was a problem hiding this comment.
D'oh, thanks! "nil" is not the name I was looking for...
There was a problem hiding this comment.
Yea this is naming from ancient Rust... We should probably rename all nil in the compiler to unit. I'll open an easy issue
| @@ -275,19 +275,19 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { | |||
| return Ok(false); | |||
| } | |||
|
|
|||
| /// Call this function -- pushing the stack frame and initializing the arguments. | |||
| /// `sig` is ptional in case of FnPtr/FnDef -- but mandatory for closures! | |||
src/librustc_mir/interpret/place.rs
Outdated
| // a fake pointer? Are we even called for ZST? | ||
|
|
||
| // We need the layout of the local. We can NOT use the layout we got, | ||
| // that might e.g. be a downcast variant! |
There was a problem hiding this comment.
what's wrong with the downcast variant? It should be the same size and alignment
There was a problem hiding this comment.
The comment was introduced by the previous PR, where it was needed when I had places pointing into locals through a field field in the Place::local variant.
Is the downcast variant always guaranteed to be like that? And there can also be structs wrapping other structs, maintaining the Scalar layout -- and some of the outer structs might have repr but the place we are seeing here is already for the inner field. That would change the alignment and be relevant here, right?
There was a problem hiding this comment.
right. That makes sense. Just expand the comment to mention the inner field case
| { | ||
| return Ok(handled); | ||
| // Handle non-integer operations | ||
| if let ty::Char = left_layout.ty.sty { |
There was a problem hiding this comment.
can you make these if lets a match over the type + method calls instead of having the logic right here?
There was a problem hiding this comment.
You want one function per type?
There was a problem hiding this comment.
if that's not too much hazzle, yes
| let variant = match self.read_discriminant(dest) { | ||
| Ok(res) => res.1, | ||
| Err(err) => match err.kind { | ||
| EvalErrorKind::InvalidDiscriminant | |
There was a problem hiding this comment.
This is a slight regression in diagnostics. Can you include the invalid value and expected values in the error variant and report that in the form of found "42" expected one of [5, 6, 7] or similar?
Also the ReadPointerAsBytes error should report that it found a pointer where it expected an integral discriminant
There was a problem hiding this comment.
You think those lists of possible discriminant values are useful? The old messages showed a range, giving the lower and largest possible value. That was certainly not useful because intermediate values were also invalid. So in terms of usefulness this is not a regression...
Getting all that information here will be really hard, I think.
In a later commit, this is changed to always say "invalid discriminant", even for ReadPointerAsBytes.
There was a problem hiding this comment.
Ok, let's leave this alone for now. Seems like a minor issue, as the user can figure out the possible values themselves. Displaying the found value is still helpful though.
There was a problem hiding this comment.
Done (with next push)
Adding/changing a variant in the EvalErrorKind enum is way too much effort...
src/librustc_mir/interpret/place.rs
Outdated
| extra: PlaceExtra::None, | ||
| } | ||
| } | ||
| let mplace = if layout.is_unsized() { |
There was a problem hiding this comment.
won't this get us into trouble with extern types? They are unsized but have no extra
There was a problem hiding this comment.
@eddyb says they technically have an extra of (). But you are right that this might still not work because their extra is not pointer-sized.
The old code here assumed that Dynamic, Str and Slice are the only possible unsized tails.
What could a testcase look like?
There was a problem hiding this comment.
#![feature(extern_types)]
extern {
type Foo;
}
fn main() {
let x: &Foo = unsafe { &*(16 as *const Foo) };
let y: &Foo = &*x;
}might already trigger it
There was a problem hiding this comment.
Hm, you are right, And I have no idea why this worked on old miri...
RalfJung
force-pushed
the
miri-refactor
branch
from
823335e
to
f86c476
Compare
|
I rebased, and also managed to get rid of the |
|
Let's ask perf... @bors try |
Miri engine cleanup
* Unify the two maps in memory to store the allocation and its kind together.
* Share the handling of statics between CTFE and miri: The miri engine always
uses "lazy" `AllocType::Static` when encountering a static. Acessing that
static invokes CTFE (no matter the machine). The machine only has any
influence when writing to a static, which CTFE outright rejects (but miri
makes a copy-on-write).
* Add an `AllocId` to by-ref consts so miri can use them as operands without
making copies.
* Move responsibilities around for the `eval_fn_call` machine hook: The hook
just has to find the MIR (or entirely take care of everything); pushing the
new stack frame is taken care of by the miri engine.
* Expose the intrinsics and lang items implemented by CTFE so miri does not
have to reimplement them.
* Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks).
* Clean up function calling.
* Switch const sanity check to work on operands, not mplaces.
* Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details.
In particular, we can finally make `eval_operand` take `&self`. :-)
Should be merged after #53609, across which I will rebase.
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@rust-timer build 3c7dbff |
|
Success: Queued 3c7dbff with parent caed80b, comparison URL. |
RalfJung
force-pushed
the
miri-refactor
branch
from
8ded044
to
6c10acb
Compare
|
Surprisingly there are perf differences. Seems the only things that got noticeably slower are the CTFE stress tests; OTOH coercions got significantly faster -- likely because strings are now validated all at once using |
|
Ah yes... getting miri to work again on all platforms will take a while. I will back out the miri update part of this, and solve that in the miri repo. |
RalfJung
force-pushed
the
miri-refactor
branch
from
c4c51be
to
c9b5fac
Compare
|
@bors r=oli-obk |
|
📌 Commit c9b5fac has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=6 |
Miri engine cleanup
* Unify the two maps in memory to store the allocation and its kind together.
* Share the handling of statics between CTFE and miri: The miri engine always
uses "lazy" `AllocType::Static` when encountering a static. Acessing that
static invokes CTFE (no matter the machine). The machine only has any
influence when writing to a static, which CTFE outright rejects (but miri
makes a copy-on-write).
* Add an `AllocId` to by-ref consts so miri can use them as operands without
making copies.
* Move responsibilities around for the `eval_fn_call` machine hook: The hook
just has to find the MIR (or entirely take care of everything); pushing the
new stack frame is taken care of by the miri engine.
* Expose the intrinsics and lang items implemented by CTFE so miri does not
have to reimplement them.
* Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks).
* Clean up function calling.
* Switch const sanity check to work on operands, not mplaces.
* Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details.
In particular, we can finally make `eval_operand` take `&self`. :-)
Should be merged after #53609, across which I will rebase.
|
☀️ Test successful - status-appveyor, status-travis |
|
This was a big (~10%) compile speed win for Other changes are smaller, and might just be noise, it's a bit hard to tell. |
|
@nnethercote Yes, constants containing strings got faster because we now validate the entire string at once, instead of byte-for-byte. @oli-obk I suppose we could make similar optimizations for slices of simple types (integer, float). Not sure if it's worth it? Probably worth writing down somewhere, at least. |
|
Done #53845 |
|
This was also a huge memory win for the old |
| } | ||
| impl<'tcx> PartialEq for OpTy<'tcx> { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.op == other.op && self.layout.ty == other.layout.ty |
There was a problem hiding this comment.
This is a similar problem as with #53424. Maybe we should just derive PartialEq, Eq, Hash on Layout and TyLayout?!
There was a problem hiding this comment.
It is the same problem in fact.
Why have they not been derived in the first place?
There was a problem hiding this comment.
I deleted the comment after seeing that PR.
| /// Metadata for unsized places. Interpretation is up to the type. | ||
| /// Must not be present for sized types, but can be missing for unsized types | ||
| /// (e.g. `extern type`). | ||
| pub extra: Option<Scalar>, |
impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.
impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.
impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.
impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.
uses "lazy"
AllocType::Staticwhen encountering a static. Acessing thatstatic invokes CTFE (no matter the machine). The machine only has any
influence when writing to a static, which CTFE outright rejects (but miri
makes a copy-on-write).
AllocIdto by-ref consts so miri can use them as operands withoutmaking copies.
eval_fn_callmachine hook: The hookjust has to find the MIR (or entirely take care of everything); pushing the
new stack frame is taken care of by the miri engine.
have to reimplement them.
In particular, we can finally make
eval_operandtake&self. :-)Should be merged after #53609, across which I will rebase.