Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate issue-64153, invalid-staticlib and no-builtins-lto run-make tests to rmake #126437

Merged
merged 3 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/tools/run-make-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ pub use cc::{cc, extra_c_flags, extra_cxx_flags, Cc};
pub use clang::{clang, Clang};
pub use diff::{diff, Diff};
pub use llvm::{
llvm_filecheck, llvm_profdata, llvm_readobj, LlvmFilecheck, LlvmProfdata, LlvmReadobj,
llvm_filecheck, llvm_objdump, llvm_profdata, llvm_readobj, LlvmFilecheck, LlvmObjdump,
LlvmProfdata, LlvmReadobj,
};
pub use run::{cmd, run, run_fail, run_with_args};
pub use rustc::{aux_build, rustc, Rustc};
Expand Down
30 changes: 30 additions & 0 deletions src/tools/run-make-support/src/llvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ pub fn llvm_filecheck() -> LlvmFilecheck {
LlvmFilecheck::new()
}

/// Construct a new `llvm-objdump` invocation. This assumes that `llvm-objdump` is available
/// at `$LLVM_BIN_DIR/llvm-objdump`.
pub fn llvm_objdump() -> LlvmObjdump {
LlvmObjdump::new()
}

/// A `llvm-readobj` invocation builder.
#[derive(Debug)]
#[must_use]
Expand All @@ -41,9 +47,17 @@ pub struct LlvmFilecheck {
cmd: Command,
}

/// A `llvm-objdump` invocation builder.
#[derive(Debug)]
#[must_use]
pub struct LlvmObjdump {
cmd: Command,
}

crate::impl_common_helpers!(LlvmReadobj);
crate::impl_common_helpers!(LlvmProfdata);
crate::impl_common_helpers!(LlvmFilecheck);
crate::impl_common_helpers!(LlvmObjdump);

/// Generate the path to the bin directory of LLVM.
#[must_use]
Expand Down Expand Up @@ -125,3 +139,19 @@ impl LlvmFilecheck {
self
}
}

impl LlvmObjdump {
/// Construct a new `llvm-objdump` invocation. This assumes that `llvm-objdump` is available
/// at `$LLVM_BIN_DIR/llvm-objdump`.
pub fn new() -> Self {
let llvm_objdump = llvm_bin_dir().join("llvm-objdump");
let cmd = Command::new(llvm_objdump);
Self { cmd }
}

/// Provide an input file.
pub fn input<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
self.cmd.arg(path.as_ref());
self
}
}
45 changes: 0 additions & 45 deletions src/tools/run-make-support/src/llvm_readobj.rs

This file was deleted.

3 changes: 0 additions & 3 deletions src/tools/tidy/src/allowed_run_make_makefiles.txt
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ run-make/interdependent-c-libraries/Makefile
run-make/intrinsic-unreachable/Makefile
run-make/invalid-library/Makefile
run-make/invalid-so/Makefile
run-make/invalid-staticlib/Makefile
run-make/issue-107094/Makefile
run-make/issue-10971-temps-dir/Makefile
run-make/issue-109934-lto-debuginfo/Makefile
Expand All @@ -96,7 +95,6 @@ run-make/issue-40535/Makefile
run-make/issue-47384/Makefile
run-make/issue-47551/Makefile
run-make/issue-51671/Makefile
run-make/issue-64153/Makefile
run-make/issue-68794-textrel-on-minimal-lib/Makefile
run-make/issue-69368/Makefile
run-make/issue-83045/Makefile
Expand Down Expand Up @@ -148,7 +146,6 @@ run-make/native-link-modifier-verbatim-rustc/Makefile
run-make/native-link-modifier-whole-archive/Makefile
run-make/no-alloc-shim/Makefile
run-make/no-builtins-attribute/Makefile
run-make/no-builtins-lto/Makefile
run-make/no-duplicate-libs/Makefile
run-make/obey-crate-type-flag/Makefile
run-make/optimization-remarks-dir-pgo/Makefile
Expand Down
5 changes: 0 additions & 5 deletions tests/run-make/invalid-staticlib/Makefile

This file was deleted.

17 changes: 17 additions & 0 deletions tests/run-make/invalid-staticlib/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// If the static library provided is not valid (in this test,
// created as an empty file),
// rustc should print a normal error message and not throw
// an internal compiler error (ICE).
// See https://github.com/rust-lang/rust/pull/28673

use run_make_support::{fs_wrapper, rustc, static_lib_name};

fn main() {
fs_wrapper::create_file(static_lib_name("foo"));
rustc()
.arg("-")
.crate_type("rlib")
.arg("-lstatic=foo")
.run_fail()
.assert_stderr_contains("failed to add native library");
}
26 changes: 0 additions & 26 deletions tests/run-make/issue-64153/Makefile

This file was deleted.

40 changes: 40 additions & 0 deletions tests/run-make/lto-avoid-object-duplication/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// ignore-tidy-tab
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved
// 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.

//@ ignore-windows
// Reason: `llvm-objdump`'s output looks different on windows than on other platforms.
// Only checking on Unix platforms should suffice.
Comment on lines +11 to +13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark (non-blocking): I'm not super convinced by this reasoning and it would be good to also make this test work on Windows in future improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have temporarily removed the ignore directive, maybe we could bors try it.

//FIXME(Oneirical): This could be adapted to work on Windows by checking how
// that output differs.

use run_make_support::{llvm_objdump, regex, rust_lib_name, rustc, static_lib_name};

fn main() {
rustc()
.crate_type("rlib")
.input("upstream.rs")
.output(rust_lib_name("upstream"))
.codegen_units(1)
.run();
rustc()
.crate_type("staticlib")
.input("downstream.rs")
.arg("-Clto")
.output(static_lib_name("downstream"))
.codegen_units(1)
.run();
let syms = llvm_objdump().arg("-t").input(static_lib_name("downstream")).run().stdout_utf8();
let re = regex::Regex::new(r#"\s*g\s*F\s.*issue64153_test_function"#).unwrap();
// Count the global instances of `issue64153_test_function`. There'll be 2
// if the `upstream` object file got erroneously 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
assert_eq!(re.find_iter(syms.as_str()).count(), 1);
}
9 changes: 0 additions & 9 deletions tests/run-make/no-builtins-lto/Makefile

This file was deleted.

20 changes: 20 additions & 0 deletions tests/run-make/no-builtins-lto/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// The rlib produced by a no_builtins crate should be explicitely linked
// during compilation, and as a result be present in the linker arguments.
// See the comments inside this file for more details.
// See https://github.com/rust-lang/rust/pull/35637

use run_make_support::{rust_lib_name, rustc};

fn main() {
// Compile a `#![no_builtins]` rlib crate
rustc().input("no_builtins.rs").run();
// 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()
.input("main.rs")
.arg("-Clto")
.print("link-args")
.run()
.assert_stdout_contains(rust_lib_name("no_builtins"));
}
Loading