Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rustc_monomorphize: Introduce check_fn_args_move_size() #116310

Merged
merged 2 commits into from
Oct 7, 2023
Merged

rustc_monomorphize: Introduce check_fn_args_move_size() #116310

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41d24cc
rustc_monomorphize: Move limit check into check_move_size()
Enselic Oct 1, 2023
56e4715
rustc_monomorphize: Introduce check_fn_args_move_size()
Enselic Oct 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 83 additions & 58 deletions compiler/rustc_monomorphize/src/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ fn collect_items_rec<'tcx>(
hir::InlineAsmOperand::SymFn { anon_const } => {
let fn_ty =
tcx.typeck_body(anon_const.body).node_type(anon_const.hir_id);
visit_fn_use(tcx, fn_ty, false, *op_sp, &mut used_items, &[]);
visit_fn_use(tcx, fn_ty, false, *op_sp, &mut used_items);
}
hir::InlineAsmOperand::SymStatic { path: _, def_id } => {
let instance = Instance::mono(tcx, *def_id);
Expand Down Expand Up @@ -593,11 +593,9 @@ struct MirUsedCollector<'a, 'tcx> {
instance: Instance<'tcx>,
/// Spans for move size lints already emitted. Helps avoid duplicate lints.
move_size_spans: Vec<Span>,
/// If true, we should temporarily skip move size checks, because we are
/// processing an operand to a `skip_move_check_fns` function call.
skip_move_size_check: bool,
visiting_call_terminator: bool,
/// Set of functions for which it is OK to move large data into.
skip_move_check_fns: Vec<DefId>,
skip_move_check_fns: Option<Vec<DefId>>,
}

impl<'a, 'tcx> MirUsedCollector<'a, 'tcx> {
Expand All @@ -613,7 +611,20 @@ impl<'a, 'tcx> MirUsedCollector<'a, 'tcx> {
)
}

fn check_move_size(&mut self, limit: usize, operand: &mir::Operand<'tcx>, location: Location) {
fn check_operand_move_size(&mut self, operand: &mir::Operand<'tcx>, location: Location) {
let limit = self.tcx.move_size_limit().0;
if limit == 0 {
return;
}

// This function is called by visit_operand() which visits _all_
// operands, including TerminatorKind::Call operands. But if
// check_fn_args_move_size() has been called, the operands have already
// been visited. Do not visit them again.
if self.visiting_call_terminator {
return;
}

let limit = Size::from_bytes(limit);
let ty = operand.ty(self.body, self.tcx);
let ty = self.monomorphize(ty);
Expand Down Expand Up @@ -651,6 +662,38 @@ impl<'a, 'tcx> MirUsedCollector<'a, 'tcx> {
);
self.move_size_spans.push(source_info.span);
}

fn check_fn_args_move_size(
&mut self,
callee_ty: Ty<'tcx>,
args: &[mir::Operand<'tcx>],
location: Location,
) {
let limit = self.tcx.move_size_limit();
if limit.0 == 0 {
return;
}

if args.is_empty() {
return;
}

// Allow large moves into container types that themselves are cheap to move
let ty::FnDef(def_id, _) = *callee_ty.kind() else {
return;
};
if self
.skip_move_check_fns
.get_or_insert_with(|| build_skip_move_check_fns(self.tcx))
.contains(&def_id)
{
return;
}

for arg in args {
self.check_operand_move_size(arg, location);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is Operand::Move, should the lint really fire? At least in some cases those don't cause an actual memcpy so there's no reason to lint against them. (But that only applies to function arguments! When used in Rvalue, they do still incur a memcpy.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is indeed something we need to take into account and that I plan on doing later. I added a task

Differentiate between Operand::Move and Operand::Copy

about it to the tracking issue now for improved visibility.

}
}
}

impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> {
Expand Down Expand Up @@ -696,14 +739,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> {
) => {
let fn_ty = operand.ty(self.body, self.tcx);
let fn_ty = self.monomorphize(fn_ty);
visit_fn_use(
self.tcx,
fn_ty,
false,
span,
&mut self.output,
&self.skip_move_check_fns,
);
visit_fn_use(self.tcx, fn_ty, false, span, &mut self.output);
}
mir::Rvalue::Cast(
mir::CastKind::PointerCoercion(PointerCoercion::ClosureFnPointer(_)),
Expand Down Expand Up @@ -775,17 +811,11 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> {
};

match terminator.kind {
mir::TerminatorKind::Call { ref func, .. } => {
mir::TerminatorKind::Call { ref func, ref args, .. } => {
let callee_ty = func.ty(self.body, tcx);
let callee_ty = self.monomorphize(callee_ty);
self.skip_move_size_check = visit_fn_use(
self.tcx,
callee_ty,
true,
source,
&mut self.output,
&self.skip_move_check_fns,
)
self.check_fn_args_move_size(callee_ty, args, location);
visit_fn_use(self.tcx, callee_ty, true, source, &mut self.output)
}
mir::TerminatorKind::Drop { ref place, .. } => {
let ty = place.ty(self.body, self.tcx).ty;
Expand All @@ -797,7 +827,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> {
match *op {
mir::InlineAsmOperand::SymFn { ref value } => {
let fn_ty = self.monomorphize(value.const_.ty());
visit_fn_use(self.tcx, fn_ty, false, source, &mut self.output, &[]);
visit_fn_use(self.tcx, fn_ty, false, source, &mut self.output);
}
mir::InlineAsmOperand::SymStatic { def_id } => {
let instance = Instance::mono(self.tcx, def_id);
Expand Down Expand Up @@ -835,16 +865,14 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> {
push_mono_lang_item(self, reason.lang_item());
}

self.visiting_call_terminator = matches!(terminator.kind, mir::TerminatorKind::Call { .. });
self.super_terminator(terminator, location);
self.skip_move_size_check = false;
self.visiting_call_terminator = false;
}

fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, location: Location) {
self.super_operand(operand, location);
let move_size_limit = self.tcx.move_size_limit().0;
if move_size_limit > 0 && !self.skip_move_size_check {
self.check_move_size(move_size_limit, operand, location);
}
self.check_operand_move_size(operand, location);
}

fn visit_local(
Expand Down Expand Up @@ -873,11 +901,8 @@ fn visit_fn_use<'tcx>(
is_direct_call: bool,
source: Span,
output: &mut MonoItems<'tcx>,
skip_move_check_fns: &[DefId],
) -> bool {
let mut skip_move_size_check = false;
) {
if let ty::FnDef(def_id, args) = *ty.kind() {
skip_move_size_check = skip_move_check_fns.contains(&def_id);
let instance = if is_direct_call {
ty::Instance::expect_resolve(tcx, ty::ParamEnv::reveal_all(), def_id, args)
} else {
Expand All @@ -888,7 +913,6 @@ fn visit_fn_use<'tcx>(
};
visit_instance_use(tcx, instance, is_direct_call, source, output);
}
skip_move_size_check
}

fn visit_instance_use<'tcx>(
Expand Down Expand Up @@ -1395,6 +1419,29 @@ fn assoc_fn_of_type<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, fn_ident: Ident) ->
return None;
}

fn build_skip_move_check_fns(tcx: TyCtxt<'_>) -> Vec<DefId> {
let mut skip_move_check_fns = vec![];
add_assoc_fn(
tcx,
tcx.lang_items().owned_box(),
Ident::from_str("new"),
&mut skip_move_check_fns,
);
add_assoc_fn(
tcx,
tcx.get_diagnostic_item(sym::Arc),
Ident::from_str("new"),
&mut skip_move_check_fns,
);
add_assoc_fn(
tcx,
tcx.get_diagnostic_item(sym::Rc),
Ident::from_str("new"),
&mut skip_move_check_fns,
);
skip_move_check_fns
}

/// Scans the MIR in order to find function calls, closures, and drop-glue.
#[instrument(skip(tcx, output), level = "debug")]
fn collect_used_items<'tcx>(
Expand All @@ -1404,28 +1451,6 @@ fn collect_used_items<'tcx>(
) {
let body = tcx.instance_mir(instance.def);

let mut skip_move_check_fns = vec![];
if tcx.move_size_limit().0 > 0 {
add_assoc_fn(
tcx,
tcx.lang_items().owned_box(),
Ident::from_str("new"),
&mut skip_move_check_fns,
);
add_assoc_fn(
tcx,
tcx.get_diagnostic_item(sym::Arc),
Ident::from_str("new"),
&mut skip_move_check_fns,
);
add_assoc_fn(
tcx,
tcx.get_diagnostic_item(sym::Rc),
Ident::from_str("new"),
&mut skip_move_check_fns,
);
}

// Here we rely on the visitor also visiting `required_consts`, so that we evaluate them
// and abort compilation if any of them errors.
MirUsedCollector {
Expand All @@ -1434,8 +1459,8 @@ fn collect_used_items<'tcx>(
output,
instance,
move_size_spans: vec![],
skip_move_size_check: false,
skip_move_check_fns,
visiting_call_terminator: false,
skip_move_check_fns: None,
}
.visit_body(&body);
}
Expand Down
Loading