Address review feedback

This commit is contained in:
Gary Guo 2022-11-15 17:00:40 +00:00
parent e3f2edc75b
commit 3af45d6c57
7 changed files with 40 additions and 31 deletions

View File

@ -147,7 +147,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
}
/// Call `fn_ptr` of `fn_abi` with the arguments `llargs`, the optional
/// return destination `destination` and the cleanup function `cleanup`.
/// return destination `destination` and the unwind action `unwind`.
fn do_call<Bx: BuilderMethods<'a, 'tcx>>(
&self,
fx: &mut FunctionCx<'a, 'tcx, Bx>,
@ -234,7 +234,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
}
}
/// Generates inline assembly with optional `destination` and `cleanup`.
/// Generates inline assembly with optional `destination` and `unwind`.
fn do_inlineasm<Bx: BuilderMethods<'a, 'tcx>>(
&self,
fx: &mut FunctionCx<'a, 'tcx, Bx>,

View File

@ -232,6 +232,24 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
}
}
fn check_unwind_edge(&mut self, location: Location, unwind: UnwindAction) {
let is_cleanup = self.body.basic_blocks[location.block].is_cleanup;
match unwind {
UnwindAction::Cleanup(unwind) => {
if is_cleanup {
self.fail(location, "unwind on cleanup block");
}
self.check_edge(location, unwind, EdgeKind::Unwind);
}
UnwindAction::Continue => {
if is_cleanup {
self.fail(location, "unwind on cleanup block");
}
}
UnwindAction::Unreachable | UnwindAction::Terminate => (),
}
}
/// Check if src can be assigned into dest.
/// This is not precise, it will accept some incorrect assignments.
fn mir_assign_valid_types(&self, src: Ty<'tcx>, dest: Ty<'tcx>) -> bool {
@ -902,9 +920,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
}
TerminatorKind::Drop { target, unwind, .. } => {
self.check_edge(location, *target, EdgeKind::Normal);
if let UnwindAction::Cleanup(unwind) = unwind {
self.check_edge(location, *unwind, EdgeKind::Unwind);
}
self.check_unwind_edge(location, *unwind);
}
TerminatorKind::Call { func, args, destination, target, unwind, .. } => {
let func_ty = func.ty(&self.body.local_decls, self.tcx);
@ -918,9 +934,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
if let Some(target) = target {
self.check_edge(location, *target, EdgeKind::Normal);
}
if let UnwindAction::Cleanup(cleanup) = unwind {
self.check_edge(location, *cleanup, EdgeKind::Unwind);
}
self.check_unwind_edge(location, *unwind);
// The call destination place and Operand::Move place used as an argument might be
// passed by a reference to the callee. Consequently they must be non-overlapping.
@ -958,9 +972,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
);
}
self.check_edge(location, *target, EdgeKind::Normal);
if let UnwindAction::Cleanup(cleanup) = unwind {
self.check_edge(location, *cleanup, EdgeKind::Unwind);
}
self.check_unwind_edge(location, *unwind);
}
TerminatorKind::Yield { resume, drop, .. } => {
if self.body.generator.is_none() {
@ -992,17 +1004,13 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
);
}
self.check_edge(location, *real_target, EdgeKind::Normal);
if let UnwindAction::Cleanup(unwind) = unwind {
self.check_edge(location, *unwind, EdgeKind::Unwind);
}
self.check_unwind_edge(location, *unwind);
}
TerminatorKind::InlineAsm { destination, unwind, .. } => {
if let Some(destination) = destination {
self.check_edge(location, *destination, EdgeKind::Normal);
}
if let UnwindAction::Cleanup(cleanup) = unwind {
self.check_edge(location, *cleanup, EdgeKind::Unwind);
}
self.check_unwind_edge(location, *unwind);
}
TerminatorKind::GeneratorDrop => {
if self.body.generator.is_none() {

View File

@ -13,7 +13,7 @@ pub struct MirPatch<'tcx> {
new_locals: Vec<LocalDecl<'tcx>>,
resume_block: Option<BasicBlock>,
// Only for unreachable in cleanup path.
unreachable_block: Option<BasicBlock>,
unreachable_cleanup_block: Option<BasicBlock>,
terminate_block: Option<BasicBlock>,
body_span: Span,
next_local: usize,
@ -28,7 +28,7 @@ impl<'tcx> MirPatch<'tcx> {
new_locals: vec![],
next_local: body.local_decls.len(),
resume_block: None,
unreachable_block: None,
unreachable_cleanup_block: None,
terminate_block: None,
body_span: body.span,
};
@ -41,8 +41,11 @@ impl<'tcx> MirPatch<'tcx> {
}
// Check if we already have an unreachable block
if let TerminatorKind::Unreachable = block.terminator().kind && block.statements.is_empty() {
result.unreachable_block = Some(bb);
if let TerminatorKind::Unreachable = block.terminator().kind
&& block.statements.is_empty()
&& block.is_cleanup
{
result.unreachable_cleanup_block = Some(bb);
continue;
}
@ -73,8 +76,8 @@ impl<'tcx> MirPatch<'tcx> {
bb
}
pub fn unreachable_block(&mut self) -> BasicBlock {
if let Some(bb) = self.unreachable_block {
pub fn unreachable_cleanup_block(&mut self) -> BasicBlock {
if let Some(bb) = self.unreachable_cleanup_block {
return bb;
}
@ -86,7 +89,7 @@ impl<'tcx> MirPatch<'tcx> {
}),
is_cleanup: true,
});
self.unreachable_block = Some(bb);
self.unreachable_cleanup_block = Some(bb);
bb
}

View File

@ -523,7 +523,7 @@ pub struct CopyNonOverlapping<'tcx> {
/// The basic block pointed to by a `Cleanup` unwind action must have its `cleanup` flag set.
/// `cleanup` basic blocks have a couple restrictions:
/// 1. All `unwind` fields in them must be `UnwindAction::Terminate` or `UnwindAction::Unreachable`.
/// 2. `Return` terminators are not allowed in them. `Terminate` and `Unwind` terminators are.
/// 2. `Return` terminators are not allowed in them. `Terminate` and `Resume` terminators are.
/// 3. All other basic blocks (in the current body) that are reachable from `cleanup` basic blocks
/// must also be `cleanup`. This is a part of the type system and checked statically, so it is
/// still an error to have such an edge in the CFG even if it's known that it won't be taken at
@ -721,8 +721,6 @@ pub enum TerminatorKind<'tcx> {
/// consider it in borrowck. We don't want to accept programs which
/// pass borrowck only when `panic=abort` or some assertions are disabled
/// due to release vs. debug mode builds.
/// This field does not necessary have to be `UnwindAction::Cleanup` because
/// of the `remove_noop_landing_pads` and `abort_unwinding_calls` passes.
unwind: UnwindAction,
},

View File

@ -415,7 +415,7 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {
UnwindAction::Cleanup(cleanup) => Unwind::To(cleanup),
UnwindAction::Continue => Unwind::To(self.patch.resume_block()),
UnwindAction::Unreachable => {
Unwind::To(self.patch.unreachable_block())
Unwind::To(self.patch.unreachable_cleanup_block())
}
UnwindAction::Terminate => {
Unwind::To(self.patch.terminate_block())

View File

@ -1063,7 +1063,7 @@ fn elaborate_generator_drops<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
Unwind::To(match *unwind {
UnwindAction::Cleanup(tgt) => tgt,
UnwindAction::Continue => elaborator.patch.resume_block(),
UnwindAction::Unreachable => elaborator.patch.unreachable_block(),
UnwindAction::Unreachable => elaborator.patch.unreachable_cleanup_block(),
UnwindAction::Terminate => elaborator.patch.terminate_block(),
})
};

View File

@ -543,7 +543,7 @@ impl<'tcx> CloneShimBuilder<'tcx> {
TerminatorKind::Drop {
place: dest_field,
target: unwind,
unwind: UnwindAction::Continue,
unwind: UnwindAction::Terminate,
},
true,
);
@ -814,7 +814,7 @@ fn build_call_shim<'tcx>(
TerminatorKind::Drop {
place: rcvr_place(),
target: BasicBlock::new(4),
unwind: UnwindAction::Continue,
unwind: UnwindAction::Terminate,
},
true,
);