From 412cec2e12ebb86fb526918431acc991ea4e540b Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 14 Oct 2019 18:04:00 +0200 Subject: [PATCH 1/3] Add regression test for issue #64153. --- .../run-make-fulldeps/issue-64153/Makefile | 22 +++++++++++++++++++ .../issue-64153/downstream.rs | 6 +++++ .../run-make-fulldeps/issue-64153/upstream.rs | 6 +++++ 3 files changed, 34 insertions(+) create mode 100644 src/test/run-make-fulldeps/issue-64153/Makefile create mode 100644 src/test/run-make-fulldeps/issue-64153/downstream.rs create mode 100644 src/test/run-make-fulldeps/issue-64153/upstream.rs diff --git a/src/test/run-make-fulldeps/issue-64153/Makefile b/src/test/run-make-fulldeps/issue-64153/Makefile new file mode 100644 index 0000000000000..5ad62a9a01ce2 --- /dev/null +++ b/src/test/run-make-fulldeps/issue-64153/Makefile @@ -0,0 +1,22 @@ +-include ../tools.mk + +# Staticlibs don't include Rust object files from upstream crates if the same +# code was already pulled into the lib via LTO. However, the bug described in +# https://github.com/rust-lang/rust/issues/64153 lead to this exclusion not +# working properly if the upstream crate was compiled with an explicit filename +# (via `-o`). +# +# This test makes sure that functions defined in the upstream crates do not +# appear twice in the final staticlib when listing all the symbols from it. + +all: + $(RUSTC) --crate-type rlib upstream.rs -o $(TMPDIR)/libupstream.rlib -Ccodegen-units=1 + $(RUSTC) --crate-type staticlib downstream.rs -Clto -Ccodegen-units=1 -o $(TMPDIR)/libdownstream.a + # Dump all the symbols from the staticlib into `syms` + "$(LLVM_BIN_DIR)"/llvm-objdump -t $(TMPDIR)/libdownstream.a > $(TMPDIR)/syms + # Count the global instances of `issue64153_test_function`. There'll be 2 + # if the `upstream` object file got erronously included twice. + # The line we are testing for with the regex looks something like: + # 0000000000000000 g F .text.issue64153_test_function 00000023 issue64153_test_function + grep -c -e "[[:space:]]g[[:space:]]*F[[:space:]].*issue64153_test_function" $(TMPDIR)/syms > $(TMPDIR)/count + [ "$$(cat $(TMPDIR)/count)" -eq "1" ] diff --git a/src/test/run-make-fulldeps/issue-64153/downstream.rs b/src/test/run-make-fulldeps/issue-64153/downstream.rs new file mode 100644 index 0000000000000..e03704665d46c --- /dev/null +++ b/src/test/run-make-fulldeps/issue-64153/downstream.rs @@ -0,0 +1,6 @@ +extern crate upstream; + +#[no_mangle] +pub extern "C" fn foo() { + print!("1 + 1 = {}", upstream::issue64153_test_function(1)); +} diff --git a/src/test/run-make-fulldeps/issue-64153/upstream.rs b/src/test/run-make-fulldeps/issue-64153/upstream.rs new file mode 100644 index 0000000000000..861a00298ea39 --- /dev/null +++ b/src/test/run-make-fulldeps/issue-64153/upstream.rs @@ -0,0 +1,6 @@ +// Make this function extern "C", public, and no-mangle, so that it gets +// exported from the downstream staticlib. +#[no_mangle] +pub extern "C" fn issue64153_test_function(x: u32) -> u32 { + x + 1 +} From 52eda13031f039b5996b7d863715b0439297071e Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 15 Oct 2019 13:42:27 +0200 Subject: [PATCH 2/3] Fix issue #64153 by checking for .rcgu.o suffix when trying to identify Rust generated object files. --- src/librustc_codegen_llvm/back/archive.rs | 6 ++++-- src/librustc_codegen_ssa/back/link.rs | 21 ++++---------------- src/librustc_codegen_ssa/lib.rs | 24 +++++++++++++++++++++-- 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/librustc_codegen_llvm/back/archive.rs b/src/librustc_codegen_llvm/back/archive.rs index 68d3f90cd3991..e169cfc4cc829 100644 --- a/src/librustc_codegen_llvm/back/archive.rs +++ b/src/librustc_codegen_llvm/back/archive.rs @@ -9,7 +9,9 @@ use std::str; use crate::llvm::archive_ro::{ArchiveRO, Child}; use crate::llvm::{self, ArchiveKind}; -use rustc_codegen_ssa::{METADATA_FILENAME, RLIB_BYTECODE_EXTENSION}; +use rustc_codegen_ssa::{ + METADATA_FILENAME, RLIB_BYTECODE_EXTENSION, looks_like_rust_object_file +}; use rustc_codegen_ssa::back::archive::{ArchiveBuilder, find_library}; use rustc::session::Session; use syntax::symbol::Symbol; @@ -141,7 +143,7 @@ impl<'a> ArchiveBuilder<'a> for LlvmArchiveBuilder<'a> { } // Don't include Rust objects if LTO is enabled - if lto && fname.starts_with(&obj_start) && fname.ends_with(".o") { + if lto && looks_like_rust_object_file(fname) { return true } diff --git a/src/librustc_codegen_ssa/back/link.rs b/src/librustc_codegen_ssa/back/link.rs index 1c5d3b1a890ee..a2b50ea8e2bf7 100644 --- a/src/librustc_codegen_ssa/back/link.rs +++ b/src/librustc_codegen_ssa/back/link.rs @@ -3,7 +3,7 @@ use rustc::session::{Session, filesearch}; use rustc::session::config::{ - self, RUST_CGU_EXT, DebugInfo, OutputFilenames, OutputType, PrintRequest, Sanitizer + self, DebugInfo, OutputFilenames, OutputType, PrintRequest, Sanitizer }; use rustc::session::search_paths::PathKind; use rustc::middle::dependency_format::Linkage; @@ -15,7 +15,8 @@ use rustc_fs_util::fix_windows_verbatim_for_gcc; use rustc_target::spec::{PanicStrategy, RelroLevel, LinkerFlavor}; use syntax::symbol::Symbol; -use crate::{METADATA_FILENAME, RLIB_BYTECODE_EXTENSION, CrateInfo, CodegenResults}; +use crate::{METADATA_FILENAME, RLIB_BYTECODE_EXTENSION, CrateInfo, + looks_like_rust_object_file, CodegenResults}; use super::archive::ArchiveBuilder; use super::command::Command; use super::linker::Linker; @@ -1549,23 +1550,9 @@ fn add_upstream_rust_crates<'a, B: ArchiveBuilder<'a>>( let canonical = f.replace("-", "_"); let canonical_name = name.replace("-", "_"); - // Look for `.rcgu.o` at the end of the filename to conclude - // that this is a Rust-related object file. - fn looks_like_rust(s: &str) -> bool { - let path = Path::new(s); - let ext = path.extension().and_then(|s| s.to_str()); - if ext != Some(OutputType::Object.extension()) { - return false - } - let ext2 = path.file_stem() - .and_then(|s| Path::new(s).extension()) - .and_then(|s| s.to_str()); - ext2 == Some(RUST_CGU_EXT) - } - let is_rust_object = canonical.starts_with(&canonical_name) && - looks_like_rust(&f); + looks_like_rust_object_file(&f); // If we've been requested to skip all native object files // (those not generated by the rust compiler) then we can skip diff --git a/src/librustc_codegen_ssa/lib.rs b/src/librustc_codegen_ssa/lib.rs index 0221a04b04518..dd75883f97deb 100644 --- a/src/librustc_codegen_ssa/lib.rs +++ b/src/librustc_codegen_ssa/lib.rs @@ -22,9 +22,9 @@ #[macro_use] extern crate rustc; #[macro_use] extern crate syntax; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use rustc::dep_graph::WorkProduct; -use rustc::session::config::{OutputFilenames, OutputType}; +use rustc::session::config::{OutputFilenames, OutputType, RUST_CGU_EXT}; use rustc::middle::lang_items::LangItem; use rustc::hir::def_id::CrateNum; use rustc::ty::query::Providers; @@ -62,6 +62,7 @@ pub struct ModuleCodegen { pub const METADATA_FILENAME: &str = "rust.metadata.bin"; pub const RLIB_BYTECODE_EXTENSION: &str = "bc.z"; + impl ModuleCodegen { pub fn into_compiled_module(self, emit_obj: bool, @@ -166,3 +167,22 @@ pub fn provide_extern(providers: &mut Providers<'_>) { crate::back::symbol_export::provide_extern(providers); crate::base::provide_both(providers); } + +/// Checks if the given filename ends with the `.rcgu.o` extension that `rustc` +/// uses for the object files it generates. +pub fn looks_like_rust_object_file(filename: &str) -> bool { + let path = Path::new(filename); + let ext = path.extension().and_then(|s| s.to_str()); + if ext != Some(OutputType::Object.extension()) { + // The file name does not end with ".o", so it can't be an object file. + return false + } + + // Strip the ".o" at the end + let ext2 = path.file_stem() + .and_then(|s| Path::new(s).extension()) + .and_then(|s| s.to_str()); + + // Check if the "inner" extension + ext2 == Some(RUST_CGU_EXT) +} From a63dfb3b82a44e31bdfadb13d240a5366f9979fb Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 28 Oct 2019 17:07:18 +0100 Subject: [PATCH 3/3] Ignore issue-64153 run-make test on Windows since supporting a Windows version is not worth the trouble. --- src/test/run-make-fulldeps/issue-64153/Makefile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/run-make-fulldeps/issue-64153/Makefile b/src/test/run-make-fulldeps/issue-64153/Makefile index 5ad62a9a01ce2..9c0c3fe6e019c 100644 --- a/src/test/run-make-fulldeps/issue-64153/Makefile +++ b/src/test/run-make-fulldeps/issue-64153/Makefile @@ -1,5 +1,9 @@ -include ../tools.mk +# `llvm-objdump`'s output looks different on windows than on other platforms. +# It should be enough to check on Unix platforms, so: +# ignore-windows + # Staticlibs don't include Rust object files from upstream crates if the same # code was already pulled into the lib via LTO. However, the bug described in # https://github.com/rust-lang/rust/issues/64153 lead to this exclusion not