-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make libcore pass -Zvalidate-mir #73175
Conversation
Yeah I think we should land this for now. But can you add a FIXME comment that points out that the shim shouldn't contain a call to |
I can definitely add a comment, but the problems are these and I can't imagine them ever changing:
|
Are those functions really what generates the bad MIR? I'd expect them to use the |
Yeah that's where it's quoted as coming from in the ICE. Essentially because of the blanket impls you end up unable to normalize |
The ICE points to the trait definition: rust/src/libcore/ops/function.rs Lines 230 to 232 in feb3536
If this was really caused by that call, I'd expect the span to point there. MIR shims appear to use the Line 688 in feb3536
I think that would end up producing what we see. |
Hmm... I could have sworn the ICE was sourced at those impls but you're right I guess I'll add that FIXME then |
During which MIR pass was that error happening? I am pretty sure that in later passes, we shouldn't see So, just allowing this is not a solution. We should understand why validation fails but Miri does not. |
// We have a `FnPtr` or `FnDef` which is trivially safe to call. | ||
} else if let Some(fn_once_trait) = self.tcx.lang_items().fn_once_trait() { | ||
// FIXME(doctorn): call shims shouldn't reference unnormalized types, so | ||
// this case should be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try calling normalize_erasing_regions
with param_env.with_reveal_all()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried normalize_erasing_regions
(didn't help) - not with that param env though. What does param_env.with_reveal_all
actually do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully know. :/ It has something to do with how much stuff can be unfolded. For example, with_reveal_all
will unfold impl Trait
types to their underlying definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It lets you see through impl Trait
and specialization. Not sure that would do anything here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to unfold Self
.
Which TyKind
variant even corresponds to Self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not completely mistaken, Self
is TyKind::Param
, which is substs[0]
when using InternalSubsts::identity_for_item
.
TyKind::Closure
should also be trivially safe to call here.
I think we still need the below branch in case someone implements FnOnce
manually though. e.g. Box<impl FnOnce>
is also callable and is represented as TyKind::Adt
afaik.
fn foo(b: Box<impl FnOnce()>) {
b()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TyKind::Closure should also be trivially safe to call here.
No. That is the type of the environment of a closure, which is not callable. It just implements a trait that permits some surface-level sugar. Miri will ICE when a Call
terminator is a Closure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you are saying makes sense on the HIR, but not any more on the MIR, which is more lower-level and where closure conversion has already happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, that's interesting 👍
So my comment mentioning Box<impl FnOnce>
is also irrelevant, as at this stage, the call of b
should be represented as TyKind::FnDef(def_id = FnOnce::call_once, substs = [Box<impl FnOnce()>, (,)])
I think. Thanks ❤️
edit: I would then expect normalize_erasing_regions
with Reveal::All
to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as at this stage, the call of b should be represented as TyKind::FnDef(def_id = FnOnce::call_once, substs = [Box<impl FnOnce()>, (,)]) I think
Exactly. :)
(I used to have the exact same misconception.^^ The only reason I know this is that I found the relevant code in Miri at some point and realized it would actually ICE for TyKind::Closure
... but closures work fine in Miri so something was clearly going on. This is actually great for Miri as it means we need to do no work at all to support closures; MIR construction has already done all the work for us.)
ICE occurs in 6 MIR passes - my understanding here is clearly fairly cloudy but it the |
The question is more, is it getting removed again at some point? |
I'm okay landing this if we keep the issue open. But this doesn't actually resolve anything, it just papers over a problem. That can be a reasonable thing to do, but warrants an issue to track fixing it properly at some point. :) |
Ok - removed the resolves tag and I'll take a serious look at the shim code this week. I'll add a note in the issue. |
The last ICE is this but I have no idea how deep this is in the pipeline:
|
That's still pretty early. So maybe we can have a flag for the phase (const/optimizing... though these names are awful) and only allow these callers in early phases. The final check that we do after |
It's the last pass we do on shims. Shims only have one set of transforms applied to the after they're generated. |
☔ The latest upstream changes (presumably #73190) made this pull request unmergeable. Please resolve the merge conflicts. |
Hm, fair. My point stands though: the validation we do from |
I think this should be ok to land now if you still think it's a good idea - I'm still going to investigate the shims either way so it's up to you guys |
As I just said, my concern remains that this accepts |
Ah sorry - I misunderstood that. |
To be clear, I think this PR is fine if we add a parameter to validation that tells it whether this is a shim or not. Then all the extra code this adds should only run for shims. That's a reasonable band-aid until we decide if we want to adjust shim MIR (so that the "what is callable" invariant is globally uniform) or do something else. |
@@ -26,12 +31,16 @@ impl<'tcx> MirPass<'tcx> for Validator { | |||
fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { | |||
let def_id = source.def_id(); | |||
let param_env = tcx.param_env(def_id); | |||
TypeChecker { when: &self.when, def_id, body, tcx, param_env }.visit_body(body); | |||
let validating_shim = | |||
if let ty::InstanceDef::Item(_) = source.instance { false } else { true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's interesting, I thought this would be passed in. But I guess this works, too? I am not very familiar with InstanceDef
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we found any instances where something that is not Self
is being called? Because then you can also special-case this even further by checking that the callee is ty.is_param(0)
, instead of doing a trait query. And we could also only special-case the specific call shims we care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have, but I think that this approach is a little more liberal in the sense that it allows shim implementations to change without changing the assumptions we require here - there's definitely a trade-off here though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shims shouldn't grow more ways to violate the assumptions we make here, they should only change so they use an FnDef
instead of Self
as the callee eventually, fixing the underlying issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean to say that another class of shim could be introduced or a previously monomorphic shim could be made polymorphic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's tidier to check ty.is_param(0)
, but we really would like Self
to implement FnOnce
right? It would almost certainly be malformed MIR if it didn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I can think at the moment is that we include both checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you can just compare with tcx.types.self_param
too
Great. :)
Hm, I'd still like to understand why Shims can "violate" the rules here and why that doesn't ICE in Miri. Does |
I really don't know anything about MIRI internals so I couldn't begin to comment, have you got any ideas about how we could go about investigating that? |
I am really confused by why the validator thinks that the type // MIR for `std::ops::FnOnce::call_once` before AddMovesForPackedDrops
fn std::ops::FnOnce::call_once(_1: Self, _2: Args) -> <Self as std::ops::FnOnce<Args>>::Output {
let mut _0: <Self as std::ops::FnOnce<Args>>::Output;
let _3: &mut Self;
bb0: {
_3 = &mut _1;
_0 = const <Self as std::ops::FnMut<Args>>::call_mut(move _3, move _2) -> [return: bb1, unwind: bb3];
// ty::Const
// + ty: for<'r> extern "rust-call" fn(&'r mut Self, Args) -> <Self as std::ops::FnOnce<Args>>::Output {<Self as std::ops::FnMut<Args>>::call_mut}
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: ~/.rustup/toolchains/nightly-2020-06-12-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5: 232:71
// + literal: Const { ty: for<'r> extern "rust-call" fn(&'r mut Self, Args) -> <Self as std::ops::FnOnce<Args>>::Output {<Self as std::ops::FnMut<Args>>::call_mut}, val: Value(Scalar(<ZST>)) }
}
bb1: {
drop(_1) -> bb2;
}
bb2: {
return;
}
bb3 (cleanup): {
drop(_1) -> bb4;
}
bb4 (cleanup): {
resume;
}
} It clearly calls an |
@bjorn3 I'm not really sure, but you might be looking at an |
I used $ echo 'fn main() {
let mut v: Vec<u8> = Vec::with_capacity(1); v.extend_from_slice(b"a"); std::mem::forget(v);
}' | rustc -Zdump-mir=all -Zvalidate-mir -Cpanic=abort -
error: internal compiler error: broken MIR in DefId(2:2112 ~ core[44b8]::ops[0]::function[0]::FnOnce[0]::call_once[0]) (input to phase Const) at bb0[0]:
encountered non-callable type Self in `Call` terminator
--> /home/bjorn/.rustup/toolchains/nightly-2020-06-12-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
|
232 | extern "rust-call" fn call_once(self, args: Args) -> Self::Output;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[...] The only files in
Excluding the
|
I've managed to fix the shim creation to avoid |
I'm going to tidy this up following our conversations yesterday in case #73359 can't land for some reason but I'm really hoping we don't need this |
Extends call-ability for a type
T
to pass wheneverT: FnOnce
.r? @jonas-schievink
Resolves #73109