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

bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip #131405

Merged
merged 2 commits into from
Nov 6, 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
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_ssa/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ codegen_ssa_L4Bender_exporting_symbols_unimplemented = exporting symbols not imp

codegen_ssa_add_native_library = failed to add native library {$library_path}: {$error}

codegen_ssa_aix_strip_not_used = using host's `strip` binary to cross-compile to AIX which is not guaranteed to work

codegen_ssa_apple_deployment_target_invalid =
failed to parse deployment target specified in {$env_var}: {$error}

Expand Down
22 changes: 17 additions & 5 deletions compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1085,9 +1085,7 @@ fn link_natively(
let strip = sess.opts.cg.strip;

if sess.target.is_like_osx {
// Use system `strip` when running on host macOS.
// <https://github.com/rust-lang/rust/pull/130781>
let stripcmd = if cfg!(target_os = "macos") { "/usr/bin/strip" } else { "strip" };
let stripcmd = "rust-objcopy";
match (strip, crate_type) {
(Strip::Debuginfo, _) => {
strip_symbols_with_external_utility(sess, stripcmd, out_filename, Some("-S"))
Expand All @@ -1103,11 +1101,14 @@ fn link_natively(
}
}

if sess.target.os == "illumos" {
if sess.target.is_like_solaris {
// Many illumos systems will have both the native 'strip' utility and
// the GNU one. Use the native version explicitly and do not rely on
// what's in the path.
let stripcmd = "/usr/bin/strip";
//
// If cross-compiling and there is not a native version, then use
// `llvm-strip` and hope.
let stripcmd = if !sess.host.is_like_solaris { "rust-objcopy" } else { "/usr/bin/strip" };
match strip {
// Always preserve the symbol table (-x).
Strip::Debuginfo => {
Expand All @@ -1120,6 +1121,10 @@ fn link_natively(
}

if sess.target.is_like_aix {
// `llvm-strip` doesn't work for AIX - their strip must be used.
if !sess.host.is_like_aix {
sess.dcx().emit_warn(errors::AixStripNotUsed);
}
let stripcmd = "/usr/bin/strip";
match strip {
Strip::Debuginfo => {
Expand Down Expand Up @@ -1147,6 +1152,13 @@ fn strip_symbols_with_external_utility(
if let Some(option) = option {
cmd.arg(option);
}

let mut new_path = sess.get_tools_search_paths(false);
if let Some(path) = env::var_os("PATH") {
new_path.extend(env::split_paths(&path));
}
cmd.env("PATH", env::join_paths(new_path).unwrap());
Comment on lines +1156 to +1160
Copy link
Member

@lqd lqd Oct 8, 2024

Choose a reason for hiding this comment

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

also: it's probably safer to look for rust-objcopy in the tools search path and launch w/ the absolute path cmd rather than overriding the $PATH?

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably is but we override the PATH for rust-lld so I'll keep it this way and we can change both together later.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, do we? I remember we set the linker search path to the lld wrappers in the sysroot without overriding the PATH (so they should execute ../rust-lld without overrides), and that rustup ensures the PATH contains the bin and rustlib/bin folders, but didn’t remember rustc overrode it for rust-lld.

Copy link
Member Author

Choose a reason for hiding this comment

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

cmd.env("PATH", env::join_paths(new_path).unwrap());

I believe this is where it happens

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's indeed likely it for the targets involving the linker directly. We don't need/make use of that on the common linux/mac targets.


let prog = cmd.arg(out_filename).output();
match prog {
Ok(prog) => {
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_ssa/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1101,3 +1101,7 @@ impl<G: EmissionGuarantee> Diagnostic<'_, G> for TargetFeatureDisableOrEnable<'_
diag
}
}

#[derive(Diagnostic)]
#[diag(codegen_ssa_aix_strip_not_used)]
pub(crate) struct AixStripNotUsed;
11 changes: 9 additions & 2 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,7 @@ impl Step for Std {
.join(compiler.host)
.join("bin");
if src_sysroot_bin.exists() {
let target_sysroot_bin =
builder.sysroot_target_libdir(compiler, target).parent().unwrap().join("bin");
let target_sysroot_bin = builder.sysroot_target_bindir(compiler, target);
t!(fs::create_dir_all(&target_sysroot_bin));
builder.cp_link_r(&src_sysroot_bin, &target_sysroot_bin);
}
Expand Down Expand Up @@ -1977,6 +1976,14 @@ impl Step for Assemble {
}
}

{
// `llvm-strip` is used by rustc, which is actually just a symlink to `llvm-objcopy`,
// so copy and rename `llvm-objcopy`.
let src_exe = exe("llvm-objcopy", target_compiler.host);
let dst_exe = exe("rust-objcopy", target_compiler.host);
builder.copy_link(&libdir_bin.join(src_exe), &libdir_bin.join(dst_exe));
}
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved

// In addition to `rust-lld` also install `wasm-component-ld` when
// LLD is enabled. This is a relatively small binary that primarily
// delegates to the `rust-lld` binary for linking and then runs
Expand Down
14 changes: 10 additions & 4 deletions src/bootstrap/src/core/build_steps/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,7 @@ impl Step for Rustc {

// Copy over lld if it's there
if builder.config.lld_enabled {
let src_dir =
builder.sysroot_target_libdir(compiler, host).parent().unwrap().join("bin");
let src_dir = builder.sysroot_target_bindir(compiler, host);
let rust_lld = exe("rust-lld", compiler.host);
builder.copy_link(&src_dir.join(&rust_lld), &dst_dir.join(&rust_lld));
let self_contained_lld_src_dir = src_dir.join("gcc-ld");
Expand All @@ -474,9 +473,16 @@ impl Step for Rustc {
);
}
}

{
let src_dir = builder.sysroot_target_bindir(compiler, host);
let llvm_objcopy = exe("llvm-objcopy", compiler.host);
let rust_objcopy = exe("rust-objcopy", compiler.host);
builder.copy_link(&src_dir.join(&llvm_objcopy), &dst_dir.join(&rust_objcopy));
}

if builder.tool_enabled("wasm-component-ld") {
let src_dir =
builder.sysroot_target_libdir(compiler, host).parent().unwrap().join("bin");
let src_dir = builder.sysroot_target_bindir(compiler, host);
let ld = exe("wasm-component-ld", compiler.host);
builder.copy_link(&src_dir.join(&ld), &dst_dir.join(&ld));
}
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/src/core/builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1161,6 +1161,11 @@ impl<'a> Builder<'a> {
self.ensure(compile::Sysroot::new(compiler))
}

/// Returns the bindir for a compiler's sysroot.
pub fn sysroot_target_bindir(&self, compiler: Compiler, target: TargetSelection) -> PathBuf {
self.sysroot_target_libdir(compiler, target).parent().unwrap().join("bin")
}

/// Returns the libdir where the standard library and other artifacts are
/// found for a compiler's sysroot.
pub fn sysroot_target_libdir(&self, compiler: Compiler, target: TargetSelection) -> PathBuf {
Expand Down
Loading