From db16909ae341f7e2de29e0e458d945fd0cdec8aa Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Fri, 12 Aug 2016 21:15:42 -0500 Subject: [PATCH 1/3] exclude `#![no_builtins]` crates from LTO this prevents intrinsics like `memcpy` from being mis-optimized to infinite recursive calls when LTO is used. fixes #31544 closes #35540 --- src/librustc_trans/back/link.rs | 28 ++++++++++++++++++---------- src/librustc_trans/back/lto.rs | 11 ++++++++++- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/librustc_trans/back/link.rs b/src/librustc_trans/back/link.rs index a9f3d2f8a1754..d17da74c87f39 100644 --- a/src/librustc_trans/back/link.rs +++ b/src/librustc_trans/back/link.rs @@ -42,7 +42,7 @@ use std::process::Command; use std::str; use flate; use syntax::ast; -use syntax::attr::AttrMetaMethods; +use syntax::attr::{self, AttrMetaMethods}; use syntax_pos::Span; // RLIB LLVM-BYTECODE OBJECT LAYOUT @@ -938,8 +938,10 @@ fn add_upstream_rust_crates(cmd: &mut Linker, Linkage::NotLinked | Linkage::IncludedFromDylib => {} Linkage::Static => { + let is_a_no_builtins_crate = + attr::contains_name(&sess.cstore.crate_attrs(cnum), "no_builtins"); add_static_crate(cmd, sess, tmpdir, crate_type, - &src.rlib.unwrap().0) + &src.rlib.unwrap().0, is_a_no_builtins_crate) } Linkage::Dynamic => { add_dynamic_crate(cmd, sess, &src.dylib.unwrap().0) @@ -963,12 +965,16 @@ fn add_upstream_rust_crates(cmd: &mut Linker, // * For LTO, we remove upstream object files. // * For dylibs we remove metadata and bytecode from upstream rlibs // - // When performing LTO, all of the bytecode from the upstream libraries has - // already been included in our object file output. As a result we need to - // remove the object files in the upstream libraries so the linker doesn't - // try to include them twice (or whine about duplicate symbols). We must - // continue to include the rest of the rlib, however, as it may contain - // static native libraries which must be linked in. + // When performing LTO, almost(*) all of the bytecode from the upstream + // libraries has already been included in our object file output. As a + // result we need to remove the object files in the upstream libraries so + // the linker doesn't try to include them twice (or whine about duplicate + // symbols). We must continue to include the rest of the rlib, however, as + // it may contain static native libraries which must be linked in. + // + // (*) Crates marked with `#![no_builtins]` don't participate in LTO and + // their bytecode wasn't included. The object files in those libraries must + // still be passed to the linker. // // When making a dynamic library, linkers by default don't include any // object files in an archive if they're not necessary to resolve the link. @@ -988,7 +994,8 @@ fn add_upstream_rust_crates(cmd: &mut Linker, sess: &Session, tmpdir: &Path, crate_type: config::CrateType, - cratepath: &Path) { + cratepath: &Path, + is_a_no_builtins_crate: bool) { if !sess.lto() && crate_type != config::CrateTypeDylib { cmd.link_rlib(&fix_windows_verbatim_for_gcc(cratepath)); return @@ -1012,7 +1019,8 @@ fn add_upstream_rust_crates(cmd: &mut Linker, } let canonical = f.replace("-", "_"); let canonical_name = name.replace("-", "_"); - if sess.lto() && canonical.starts_with(&canonical_name) && + if sess.lto() && !is_a_no_builtins_crate && + canonical.starts_with(&canonical_name) && canonical.ends_with(".o") { let num = &f[name.len()..f.len() - 2]; if num.len() > 0 && num[1..].parse::().is_ok() { diff --git a/src/librustc_trans/back/lto.rs b/src/librustc_trans/back/lto.rs index 69e4a50804fad..016eb7c0cfb3c 100644 --- a/src/librustc_trans/back/lto.rs +++ b/src/librustc_trans/back/lto.rs @@ -17,6 +17,7 @@ use llvm::{ModuleRef, TargetMachineRef, True, False}; use rustc::util::common::time; use rustc::util::common::path2cstr; use back::write::{ModuleConfig, with_llvm_pmb}; +use syntax::attr; use libc; use flate; @@ -52,7 +53,15 @@ pub fn run(sess: &session::Session, llmod: ModuleRef, // For each of our upstream dependencies, find the corresponding rlib and // load the bitcode from the archive. Then merge it into the current LLVM // module that we've got. - link::each_linked_rlib(sess, &mut |_, path| { + link::each_linked_rlib(sess, &mut |cnum, path| { + let is_a_no_builtins_crate = + attr::contains_name(&sess.cstore.crate_attrs(cnum), "no_builtins"); + + // `#![no_builtins]` crates don't participate in LTO. + if is_a_no_builtins_crate { + return; + } + let archive = ArchiveRO::open(&path).expect("wanted an rlib"); let bytecodes = archive.iter().filter_map(|child| { child.ok().and_then(|c| c.name().map(|name| (name, c))) From 35eba85c3d63579bbe0929dc880caf2be906b767 Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Sun, 14 Aug 2016 21:56:26 -0500 Subject: [PATCH 2/3] refactor: use CStore::is_no_builtins --- src/librustc/middle/cstore.rs | 2 ++ src/librustc_metadata/csearch.rs | 4 ++++ src/librustc_trans/back/link.rs | 6 ++---- src/librustc_trans/back/lto.rs | 6 +----- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustc/middle/cstore.rs b/src/librustc/middle/cstore.rs index f1bb3a37e3c27..05cd822c6d635 100644 --- a/src/librustc/middle/cstore.rs +++ b/src/librustc/middle/cstore.rs @@ -227,6 +227,7 @@ pub trait CrateStore<'tcx> { fn plugin_registrar_fn(&self, cnum: ast::CrateNum) -> Option; fn native_libraries(&self, cnum: ast::CrateNum) -> Vec<(NativeLibraryKind, String)>; fn reachable_ids(&self, cnum: ast::CrateNum) -> Vec; + fn is_no_builtins(&self, cnum: ast::CrateNum) -> bool; // resolve fn def_index_for_def_key(&self, @@ -428,6 +429,7 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore { fn native_libraries(&self, cnum: ast::CrateNum) -> Vec<(NativeLibraryKind, String)> { bug!("native_libraries") } fn reachable_ids(&self, cnum: ast::CrateNum) -> Vec { bug!("reachable_ids") } + fn is_no_builtins(&self, cnum: ast::CrateNum) -> bool { bug!("is_no_builtins") } // resolve fn def_key(&self, def: DefId) -> hir_map::DefKey { bug!("def_key") } diff --git a/src/librustc_metadata/csearch.rs b/src/librustc_metadata/csearch.rs index 7ee6e54a666d6..2cdbd1b86012d 100644 --- a/src/librustc_metadata/csearch.rs +++ b/src/librustc_metadata/csearch.rs @@ -409,6 +409,10 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore { decoder::get_reachable_ids(&cdata) } + fn is_no_builtins(&self, cnum: ast::CrateNum) -> bool { + attr::contains_name(&self.crate_attrs(cnum), "no_builtins") + } + fn def_index_for_def_key(&self, cnum: ast::CrateNum, def: DefKey) diff --git a/src/librustc_trans/back/link.rs b/src/librustc_trans/back/link.rs index d17da74c87f39..e7ee952129466 100644 --- a/src/librustc_trans/back/link.rs +++ b/src/librustc_trans/back/link.rs @@ -42,7 +42,7 @@ use std::process::Command; use std::str; use flate; use syntax::ast; -use syntax::attr::{self, AttrMetaMethods}; +use syntax::attr::AttrMetaMethods; use syntax_pos::Span; // RLIB LLVM-BYTECODE OBJECT LAYOUT @@ -938,10 +938,8 @@ fn add_upstream_rust_crates(cmd: &mut Linker, Linkage::NotLinked | Linkage::IncludedFromDylib => {} Linkage::Static => { - let is_a_no_builtins_crate = - attr::contains_name(&sess.cstore.crate_attrs(cnum), "no_builtins"); add_static_crate(cmd, sess, tmpdir, crate_type, - &src.rlib.unwrap().0, is_a_no_builtins_crate) + &src.rlib.unwrap().0, sess.cstore.is_no_builtins(cnum)) } Linkage::Dynamic => { add_dynamic_crate(cmd, sess, &src.dylib.unwrap().0) diff --git a/src/librustc_trans/back/lto.rs b/src/librustc_trans/back/lto.rs index 016eb7c0cfb3c..522864c6ec3a4 100644 --- a/src/librustc_trans/back/lto.rs +++ b/src/librustc_trans/back/lto.rs @@ -17,7 +17,6 @@ use llvm::{ModuleRef, TargetMachineRef, True, False}; use rustc::util::common::time; use rustc::util::common::path2cstr; use back::write::{ModuleConfig, with_llvm_pmb}; -use syntax::attr; use libc; use flate; @@ -54,11 +53,8 @@ pub fn run(sess: &session::Session, llmod: ModuleRef, // load the bitcode from the archive. Then merge it into the current LLVM // module that we've got. link::each_linked_rlib(sess, &mut |cnum, path| { - let is_a_no_builtins_crate = - attr::contains_name(&sess.cstore.crate_attrs(cnum), "no_builtins"); - // `#![no_builtins]` crates don't participate in LTO. - if is_a_no_builtins_crate { + if sess.cstore.is_no_builtins(cnum) { return; } From e9964056964706fa3746c7e1135c34a45f2f467f Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Sun, 14 Aug 2016 21:56:48 -0500 Subject: [PATCH 3/3] test that a no_builtins crate is passed to the linker --- src/test/run-make/no-builtins-lto/Makefile | 9 +++++++++ src/test/run-make/no-builtins-lto/main.rs | 13 +++++++++++++ src/test/run-make/no-builtins-lto/no_builtins.rs | 12 ++++++++++++ 3 files changed, 34 insertions(+) create mode 100644 src/test/run-make/no-builtins-lto/Makefile create mode 100644 src/test/run-make/no-builtins-lto/main.rs create mode 100644 src/test/run-make/no-builtins-lto/no_builtins.rs diff --git a/src/test/run-make/no-builtins-lto/Makefile b/src/test/run-make/no-builtins-lto/Makefile new file mode 100644 index 0000000000000..3f70de5f76c3b --- /dev/null +++ b/src/test/run-make/no-builtins-lto/Makefile @@ -0,0 +1,9 @@ +-include ../tools.mk + +all: + # Compile a `#![no_builtins]` rlib crate + $(RUSTC) no_builtins.rs + # Build an executable that depends on that crate using LTO. The no_builtins crate doesn't + # participate in LTO, so its rlib must be explicitly linked into the final binary. Verify this by + # grepping the linker arguments. + $(RUSTC) main.rs -C lto -Z print-link-args | grep 'libno_builtins.rlib' diff --git a/src/test/run-make/no-builtins-lto/main.rs b/src/test/run-make/no-builtins-lto/main.rs new file mode 100644 index 0000000000000..e960c726a98cc --- /dev/null +++ b/src/test/run-make/no-builtins-lto/main.rs @@ -0,0 +1,13 @@ +// 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. + +extern crate no_builtins; + +fn main() {} diff --git a/src/test/run-make/no-builtins-lto/no_builtins.rs b/src/test/run-make/no-builtins-lto/no_builtins.rs new file mode 100644 index 0000000000000..be95e7c5521ee --- /dev/null +++ b/src/test/run-make/no-builtins-lto/no_builtins.rs @@ -0,0 +1,12 @@ +// 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. + +#![crate_type = "lib"] +#![no_builtins]