From f5d29a3b594719d0303bdffccb8470ebb10b0d3e Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 2 Jun 2016 23:43:16 +0200 Subject: [PATCH] Move variant_size_differences out of trans Also enhances the error message a bit, fixes #30505 on the way, and adds a test (which was missing). Closes #34018 --- src/librustc/lint/builtin.rs | 7 - src/librustc/lint/context.rs | 36 +--- src/librustc/lint/mod.rs | 2 +- src/librustc/session/config.rs | 2 - src/librustc/session/mod.rs | 3 - src/librustc_lint/lib.rs | 4 +- src/librustc_lint/types.rs | 68 +++++++ src/librustc_trans/base.rs | 184 +----------------- src/librustc_trans/context.rs | 8 - src/librustc_trans/trans_item.rs | 32 +++ .../compile-fail/variant-size-differences.rs | 18 ++ 11 files changed, 123 insertions(+), 241 deletions(-) create mode 100644 src/test/compile-fail/variant-size-differences.rs diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index 41086b5d1c9..3230a08c276 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -94,12 +94,6 @@ declare_lint! { "unknown crate type found in #[crate_type] directive" } -declare_lint! { - pub VARIANT_SIZE_DIFFERENCES, - Allow, - "detects enums with widely varying variant sizes" -} - declare_lint! { pub FAT_PTR_TRANSMUTES, Allow, @@ -230,7 +224,6 @@ impl LintPass for HardwiredLints { UNUSED_FEATURES, STABLE_FEATURES, UNKNOWN_CRATE_TYPES, - VARIANT_SIZE_DIFFERENCES, FAT_PTR_TRANSMUTES, TRIVIAL_CASTS, TRIVIAL_NUMERIC_CASTS, diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 01e14ad71b3..8d032fd98ba 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -29,8 +29,8 @@ use dep_graph::DepNode; use middle::privacy::AccessLevels; use ty::TyCtxt; use session::{config, early_error, Session}; -use lint::{Level, LevelSource, Lint, LintId, LintArray, LintPass}; -use lint::{EarlyLintPassObject, LateLintPass, LateLintPassObject}; +use lint::{Level, LevelSource, Lint, LintId, LintPass}; +use lint::{EarlyLintPassObject, LateLintPassObject}; use lint::{Default, CommandLine, Node, Allow, Warn, Deny, Forbid}; use lint::builtin; use util::nodemap::FnvHashMap; @@ -1064,38 +1064,6 @@ impl<'a, 'tcx> IdVisitingOperation for LateContext<'a, 'tcx> { } } -// This lint pass is defined here because it touches parts of the `LateContext` -// that we don't want to expose. It records the lint level at certain AST -// nodes, so that the variant size difference check in trans can call -// `raw_emit_lint`. - -pub struct GatherNodeLevels; - -impl LintPass for GatherNodeLevels { - fn get_lints(&self) -> LintArray { - lint_array!() - } -} - -impl LateLintPass for GatherNodeLevels { - fn check_item(&mut self, cx: &LateContext, it: &hir::Item) { - match it.node { - hir::ItemEnum(..) => { - let lint_id = LintId::of(builtin::VARIANT_SIZE_DIFFERENCES); - let lvlsrc = cx.lints.get_level_source(lint_id); - match lvlsrc { - (lvl, _) if lvl != Allow => { - cx.node_levels.borrow_mut() - .insert((it.id, lint_id), lvlsrc); - }, - _ => { } - } - }, - _ => { } - } - } -} - enum CheckLintNameResult { Ok, // Lint doesn't exist diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 92aa446c265..121033549c0 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -41,7 +41,7 @@ use hir; pub use lint::context::{LateContext, EarlyContext, LintContext, LintStore, raw_emit_lint, check_crate, check_ast_crate, gather_attrs, - raw_struct_lint, GatherNodeLevels, FutureIncompatibleInfo}; + raw_struct_lint, FutureIncompatibleInfo}; /// Specification of a single lint. #[derive(Copy, Clone, Debug)] diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 5ccc96210be..ab9a0fcb19b 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -727,8 +727,6 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "load extra plugins"), unstable_options: bool = (false, parse_bool, "adds unstable command line options to rustc interface"), - print_enum_sizes: bool = (false, parse_bool, - "print the size of enums and their variants"), force_overflow_checks: Option = (None, parse_opt_bool, "force overflow checks on or off"), force_dropflag_checks: Option = (None, parse_opt_bool, diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs index 57c4af6bed5..0e516bdc211 100644 --- a/src/librustc/session/mod.rs +++ b/src/librustc/session/mod.rs @@ -304,9 +304,6 @@ impl Session { pub fn unstable_options(&self) -> bool { self.opts.debugging_opts.unstable_options } - pub fn print_enum_sizes(&self) -> bool { - self.opts.debugging_opts.print_enum_sizes - } pub fn nonzeroing_move_hints(&self) -> bool { self.opts.debugging_opts.enable_nonzeroing_move_hints } diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs index 4ae5b3afdba..7b0ee91b69e 100644 --- a/src/librustc_lint/lib.rs +++ b/src/librustc_lint/lib.rs @@ -108,6 +108,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { HardwiredLints, WhileTrue, ImproperCTypes, + VariantSizeDifferences, BoxPointers, UnusedAttributes, PathStatements, @@ -209,9 +210,6 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { }, ]); - // We have one lint pass defined specially - store.register_late_pass(sess, false, box lint::GatherNodeLevels); - // Register renamed and removed lints store.register_renamed("unknown_features", "unused_features"); store.register_removed("unsigned_negation", "replaced by negate_unsigned feature gate"); diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 97f97a889ed..8b44157a405 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -13,6 +13,8 @@ use rustc::hir::def_id::DefId; use rustc::ty::subst::Substs; use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::layout::{Layout, Primitive}; +use rustc::traits::ProjectionMode; use middle::const_val::ConstVal; use rustc_const_eval::eval_const_expr_partial; use rustc_const_eval::EvalHint::ExprTypeChecked; @@ -77,6 +79,12 @@ declare_lint! { "shift exceeds the type's number of bits" } +declare_lint! { + VARIANT_SIZE_DIFFERENCES, + Allow, + "detects enums with widely varying variant sizes" +} + #[derive(Copy, Clone)] pub struct TypeLimits { /// Id of the last visited negated expression @@ -676,3 +684,63 @@ impl LateLintPass for ImproperCTypes { } } } + +pub struct VariantSizeDifferences; + +impl LintPass for VariantSizeDifferences { + fn get_lints(&self) -> LintArray { + lint_array!(VARIANT_SIZE_DIFFERENCES) + } +} + +impl LateLintPass for VariantSizeDifferences { + fn check_item(&mut self, cx: &LateContext, it: &hir::Item) { + if let hir::ItemEnum(ref enum_definition, ref gens) = it.node { + if gens.ty_params.is_empty() { // sizes only make sense for non-generic types + let mut sizes = vec![]; + let t = cx.tcx.node_id_to_type(it.id); + let layout = cx.tcx.normalizing_infer_ctxt(ProjectionMode::Any).enter(|infcx| { + t.layout(&infcx).unwrap_or_else(|e| { + bug!("failed to get layout for `{}`: {}", t, e) + }) + }); + + if let Layout::General { ref variants, ref size, discr, .. } = *layout { + let discr_size = Primitive::Int(discr).size(&cx.tcx.data_layout).bytes(); + + debug!("enum `{}` is {} bytes large", t, size.bytes()); + + for (variant, variant_layout) in enum_definition.variants.iter().zip(variants) { + // Subtract the size of the enum discriminant + let bytes = variant_layout.min_size().bytes().saturating_sub(discr_size); + sizes.push(bytes); + + debug!("- variant `{}` is {} bytes large", variant.node.name, bytes); + } + + let (largest, slargest, largest_index) = sizes.iter() + .enumerate() + .fold((0, 0, 0), + |(l, s, li), (idx, &size)| + if size > l { + (size, l, idx) + } else if size > s { + (l, size, li) + } else { + (l, s, li) + } + ); + + // we only warn if the largest variant is at least thrice as large as + // the second-largest. + if largest > slargest * 3 && slargest > 0 { + cx.span_lint(VARIANT_SIZE_DIFFERENCES, + enum_definition.variants[largest_index].span, + &format!("enum variant is more than three times larger \ + ({} bytes) than the next largest", largest)); + } + } + } + } + } +} diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index c080d1f06d0..ee3eeefc124 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -30,7 +30,6 @@ use super::ModuleTranslation; use back::link; use back::linker::LinkerInfo; -use lint; use llvm::{BasicBlockRef, Linkage, ValueRef, Vector, get_param}; use llvm; use rustc::cfg; @@ -75,7 +74,7 @@ use expr; use glue; use inline; use machine; -use machine::{llalign_of_min, llsize_of, llsize_of_real}; +use machine::{llalign_of_min, llsize_of}; use meth; use mir; use monomorphize::{self, Instance}; @@ -86,7 +85,6 @@ use trans_item::TransItem; use tvec; use type_::Type; use type_of; -use type_of::*; use value::Value; use Disr; use util::common::indenter; @@ -2074,87 +2072,6 @@ pub fn trans_ctor_shim<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, fcx.finish(bcx, DebugLoc::None); } -fn enum_variant_size_lint(ccx: &CrateContext, enum_def: &hir::EnumDef, sp: Span, id: ast::NodeId) { - let mut sizes = Vec::new(); // does no allocation if no pushes, thankfully - - let print_info = ccx.sess().print_enum_sizes(); - - let levels = ccx.tcx().node_lint_levels.borrow(); - let lint_id = lint::LintId::of(lint::builtin::VARIANT_SIZE_DIFFERENCES); - let lvlsrc = levels.get(&(id, lint_id)); - let is_allow = lvlsrc.map_or(true, |&(lvl, _)| lvl == lint::Allow); - - if is_allow && !print_info { - // we're not interested in anything here - return; - } - - let ty = ccx.tcx().node_id_to_type(id); - let avar = adt::represent_type(ccx, ty); - match *avar { - adt::General(_, ref variants, _) => { - for var in variants { - let mut size = 0; - for field in var.fields.iter().skip(1) { - // skip the discriminant - size += llsize_of_real(ccx, sizing_type_of(ccx, *field)); - } - sizes.push(size); - } - }, - _ => { /* its size is either constant or unimportant */ } - } - - let (largest, slargest, largest_index) = sizes.iter().enumerate().fold((0, 0, 0), - |(l, s, li), (idx, &size)| - if size > l { - (size, l, idx) - } else if size > s { - (l, size, li) - } else { - (l, s, li) - } - ); - - // FIXME(#30505) Should use logging for this. - if print_info { - let llty = type_of::sizing_type_of(ccx, ty); - - let sess = &ccx.tcx().sess; - sess.span_note_without_error(sp, - &format!("total size: {} bytes", llsize_of_real(ccx, llty))); - match *avar { - adt::General(..) => { - for (i, var) in enum_def.variants.iter().enumerate() { - ccx.tcx() - .sess - .span_note_without_error(var.span, - &format!("variant data: {} bytes", sizes[i])); - } - } - _ => {} - } - } - - // we only warn if the largest variant is at least thrice as large as - // the second-largest. - if !is_allow && largest > slargest * 3 && slargest > 0 { - // Use lint::raw_emit_lint rather than sess.add_lint because the lint-printing - // pass for the latter already ran. - lint::raw_struct_lint(&ccx.tcx().sess, - &ccx.tcx().sess.lint_store.borrow(), - lint::builtin::VARIANT_SIZE_DIFFERENCES, - *lvlsrc.unwrap(), - Some(sp), - &format!("enum variant is more than three times larger ({} bytes) \ - than the next largest (ignoring padding)", - largest)) - .span_note(enum_def.variants[largest_index].span, - "this variant is the largest") - .emit(); - } -} - pub fn llvm_linkage_by_name(name: &str) -> Option { // Use the names from src/llvm/docs/LangRef.rst here. Most types are only // applicable to variable declarations and may not really make sense for @@ -2194,26 +2111,6 @@ pub fn set_link_section(ccx: &CrateContext, } } -fn trans_item(ccx: &CrateContext, item: &hir::Item) { - let _icx = push_ctxt("trans_item"); - - match item.node { - hir::ItemEnum(ref enum_definition, ref gens) => { - if gens.ty_params.is_empty() { - // sizes only make sense for non-generic types - enum_variant_size_lint(ccx, enum_definition, item.span, item.id); - } - } - hir::ItemFn(..) | - hir::ItemImpl(..) | - hir::ItemStatic(..) => { - // Don't do anything here. Translation has been moved to - // being "collector-driven". - } - _ => {} - } -} - /// Create the `main` function which will initialise the rust runtime and call /// users’ main function. pub fn maybe_create_entry_wrapper(ccx: &CrateContext) { @@ -2659,19 +2556,6 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, symbol_names_test::report_symbol_names(&shared_ccx); - { - let ccx = crate_context_list.get_ccx(0); - - // FIXME: #34018 - // At this point, we only walk the HIR for running - // enum_variant_size_lint(). This should arguably be moved somewhere - // else. - { - intravisit::walk_mod(&mut TransItemsWithinModVisitor { ccx: &ccx }, &krate.module); - krate.visit_all_items(&mut TransModVisitor { ccx: &ccx }); - } - } - if shared_ccx.sess().trans_stats() { let stats = shared_ccx.stats(); println!("--- trans stats ---"); @@ -2758,72 +2642,6 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, } } -/// We visit all the items in the krate and translate them. We do -/// this in two walks. The first walk just finds module items. It then -/// walks the full contents of those module items and translates all -/// the items within. Note that this entire process is O(n). The -/// reason for this two phased walk is that each module is -/// (potentially) placed into a distinct codegen-unit. This walk also -/// ensures that the immediate contents of each module is processed -/// entirely before we proceed to find more modules, helping to ensure -/// an equitable distribution amongst codegen-units. -pub struct TransModVisitor<'a, 'tcx: 'a> { - pub ccx: &'a CrateContext<'a, 'tcx>, -} - -impl<'a, 'tcx, 'v> Visitor<'v> for TransModVisitor<'a, 'tcx> { - fn visit_item(&mut self, i: &hir::Item) { - match i.node { - hir::ItemMod(_) => { - let item_ccx = self.ccx.rotate(); - intravisit::walk_item(&mut TransItemsWithinModVisitor { ccx: &item_ccx }, i); - } - _ => { } - } - } -} - -/// Translates all the items within a given module. Expects owner to -/// invoke `walk_item` on a module item. Ignores nested modules. -pub struct TransItemsWithinModVisitor<'a, 'tcx: 'a> { - pub ccx: &'a CrateContext<'a, 'tcx>, -} - -impl<'a, 'tcx, 'v> Visitor<'v> for TransItemsWithinModVisitor<'a, 'tcx> { - fn visit_nested_item(&mut self, item_id: hir::ItemId) { - self.visit_item(self.ccx.tcx().map.expect_item(item_id.id)); - } - - fn visit_item(&mut self, i: &hir::Item) { - match i.node { - hir::ItemMod(..) => { - // skip modules, they will be uncovered by the TransModVisitor - } - _ => { - let def_id = self.ccx.tcx().map.local_def_id(i.id); - let tcx = self.ccx.tcx(); - - // Create a subtask for trans'ing a particular item. We are - // giving `trans_item` access to this item, so also record a read. - tcx.dep_graph.with_task(DepNode::TransCrateItem(def_id), || { - tcx.dep_graph.read(DepNode::Hir(def_id)); - - // We are going to be accessing various tables - // generated by TypeckItemBody; we also assume - // that the body passes type check. These tables - // are not individually tracked, so just register - // a read here. - tcx.dep_graph.read(DepNode::TypeckItemBody(def_id)); - - trans_item(self.ccx, i); - }); - - intravisit::walk_item(self, i); - } - } - } -} - fn collect_and_partition_translation_items<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>) -> (Vec>, SymbolMap<'tcx>) { let time_passes = scx.sess().time_passes(); diff --git a/src/librustc_trans/context.rs b/src/librustc_trans/context.rs index b8d231db40a..88903726d64 100644 --- a/src/librustc_trans/context.rs +++ b/src/librustc_trans/context.rs @@ -219,14 +219,6 @@ impl<'a, 'tcx: 'a> CrateContextList<'a, 'tcx> { } } - pub fn get_ccx<'b>(&'b self, index: usize) -> CrateContext<'b, 'tcx> { - CrateContext { - shared: self.shared, - index: index, - local_ccxs: &self.local_ccxs[..], - } - } - pub fn shared(&self) -> &'a SharedCrateContext<'a, 'tcx> { self.shared } diff --git a/src/librustc_trans/trans_item.rs b/src/librustc_trans/trans_item.rs index b7b18b2631b..aa0c8ba5528 100644 --- a/src/librustc_trans/trans_item.rs +++ b/src/librustc_trans/trans_item.rs @@ -28,6 +28,7 @@ use rustc::hir::map as hir_map; use rustc::hir::def_id::DefId; use rustc::ty::{self, Ty, TyCtxt, TypeFoldable}; use rustc::ty::subst; +use rustc::dep_graph::DepNode; use std::hash::{Hash, Hasher}; use syntax::ast::{self, NodeId}; use syntax::{attr,errors}; @@ -73,6 +74,8 @@ impl<'a, 'tcx> TransItem<'tcx> { self.to_raw_string(), ccx.codegen_unit().name); + self.register_reads(ccx); + match *self { TransItem::Static(node_id) => { let item = ccx.tcx().map.expect_item(node_id); @@ -99,6 +102,35 @@ impl<'a, 'tcx> TransItem<'tcx> { ccx.codegen_unit().name); } + /// If necessary, creates a subtask for trans'ing a particular item and registers reads on + /// `TypeckItemBody` and `Hir`. + fn register_reads(&self, ccx: &CrateContext<'a, 'tcx>) { + let tcx = ccx.tcx(); + let def_id = match *self { + TransItem::Static(node_id) => { + tcx.map.local_def_id(node_id) + } + TransItem::Fn(instance) => { + instance.def + } + TransItem::DropGlue(_) => { + // Nothing to track for drop glue + return; + } + }; + + tcx.dep_graph.with_task(DepNode::TransCrateItem(def_id), || { + tcx.dep_graph.read(DepNode::Hir(def_id)); + + // We are going to be accessing various tables + // generated by TypeckItemBody; we also assume + // that the body passes type check. These tables + // are not individually tracked, so just register + // a read here. + tcx.dep_graph.read(DepNode::TypeckItemBody(def_id)); + }); + } + pub fn predefine(&self, ccx: &CrateContext<'a, 'tcx>, linkage: llvm::Linkage) { diff --git a/src/test/compile-fail/variant-size-differences.rs b/src/test/compile-fail/variant-size-differences.rs new file mode 100644 index 00000000000..f2cffeefe90 --- /dev/null +++ b/src/test/compile-fail/variant-size-differences.rs @@ -0,0 +1,18 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![deny(variant_size_differences)] + +enum _En { + V0(u8), + VBig([u8; 1024]), //~ ERROR variant is more than three times larger +} + +fn main() {}