From 0a2282e1286823c06ee9c4fa5e49544fc4f47771 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 23 Aug 2018 00:33:32 -0700 Subject: [PATCH] rustc: Continue to tweak "std internal symbols" In investigating [an issue][1] with `panic_implementation` defined in an executable that's optimized I once again got to rethinking a bit about the `rustc_std_internal_symbol` attribute as well as weak lang items. We've sort of been non-stop tweaking these items ever since their inception, and this continues to the trend. The crux of the bug was that in the reachability we have a [different branch][2] for non-library builds which meant that weak lang items (and std internal symbols) weren't considered reachable, causing them to get eliminiated by ThinLTO passes. The fix was to basically tweak that branch to consider these symbols to ensure that they're propagated all the way to the linker. Along the way I've attempted to erode the distinction between std internal symbols and weak lang items by having weak lang items automatically configure fields of `CodegenFnAttrs`. That way most code no longer even considers weak lang items and they're simply considered normal functions with attributes about the ABI. In the end this fixes the final comment of #51342 [1]: https://github.com/rust-lang/rust/issues/51342#issuecomment-414368019 [2]: https://github.com/rust-lang/rust/blob/35bf1ae25799a4e62131159f052e0a3cbd27c960/src/librustc/middle/reachable.rs#L225-L238 --- src/liballoc_jemalloc/lib.rs | 8 ++--- src/libpanic_abort/lib.rs | 4 +-- src/librustc/hir/mod.rs | 35 +++++++++++++++++++ src/librustc/ich/impls_hir.rs | 1 + src/librustc/middle/reachable.rs | 7 ++-- src/librustc_allocator/expand.rs | 8 +---- src/librustc_codegen_utils/symbol_names.rs | 15 +++----- src/librustc_mir/monomorphize/collector.rs | 1 - src/librustc_mir/monomorphize/partitioning.rs | 6 ++-- src/librustc_typeck/collect.rs | 23 ++++++++++++ src/libstd/alloc.rs | 8 ++--- .../wasm-symbols-not-imported/Makefile | 16 +++++++++ .../run-make/wasm-symbols-not-imported/foo.rs | 26 ++++++++++++++ .../verify-no-imports.js | 20 +++++++++++ .../ui/panic-handler/panic-handler-std.stderr | 8 ++++- 15 files changed, 151 insertions(+), 35 deletions(-) create mode 100644 src/test/run-make/wasm-symbols-not-imported/Makefile create mode 100644 src/test/run-make/wasm-symbols-not-imported/foo.rs create mode 100644 src/test/run-make/wasm-symbols-not-imported/verify-no-imports.js diff --git a/src/liballoc_jemalloc/lib.rs b/src/liballoc_jemalloc/lib.rs index 480a24b9bd1f2..bdf0e37a2e672 100644 --- a/src/liballoc_jemalloc/lib.rs +++ b/src/liballoc_jemalloc/lib.rs @@ -89,16 +89,16 @@ mod contents { // linkage directives are provided as part of the current compiler allocator // ABI - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rde_alloc(size: usize, align: usize) -> *mut u8 { let flags = align_to_flags(align, size); let ptr = mallocx(size as size_t, flags) as *mut u8; ptr } - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rde_dealloc(ptr: *mut u8, size: usize, align: usize) { @@ -106,8 +106,8 @@ mod contents { sdallocx(ptr as *mut c_void, size, flags); } - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rde_realloc(ptr: *mut u8, _old_size: usize, align: usize, @@ -117,8 +117,8 @@ mod contents { ptr } - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rde_alloc_zeroed(size: usize, align: usize) -> *mut u8 { let ptr = if align <= MIN_ALIGN && align <= size { calloc(size as size_t, 1) as *mut u8 diff --git a/src/libpanic_abort/lib.rs b/src/libpanic_abort/lib.rs index da568fae70e14..14221f3d79e84 100644 --- a/src/libpanic_abort/lib.rs +++ b/src/libpanic_abort/lib.rs @@ -31,7 +31,7 @@ // Rust's "try" function, but if we're aborting on panics we just call the // function as there's nothing else we need to do here. -#[no_mangle] +#[cfg_attr(stage0, no_mangle)] #[rustc_std_internal_symbol] pub unsafe extern fn __rust_maybe_catch_panic(f: fn(*mut u8), data: *mut u8, @@ -51,7 +51,7 @@ pub unsafe extern fn __rust_maybe_catch_panic(f: fn(*mut u8), // which would break compat with XP. For now just use `intrinsics::abort` which // will kill us with an illegal instruction, which will do a good enough job for // now hopefully. -#[no_mangle] +#[cfg_attr(stage0, no_mangle)] #[rustc_std_internal_symbol] pub unsafe extern fn __rust_start_panic(_payload: usize) -> u32 { abort(); diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index 6bdfbd40e8d36..8aa0f5e07c0ea 100644 --- a/src/librustc/hir/mod.rs +++ b/src/librustc/hir/mod.rs @@ -2287,25 +2287,59 @@ pub fn provide(providers: &mut Providers) { #[derive(Clone, RustcEncodable, RustcDecodable)] pub struct CodegenFnAttrs { pub flags: CodegenFnAttrFlags, + /// Parsed representation of the `#[inline]` attribute pub inline: InlineAttr, + /// The `#[export_name = "..."]` attribute, indicating a custom symbol a + /// function should be exported under pub export_name: Option, + /// The `#[link_name = "..."]` attribute, indicating a custom symbol an + /// imported function should be imported as. Note that `export_name` + /// probably isn't set when this is set, this is for foreign items while + /// `#[export_name]` is for Rust-defined functions. + pub link_name: Option, + /// The `#[target_feature(enable = "...")]` attribute and the enabled + /// features (only enabled features are supported right now). pub target_features: Vec, + /// The `#[linkage = "..."]` attribute and the value we found. pub linkage: Option, + /// The `#[link_section = "..."]` attribute, or what executable section this + /// should be placed in. pub link_section: Option, } bitflags! { #[derive(RustcEncodable, RustcDecodable)] pub struct CodegenFnAttrFlags: u32 { + /// #[cold], a hint to LLVM that this function, when called, is never on + /// the hot path const COLD = 1 << 0; + /// #[allocator], a hint to LLVM that the pointer returned from this + /// function is never null const ALLOCATOR = 1 << 1; + /// #[unwind], an indicator that this function may unwind despite what + /// its ABI signature may otherwise imply const UNWIND = 1 << 2; + /// #[rust_allocator_nounwind], an indicator that an imported FFI + /// function will never unwind. Probably obsolete by recent changes with + /// #[unwind], but hasn't been removed/migrated yet const RUSTC_ALLOCATOR_NOUNWIND = 1 << 3; + /// #[naked], indicates to LLVM that no function prologue/epilogue + /// should be generated const NAKED = 1 << 4; + /// #[no_mangle], the function's name should be the same as its symbol const NO_MANGLE = 1 << 5; + /// #[rustc_std_internal_symbol], and indicator that this symbol is a + /// "weird symbol" for the standard library in that it has slightly + /// different linkage, visibility, and reachability rules. const RUSTC_STD_INTERNAL_SYMBOL = 1 << 6; + /// #[no_debug], indicates that no debugging information should be + /// generated for this function by LLVM const NO_DEBUG = 1 << 7; + /// #[thread_local], indicates a static is actually a thread local + /// piece of memory const THREAD_LOCAL = 1 << 8; + /// #[used], indicates that LLVM can't eliminate this function (but the + /// linker can!) const USED = 1 << 9; } } @@ -2316,6 +2350,7 @@ impl CodegenFnAttrs { flags: CodegenFnAttrFlags::empty(), inline: InlineAttr::None, export_name: None, + link_name: None, target_features: vec![], linkage: None, link_section: None, diff --git a/src/librustc/ich/impls_hir.rs b/src/librustc/ich/impls_hir.rs index e82ef8bbdae55..2ac195dca82c4 100644 --- a/src/librustc/ich/impls_hir.rs +++ b/src/librustc/ich/impls_hir.rs @@ -1136,6 +1136,7 @@ impl_stable_hash_for!(struct hir::CodegenFnAttrs { flags, inline, export_name, + link_name, target_features, linkage, link_section, diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs index 0328b5f1fd5e5..ccfc84940f2df 100644 --- a/src/librustc/middle/reachable.rs +++ b/src/librustc/middle/reachable.rs @@ -231,8 +231,11 @@ impl<'a, 'tcx> ReachableContext<'a, 'tcx> { false }; let def_id = self.tcx.hir.local_def_id(item.id); - let is_extern = self.tcx.codegen_fn_attrs(def_id).contains_extern_indicator(); - if reachable || is_extern { + let codegen_attrs = self.tcx.codegen_fn_attrs(def_id); + let is_extern = codegen_attrs.contains_extern_indicator(); + let std_internal = codegen_attrs.flags.contains( + CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL); + if reachable || is_extern || std_internal { self.reachable_symbols.insert(search_item); } } diff --git a/src/librustc_allocator/expand.rs b/src/librustc_allocator/expand.rs index 5999416cecf15..11e8c30760b8a 100644 --- a/src/librustc_allocator/expand.rs +++ b/src/librustc_allocator/expand.rs @@ -236,15 +236,9 @@ impl<'a> AllocFnFactory<'a> { } fn attrs(&self) -> Vec { - let no_mangle = Symbol::intern("no_mangle"); - let no_mangle = self.cx.meta_word(self.span, no_mangle); - let special = Symbol::intern("rustc_std_internal_symbol"); let special = self.cx.meta_word(self.span, special); - vec![ - self.cx.attribute(self.span, no_mangle), - self.cx.attribute(self.span, special), - ] + vec![self.cx.attribute(self.span, special)] } fn arg_ty( diff --git a/src/librustc_codegen_utils/symbol_names.rs b/src/librustc_codegen_utils/symbol_names.rs index 33ce06217a46e..4f5e76c24e379 100644 --- a/src/librustc_codegen_utils/symbol_names.rs +++ b/src/librustc_codegen_utils/symbol_names.rs @@ -99,9 +99,9 @@ use rustc::hir::def_id::{DefId, LOCAL_CRATE}; use rustc::hir::map as hir_map; +use rustc::hir::CodegenFnAttrFlags; use rustc::hir::map::definitions::DefPathData; use rustc::ich::NodeIdHashingMode; -use rustc::middle::weak_lang_items; use rustc::ty::item_path::{self, ItemPathBuffer, RootMode}; use rustc::ty::query::Providers; use rustc::ty::subst::Substs; @@ -111,7 +111,6 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_mir::monomorphize::item::{InstantiationMode, MonoItem, MonoItemExt}; use rustc_mir::monomorphize::Instance; -use syntax::attr; use syntax_pos::symbol::Symbol; use std::fmt::Write; @@ -260,7 +259,6 @@ fn compute_symbol_name<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, instance: Instance } // FIXME(eddyb) Precompute a custom symbol name based on attributes. - let attrs = tcx.get_attrs(def_id); let is_foreign = if let Some(id) = node_id { match tcx.hir.get(id) { hir_map::NodeForeignItem(_) => true, @@ -270,24 +268,21 @@ fn compute_symbol_name<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, instance: Instance tcx.is_foreign_item(def_id) }; - if let Some(name) = weak_lang_items::link_name(&attrs) { - return name.to_string(); - } - + let attrs = tcx.codegen_fn_attrs(def_id); if is_foreign { - if let Some(name) = attr::first_attr_value_str_by_name(&attrs, "link_name") { + if let Some(name) = attrs.link_name { return name.to_string(); } // Don't mangle foreign items. return tcx.item_name(def_id).to_string(); } - if let Some(name) = tcx.codegen_fn_attrs(def_id).export_name { + if let Some(name) = &attrs.export_name { // Use provided name return name.to_string(); } - if attr::contains_name(&attrs, "no_mangle") { + if attrs.flags.contains(CodegenFnAttrFlags::NO_MANGLE) { // Don't mangle return tcx.item_name(def_id).to_string(); } diff --git a/src/librustc_mir/monomorphize/collector.rs b/src/librustc_mir/monomorphize/collector.rs index a1dbf9ddb0356..52ed17f86d9c0 100644 --- a/src/librustc_mir/monomorphize/collector.rs +++ b/src/librustc_mir/monomorphize/collector.rs @@ -1023,7 +1023,6 @@ impl<'b, 'a, 'v> RootCollector<'b, 'a, 'v> { MonoItemCollectionMode::Lazy => { self.entry_fn == Some(def_id) || self.tcx.is_reachable_non_generic(def_id) || - self.tcx.is_weak_lang_item(def_id) || self.tcx.codegen_fn_attrs(def_id).flags.contains( CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) } diff --git a/src/librustc_mir/monomorphize/partitioning.rs b/src/librustc_mir/monomorphize/partitioning.rs index 7d7be69b35559..c480fa4124665 100644 --- a/src/librustc_mir/monomorphize/partitioning.rs +++ b/src/librustc_mir/monomorphize/partitioning.rs @@ -516,10 +516,8 @@ fn mono_item_visibility( // visibility below. Like the weak lang items, though, we can't let // LLVM internalize them as this decision is left up to the linker to // omit them, so prevent them from being internalized. - let codegen_fn_attrs = tcx.codegen_fn_attrs(def_id); - let std_internal_symbol = codegen_fn_attrs.flags - .contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL); - if tcx.is_weak_lang_item(def_id) || std_internal_symbol { + let attrs = tcx.codegen_fn_attrs(def_id); + if attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) { *can_be_internalized = false; } diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index e404eb4ecca49..79061c250f7d6 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -29,6 +29,7 @@ use constrained_type_params as ctp; use lint; use middle::lang_items::SizedTraitLangItem; use middle::resolve_lifetime as rl; +use middle::weak_lang_items; use rustc::mir::mono::Linkage; use rustc::ty::query::Providers; use rustc::ty::subst::Substs; @@ -2281,6 +2282,8 @@ fn codegen_fn_attrs<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, id: DefId) -> Codegen codegen_fn_attrs.link_section = Some(val); } } + } else if attr.check_name("link_name") { + codegen_fn_attrs.link_name = attr.value_str(); } } @@ -2300,5 +2303,25 @@ fn codegen_fn_attrs<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, id: DefId) -> Codegen } } + // Weak lang items have the same semantics as "std internal" symbols in the + // sense that they're preserved through all our LTO passes and only + // strippable by the linker. + // + // Additionally weak lang items have predetermined symbol names. + if tcx.is_weak_lang_item(id) { + codegen_fn_attrs.flags |= CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL; + } + if let Some(name) = weak_lang_items::link_name(&attrs) { + codegen_fn_attrs.export_name = Some(name); + codegen_fn_attrs.link_name = Some(name); + } + + // Internal symbols to the standard library all have no_mangle semantics in + // that they have defined symbol names present in the function name. This + // also applies to weak symbols where they all have known symbol names. + if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) { + codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_MANGLE; + } + codegen_fn_attrs } diff --git a/src/libstd/alloc.rs b/src/libstd/alloc.rs index b9aba1e9cab32..6753ed4a3df6d 100644 --- a/src/libstd/alloc.rs +++ b/src/libstd/alloc.rs @@ -150,23 +150,23 @@ pub mod __default_lib_allocator { // linkage directives are provided as part of the current compiler allocator // ABI - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rdl_alloc(size: usize, align: usize) -> *mut u8 { let layout = Layout::from_size_align_unchecked(size, align); System.alloc(layout) } - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rdl_dealloc(ptr: *mut u8, size: usize, align: usize) { System.dealloc(ptr, Layout::from_size_align_unchecked(size, align)) } - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rdl_realloc(ptr: *mut u8, old_size: usize, align: usize, @@ -175,8 +175,8 @@ pub mod __default_lib_allocator { System.realloc(ptr, old_layout, new_size) } - #[no_mangle] #[rustc_std_internal_symbol] + #[cfg_attr(stage0, no_mangle)] pub unsafe extern fn __rdl_alloc_zeroed(size: usize, align: usize) -> *mut u8 { let layout = Layout::from_size_align_unchecked(size, align); System.alloc_zeroed(layout) diff --git a/src/test/run-make/wasm-symbols-not-imported/Makefile b/src/test/run-make/wasm-symbols-not-imported/Makefile new file mode 100644 index 0000000000000..773e32a031588 --- /dev/null +++ b/src/test/run-make/wasm-symbols-not-imported/Makefile @@ -0,0 +1,16 @@ +-include ../../run-make-fulldeps/tools.mk + +ifeq ($(TARGET),wasm32-unknown-unknown) +all: + $(RUSTC) foo.rs --target wasm32-unknown-unknown + $(NODE) verify-no-imports.js $(TMPDIR)/foo.wasm + $(RUSTC) foo.rs --target wasm32-unknown-unknown -C lto + $(NODE) verify-no-imports.js $(TMPDIR)/foo.wasm + $(RUSTC) foo.rs --target wasm32-unknown-unknown -O + $(NODE) verify-no-imports.js $(TMPDIR)/foo.wasm + $(RUSTC) foo.rs --target wasm32-unknown-unknown -O -C lto + $(NODE) verify-no-imports.js $(TMPDIR)/foo.wasm +else +all: +endif + diff --git a/src/test/run-make/wasm-symbols-not-imported/foo.rs b/src/test/run-make/wasm-symbols-not-imported/foo.rs new file mode 100644 index 0000000000000..156db486a4767 --- /dev/null +++ b/src/test/run-make/wasm-symbols-not-imported/foo.rs @@ -0,0 +1,26 @@ +// Copyright 2018 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. + +#![crate_type = "cdylib"] + +#![feature(panic_implementation)] +#![no_std] + +use core::panic::PanicInfo; + +#[no_mangle] +pub extern fn foo() { + panic!() +} + +#[panic_implementation] +fn panic(_info: &PanicInfo) -> ! { + loop {} +} diff --git a/src/test/run-make/wasm-symbols-not-imported/verify-no-imports.js b/src/test/run-make/wasm-symbols-not-imported/verify-no-imports.js new file mode 100644 index 0000000000000..28a98b5f2c25c --- /dev/null +++ b/src/test/run-make/wasm-symbols-not-imported/verify-no-imports.js @@ -0,0 +1,20 @@ +// Copyright 2018 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. + +const fs = require('fs'); +const process = require('process'); +const assert = require('assert'); +const buffer = fs.readFileSync(process.argv[2]); + +let m = new WebAssembly.Module(buffer); +let list = WebAssembly.Module.imports(m); +console.log('imports', list); +if (list.length !== 0) + throw new Error("there are some imports"); diff --git a/src/test/ui/panic-handler/panic-handler-std.stderr b/src/test/ui/panic-handler/panic-handler-std.stderr index c34a993e2c5e3..b141a2171646c 100644 --- a/src/test/ui/panic-handler/panic-handler-std.stderr +++ b/src/test/ui/panic-handler/panic-handler-std.stderr @@ -8,6 +8,12 @@ LL | | } | = note: first defined in crate `std`. -error: aborting due to previous error +error: argument should be `&PanicInfo` + --> $DIR/panic-handler-std.rs:18:16 + | +LL | fn panic(info: PanicInfo) -> ! { + | ^^^^^^^^^ + +error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0152`.