Skip to content

fix array drop glue: properly turn raw ptr into reference#53536

Merged
bors merged 1 commit intorust-lang:masterfrom
RalfJung:array-drop
Aug 22, 2018
Merged

fix array drop glue: properly turn raw ptr into reference#53536
bors merged 1 commit intorust-lang:masterfrom
RalfJung:array-drop

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 20, 2018

Discovered while working on #53424: The generated drop glue uses an assignment ptr = cur where ptr is a reference and cur a raw pointer. This is not well-formed MIR.

Do we have MIR sanity checks that run on the drop glue and should have caught this?

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2018
@eddyb
Copy link
Member

eddyb commented Aug 20, 2018

cc @arielb1

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 20, 2018

📌 Commit 0447b50 has been approved by eddyb

@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 Aug 20, 2018
@arielb1
Copy link
Contributor

arielb1 commented Aug 20, 2018

I'm not sure how legitimate it is to call drop on an &mut T (maybe it is with your semantics?). If that is a problem, I would prefer the type to be *mut T everywhere.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 20, 2018

I do not understand -- drop takes &mut...

@RalfJung
Copy link
Member Author

RalfJung commented Aug 20, 2018

Also, I couldn't actually figure out how ptr is used later. Not sure if I have been looking at the wrong places or if the comments showing the MIR syntax being generated are just missing? #53532 does not help either.

@matthewjasper
Copy link
Contributor

matthewjasper commented Aug 20, 2018

ptr is used here:

rust/src/librustc_mir/util/elaborate_drops.rs

Lines 632 to 636 in 1558ae7

self.elaborator.patch().patch_terminator(drop_block, TerminatorKind::Drop {
location: ptr.clone().deref(),
target: loop_block,
unwind: unwind.into_option()
});

The mir that you want to look at is probably the one generated here:

rust/src/librustc_mir/shim.rs

Lines 160 to 236 in 6a3db03

fn build_drop_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId,
ty: Option<Ty<'tcx>>)
-> Mir<'tcx>
{
debug!("build_drop_shim(def_id={:?}, ty={:?})", def_id, ty);
// Check if this is a generator, if so, return the drop glue for it
if let Some(&ty::TyS { sty: ty::TyGenerator(gen_def_id, substs, _), .. }) = ty {
let mir = &**tcx.optimized_mir(gen_def_id).generator_drop.as_ref().unwrap();
return mir.subst(tcx, substs.substs);
}
let substs = if let Some(ty) = ty {
tcx.intern_substs(&[ty.into()])
} else {
Substs::identity_for_item(tcx, def_id)
};
let sig = tcx.fn_sig(def_id).subst(tcx, substs);
let sig = tcx.erase_late_bound_regions(&sig);
let span = tcx.def_span(def_id);
let source_info = SourceInfo { span, scope: OUTERMOST_SOURCE_SCOPE };
let return_block = BasicBlock::new(1);
let mut blocks = IndexVec::new();
let block = |blocks: &mut IndexVec<_, _>, kind| {
blocks.push(BasicBlockData {
statements: vec![],
terminator: Some(Terminator { source_info, kind }),
is_cleanup: false
})
};
block(&mut blocks, TerminatorKind::Goto { target: return_block });
block(&mut blocks, TerminatorKind::Return);
let mut mir = Mir::new(
blocks,
IndexVec::from_elem_n(
SourceScopeData { span: span, parent_scope: None }, 1
),
ClearCrossCrate::Clear,
IndexVec::new(),
None,
local_decls_for_sig(&sig, span),
sig.inputs().len(),
vec![],
span
);
if let Some(..) = ty {
let patch = {
let param_env = tcx.param_env(def_id).with_reveal_all();
let mut elaborator = DropShimElaborator {
mir: &mir,
patch: MirPatch::new(&mir),
tcx,
param_env
};
let dropee = Place::Local(Local::new(1+0)).deref();
let resume_block = elaborator.patch.resume_block();
elaborate_drops::elaborate_drop(
&mut elaborator,
source_info,
&dropee,
(),
return_block,
elaborate_drops::Unwind::To(resume_block),
START_BLOCK
);
elaborator.patch
};
patch.apply(&mut mir);
}
mir
}

@RalfJung
Copy link
Member Author

RalfJung commented Aug 21, 2018 •
edited
Loading

Ah so it moves on to the Drop terminator, which takes any place -- so we could just as well deref a raw pointer?

EDIT: OTOH, I do not see why. If the reference is valid enough for Drop to be called, it better be valid enough for &mut.

@RalfJung RalfJung mentioned this pull request Aug 22, 2018
Merged
@bors
Copy link
Collaborator

bors commented Aug 22, 2018

⌛ Testing commit 0447b50 with merge c24f27c...

bors added a commit that referenced this pull request Aug 22, 2018
@bors
Auto merge of #53536 - RalfJung:array-drop, r=eddyb
c24f27c 
fix array drop glue: properly turn raw ptr into reference

Discovered while working on #53424: The generated drop glue uses an assignment `ptr = cur` where `ptr` is a reference and `cur` a raw pointer. This is not well-formed MIR.

Do we have MIR sanity checks that run on the drop glue and should have caught this?

r? @eddyb
@bors
Copy link
Collaborator

bors commented Aug 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing c24f27c to master...

@bors bors merged commit 0447b50 into rust-lang:master Aug 22, 2018
@RalfJung RalfJung deleted the array-drop branch September 1, 2018 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@eddyb eddyb

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@RalfJung @eddyb @bors @arielb1 @matthewjasper @rust-highfive