-
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
Don't build drop shim for polymorphic types #110930
Conversation
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
Actually I think this might not be the proper fix for this. Inlining actually is only done post-monomorphization afaict (its only called through |
The reported issue is a dupe of #106444 , which has a draft PR up to fix it. We definitely do try to inline pre-mono Mir, and normally that works fine. The problem is just that drop shim generation specifically is not set up to deal with that at all, which is what causes the problems. I don't know what the right fix is either, you'll probably have to coordinate that with Camille. |
Originally In this particular case, a field type determines whether to build or omit a drop terminator. When drop terminator is built, but eventually the type doesn't need to be dropped, the drop terminator is a no-op. That seems fine. It might be worth pointing out, that the de-facto behavior of the drop shim will be different if it is created when not monomorphic. Consider the example below. If shim is generated at the point when pub enum E<A, B> {
X(A),
Y(B),
}
pub unsafe fn f<A, B>(e: *mut E<A, B>) {
std::ptr::drop_in_place(e);
} |
#106905 stumbled on this ICE #106905 (comment). This PR may have a similar one. |
I think this change wouldn't be affected by that, as we keep normalizing as we previously do, so this should not introduce any other normalization related ICEs, though it might lead to other problems. I've tried to familiarize myself more with the inlining and drop elaboration code and I believe the current behaviour of not being able to normalize during inlining introduces some inconsistencies. When we
results in the following mir after inlining for
So here we do inline the call to Whereas for the following example we do not, but we could if we would properly normalize this:
which results in the following post-inline mir:
This is because after having inlined
we cannot resolve the callsite for Is this inconsistent? When do we actually start to inline at the earliest? AFAICT the earliest call that would result in inlining is this one in If it is, then I think the proper fix is to have a version of |
Falling back to the unnormalized type may cause the other ICE: if for some reason, we cannot normalize and drop elaboration expects and ADT, boom.
I don't think this is inconsistent. The whole point of working on MIR that we optimize MIR in its polymorphic form. The If polymorphism prevents some inlining, then we leave to LLVM to perform the monomorphic inlining. However, specifically for drop shims, if trying to manufacture polymorphic drop shims is an issue, we may need to check that the instance is monomorphic enough and skip trying to inline it if it isn't. |
let field_ty = match tcx | ||
.try_normalize_erasing_regions(self.elaborator.param_env(), f.ty(tcx, substs)) |
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.
Nit
let field_ty = match tcx | |
.try_normalize_erasing_regions(self.elaborator.param_env(), f.ty(tcx, substs)) | |
let field_ty = match tcx | |
.try_normalize_erasing_regions(self.elaborator.param_env(), fty) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
) -> Result<&'tcx Body<'tcx>, &'static str> { | ||
match instance { | ||
ty::InstanceDef::DropGlue(_, Some(ty)) => match ty.kind() { | ||
ty::Adt(def, substs) => { |
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.
Not sure if we want to do this in a more generalized way. I believe the normalization in move_paths_for_fields
is the only place where building drop shims can go wrong, so rejecting those types here should be sufficient (though this change feels a little hacky).
Rejecting these here seemed preferable to me compared to making changes in the shim/drop elaboration code.
This comment has been minimized.
This comment has been minimized.
0ff8a6f
to
a20ce57
Compare
a20ce57
to
e7a2f52
Compare
@bors r+ |
Rollup of 6 pull requests Successful merges: - rust-lang#110930 (Don't expect normalization to succeed in elaborate_drops) - rust-lang#111557 (Revert "Validate resolution for SelfCtor too.") - rust-lang#111565 (rustdoc-json: Add tests for visibility of impls) - rust-lang#111588 (Emits E0599 when meeting `MyTrait::missing_method`) - rust-lang#111625 (Move rustc_middle/src/ty/query.rs to rustc_middle/src/query/plumbing.rs) - rust-lang#111674 (Add missing backslash in HTML string) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #110682
This was exposed through the changes in #109247, which causes more things to be inlined. Inlining can happen before monomorphization, so we can't expect normalization to succeed. In the elaborate_drops analysis we currently have this call to
normalize_erasing_regions
, which ICEs when normalization fails. The types are used to infer whether the type needs a drop, whereneeds_drop
itself usestry_normalize_erasing_regions
.instance_mir
isn't explicit about whether it expects the instances corresponding to theInstanceDef
s to be monomorphized (though I think in all other contexts the function is used post-monomorphization), so the use ofinstance_mir
in inlining doesn't necessarily seem wrong to me.