From e9696c8b62c45903bed1bb39782abe43e392cd21 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 26 Mar 2021 13:39:04 +0000 Subject: [PATCH] Implement a lint that highlights all moves larger than 1000 bytes --- compiler/rustc_lint_defs/src/builtin.rs | 34 +++++++++++++++++ compiler/rustc_middle/src/mir/mod.rs | 22 ++++++++++- .../rustc_mir/src/monomorphize/collector.rs | 37 ++++++++++++++++++ .../rustc_mir/src/transform/const_prop.rs | 15 +------- src/test/ui/async-await/large_moves.rs | 21 ++++++++++ src/test/ui/async-await/large_moves.stderr | 38 +++++++++++++++++++ 6 files changed, 153 insertions(+), 14 deletions(-) create mode 100644 src/test/ui/async-await/large_moves.rs create mode 100644 src/test/ui/async-await/large_moves.stderr diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index f15a7cc5ec2c..e29005c0c897 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -2877,6 +2877,39 @@ declare_lint! { }; } +declare_lint! { + /// The `large_assigments` lint detects when objects of large + /// types are being moved around. + /// + /// ### Example + /// + /// ```rust,ignore (can crash on some platforms) + /// let x = [0; 50000]; + /// let y = x; + /// ``` + /// + /// produces: + /// + /// ```text + /// warning: moving a large value + /// --> $DIR/move-large.rs:1:3 + /// let y = x; + /// - Copied large value here + /// ``` + /// + /// ### Explanation + /// + /// When using a large type in a plain assignment or in a function + /// argument, idiomatic code can be inefficient. + /// Ideally appropriate optimizations would resolve this, but such + /// optimizations are only done in a best-effort manner. + /// This lint will trigger on all sites of large moves and thus allow the + /// user to resolve them in code. + pub LARGE_ASSIGNMENTS, + Allow, + "detects large moves or copies", +} + declare_lint_pass! { /// Does nothing as a lint pass, but registers some `Lint`s /// that are used by other parts of the compiler. @@ -2962,6 +2995,7 @@ declare_lint_pass! { LEGACY_DERIVE_HELPERS, PROC_MACRO_BACK_COMPAT, OR_PATTERNS_BACK_COMPAT, + LARGE_ASSIGNMENTS, ] } diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 998868211401..1ebb3dbe7c63 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -12,10 +12,10 @@ use crate::ty::print::{FmtPrinter, Printer}; use crate::ty::subst::{Subst, SubstsRef}; use crate::ty::{self, List, Ty, TyCtxt}; use crate::ty::{AdtDef, InstanceDef, Region, ScalarInt, UserTypeAnnotationIndex}; -use rustc_hir as hir; use rustc_hir::def::{CtorKind, Namespace}; use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX}; use rustc_hir::{self, GeneratorKind}; +use rustc_hir::{self as hir, HirId}; use rustc_target::abi::{Size, VariantIdx}; use polonius_engine::Atom; @@ -1948,6 +1948,26 @@ rustc_index::newtype_index! { } } +impl SourceScope { + /// Finds the original HirId this MIR item came from. + /// This is necessary after MIR optimizations, as otherwise we get a HirId + /// from the function that was inlined instead of the function call site. + pub fn lint_root(self, source_scopes: &IndexVec>) -> Option { + let mut data = &source_scopes[self]; + // FIXME(oli-obk): we should be able to just walk the `inlined_parent_scope`, but it + // does not work as I thought it would. Needs more investigation and documentation. + while data.inlined.is_some() { + trace!(?data); + data = &source_scopes[data.parent_scope.unwrap()]; + } + trace!(?data); + match &data.local_data { + ClearCrossCrate::Set(data) => Some(data.lint_root), + ClearCrossCrate::Clear => None, + } + } +} + #[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable, TypeFoldable)] pub struct SourceScopeData<'tcx> { pub span: Span, diff --git a/compiler/rustc_mir/src/monomorphize/collector.rs b/compiler/rustc_mir/src/monomorphize/collector.rs index 1fda71d74bbf..8ff3edac65fb 100644 --- a/compiler/rustc_mir/src/monomorphize/collector.rs +++ b/compiler/rustc_mir/src/monomorphize/collector.rs @@ -198,7 +198,9 @@ use rustc_middle::ty::subst::{GenericArgKind, InternalSubsts}; use rustc_middle::ty::{self, GenericParamDefKind, Instance, Ty, TyCtxt, TypeFoldable}; use rustc_middle::{middle::codegen_fn_attrs::CodegenFnAttrFlags, mir::visit::TyContext}; use rustc_session::config::EntryFnType; +use rustc_session::lint::builtin::LARGE_ASSIGNMENTS; use rustc_span::source_map::{dummy_spanned, respan, Span, Spanned, DUMMY_SP}; +use rustc_target::abi::Size; use smallvec::SmallVec; use std::iter; use std::ops::Range; @@ -753,6 +755,41 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { self.super_terminator(terminator, location); } + fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, location: Location) { + self.super_operand(operand, location); + let ty = operand.ty(self.body, self.tcx); + let ty = self.monomorphize(ty); + let layout = self.tcx.layout_of(ty::ParamEnv::reveal_all().and(ty)); + if let Ok(layout) = layout { + if layout.size > Size::from_bytes(1000) { + debug!(?layout); + let source_info = self.body.source_info(location); + debug!(?source_info); + let lint_root = source_info.scope.lint_root(&self.body.source_scopes); + debug!(?lint_root); + let lint_root = match lint_root { + Some(lint_root) => lint_root, + // This happens when the issue is in a function from a foreign crate that + // we monomorphized in the current crate. We can't get a `HirId` for things + // in other crates. + // FIXME: Find out where to report the lint on. Maybe simply crate-level lint root + // but correct span? This would make the lint at least accept crate-level lint attributes. + None => return, + }; + self.tcx.struct_span_lint_hir( + LARGE_ASSIGNMENTS, + lint_root, + source_info.span, + |lint| { + let mut err = lint.build(&format!("moving {} bytes", layout.size.bytes())); + err.span_label(source_info.span, "value moved from here"); + err.emit() + }, + ); + } + } + } + fn visit_local( &mut self, _place_local: &Local, diff --git a/compiler/rustc_mir/src/transform/const_prop.rs b/compiler/rustc_mir/src/transform/const_prop.rs index 7706316c9651..0d48ff6530e5 100644 --- a/compiler/rustc_mir/src/transform/const_prop.rs +++ b/compiler/rustc_mir/src/transform/const_prop.rs @@ -13,7 +13,7 @@ use rustc_middle::mir::visit::{ MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor, }; use rustc_middle::mir::{ - AssertKind, BasicBlock, BinOp, Body, ClearCrossCrate, Constant, ConstantKind, Local, LocalDecl, + AssertKind, BasicBlock, BinOp, Body, Constant, ConstantKind, Local, LocalDecl, LocalKind, Location, Operand, Place, Rvalue, SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind, UnOp, RETURN_PLACE, }; @@ -440,18 +440,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn lint_root(&self, source_info: SourceInfo) -> Option { - let mut data = &self.source_scopes[source_info.scope]; - // FIXME(oli-obk): we should be able to just walk the `inlined_parent_scope`, but it - // does not work as I thought it would. Needs more investigation and documentation. - while data.inlined.is_some() { - trace!(?data); - data = &self.source_scopes[data.parent_scope.unwrap()]; - } - trace!(?data); - match &data.local_data { - ClearCrossCrate::Set(data) => Some(data.lint_root), - ClearCrossCrate::Clear => None, - } + source_info.scope.lint_root(&self.source_scopes) } fn use_ecx(&mut self, f: F) -> Option diff --git a/src/test/ui/async-await/large_moves.rs b/src/test/ui/async-await/large_moves.rs new file mode 100644 index 000000000000..7e6819d6d139 --- /dev/null +++ b/src/test/ui/async-await/large_moves.rs @@ -0,0 +1,21 @@ +#![deny(large_assignments)] +// build-fail + +// edition:2018 + +fn main() { + let x = async { //~ ERROR large_assignments + let y = [0; 9999]; + dbg!(y); + thing(&y).await; + dbg!(y); + }; + let z = (x, 42); //~ ERROR large_assignments + //~^ ERROR large_assignments + let a = z.0; //~ ERROR large_assignments + let b = z.1; +} + +async fn thing(y: &[u8]) { + dbg!(y); +} \ No newline at end of file diff --git a/src/test/ui/async-await/large_moves.stderr b/src/test/ui/async-await/large_moves.stderr new file mode 100644 index 000000000000..d395d2ae8696 --- /dev/null +++ b/src/test/ui/async-await/large_moves.stderr @@ -0,0 +1,38 @@ +error: moving 10024 bytes + --> $DIR/large_moves.rs:7:13 + | +LL | let x = async { + | _____________^ +LL | | let y = [0; 9999]; +LL | | dbg!(y); +LL | | thing(&y).await; +LL | | dbg!(y); +LL | | }; + | |_____^ value moved from here + | +note: the lint level is defined here + --> $DIR/large_moves.rs:1:9 + | +LL | #![deny(large_assignments)] + | ^^^^^^^^^^^^^^^^^ + +error: moving 10024 bytes + --> $DIR/large_moves.rs:13:14 + | +LL | let z = (x, 42); + | ^ value moved from here + +error: moving 10024 bytes + --> $DIR/large_moves.rs:13:13 + | +LL | let z = (x, 42); + | ^^^^^^^ value moved from here + +error: moving 10024 bytes + --> $DIR/large_moves.rs:15:13 + | +LL | let a = z.0; + | ^^^ value moved from here + +error: aborting due to 4 previous errors +