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

davidtwco · 2024-10-08T14:42:53Z

Includes llvm-strip (a symlink to llvm-objcopy) in the compiler dist artifact so that it can be used for -Cstrip instead of the system tooling.
Uses llvm-strip instead of /usr/bin/strip for macOS. macOS needs a specific linker and the system one is preferred, hence Fix up setting strip = true in Cargo.toml makes build scripts fail in… #130781 but that doesn't work when cross-compiling, so use the llvm-strip utility instead.

rustbot · 2024-10-08T14:43:02Z

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

davidtwco · 2024-10-08T14:48:21Z

We could make this same change to the illumos and AIX targets which also hardcode a path to a strip binary. Pinging relevant people for those targets to see if this would be desirable.

@ecnelises @bzEq (listed as target maintainers for powerpc64-ibm-aix
@jclulow (we have no target maintainers for the illumos targets in the documentation, but I've seen @jclulow in the issue tracker and I know is involved in illumos)

src/bootstrap/src/core/build_steps/dist.rs

src/bootstrap/src/core/build_steps/compile.rs

lqd · 2024-10-08T16:37:27Z

src/bootstrap/src/core/builder.rs

@@ -1151,6 +1151,11 @@ impl<'a> Builder<'a> {
        self.ensure(compile::Sysroot::new(compiler))
    }

+    /// Returns the bindir for a compiler's sysroot.


Probably t-bootstrap will have opinions here, because there are 2 "bin" directories 😓

workingjubilee · 2024-10-08T16:48:00Z

Thank you for posting this. I tried twice and couldn't even figure out where to start.

taiki-e · 2024-10-08T16:50:36Z

illumos

AFAIK some change is needed for illumos too because using the default strip binary of Linux host causes a problem (oxidecomputer/helios#147).
However, I have not actually tested to see if using LLVM strip would solve the problem. (This comment may indicate that anything other than native strip utility would not work.)

cc @sunshowers

lqd · 2024-10-08T16:53:42Z

compiler/rustc_codegen_ssa/src/back/link.rs

+    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());


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?

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.

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.

rust/compiler/rustc_codegen_ssa/src/back/linker.rs

Line 130 in bca5fde

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

I believe this is where it happens

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.

jclulow · 2024-10-08T17:57:12Z

@jclulow (we have no target maintainers for the illumos targets in the documentation, but I've seen @jclulow in the issue tracker and I know is involved in illumos)

Hey, with my apologies, someone pointed this out to me recently and I owe a PR to fix the docs and list some maintainers there.

We could make this same change to the illumos and AIX targets which also hardcode a path to a strip binary. Pinging relevant people for those targets to see if this would be desirable.

When we're running on an illumos host and compiling for an illumos target, we definitely want to use the system strip binary explicitly. If we're cross compiling, either on an illumos host for a non-illumos target, or on a non-illumos host, I don't have a particular preference and we should do whatever is tested to work there.

Does that make sense?

bzEq · 2024-10-08T22:34:10Z

Thanks for headsup. On AIX, we are still using /usr/bin/strip. llvm-strip is not compatible with AIX's system strip utility.

…nnethercote Fix hardcoded strip path when cross-compiling from Linux to Darwin Fixes rust-lang#131206. I fear that rust-lang#131405 might end up taking some time, so opening this PR to resolve the regression. `@rustbot` label O-apple

Rollup merge of rust-lang#131480 - madsmtm:macos-fix-strip-binary, r=nnethercote Fix hardcoded strip path when cross-compiling from Linux to Darwin Fixes rust-lang#131206. I fear that rust-lang#131405 might end up taking some time, so opening this PR to resolve the regression. `@rustbot` label O-apple

davidtwco · 2024-10-11T13:03:43Z

Thanks for headsup. On AIX, we are still using /usr/bin/strip. llvm-strip is not compatible with AIX's system strip utility.

Okay, I've kept AIX's logic as it is, but I have added a warning when cross-compiling that whatever is in /usr/bin/strip may not work because it isn't AIX's system strip.

When we're running on an illumos host and compiling for an illumos target, we definitely want to use the system strip binary explicitly. If we're cross compiling, either on an illumos host for a non-illumos target, or on a non-illumos host, I don't have a particular preference and we should do whatever is tested to work there.

Does that make sense?

That makes sense. I've kept /usr/bin/strip if not cross-compiling, and we'll now use llvm-strip instead of /usr/bin/strip when cross-compiling (that seems more likely to work reliably than relying on whatever /usr/bin/strip could be). I also changed the condition we used here from target.os == "illumos" to target.is_like_solaris, since that's why we have is_like_solaris in the first place. Let me know if that's not correct and there is some difference with stripping between illumos and solaris in general.

albertlarsan68

LGTM for the technical side in bootstrap

albertlarsan68 · 2024-10-13T15:48:28Z

r? t-compiler for the compiler side of things

jieyouxu · 2024-11-02T11:52:04Z

r? jieyouxu

jieyouxu

Thanks, this looks good to me (with compiler hat 🎩 on). Feel free to r=albertlarsan68 and me after a rebase.

jieyouxu · 2024-11-02T12:17:46Z

src/bootstrap/src/core/builder/mod.rs

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


Remark (for myself): send a follow-up to avoid this dance

Follow-up PR: #132723

jieyouxu · 2024-11-04T13:05:03Z

@rustbot author

davidtwco · 2024-11-05T13:33:29Z

@bors r=jieyouxu,albertlarsan68

bors · 2024-11-05T13:33:32Z

📌 Commit f745467 has been approved by jieyouxu,albertlarsan68

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#131261 (Stabilize `UnsafeCell::from_mut`) - rust-lang#131405 (bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip) - rust-lang#132077 (Add a new `wide-arithmetic` feature for WebAssembly) - rust-lang#132562 (Remove the `wasm32-wasi` target from rustc) - rust-lang#132660 (Remove unused errs.rs file) Failed merges: - rust-lang#131721 (Add new unstable feature `const_eq_ignore_ascii_case`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131405 - davidtwco:hardcoded-strip-macos, r=jieyouxu,albertlarsan68 bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip Fixes rust-lang#131206. - Includes `llvm-strip` (a symlink to `llvm-objcopy`) in the compiler dist artifact so that it can be used for `-Cstrip` instead of the system tooling. - Uses `llvm-strip` instead of `/usr/bin/strip` for macOS. macOS needs a specific linker and the system one is preferred, hence rust-lang#130781 but that doesn't work when cross-compiling, so use the `llvm-strip` utility instead. cc rust-lang#123151

src/bootstrap/src/core/build_steps/compile.rs

Only copy, rename and link `llvm-objcopy` if llvm tools are enabled Fixes rust-lang#132719. cc `@bjorn3` who reported the bootstrapping problem for cg_clif. cc `@davidtwco` in case this might be problematic for linux -> macOS cross-compile builds, but seems very unlikely. cc `@albertlarsan68` (co-reviewed rust-lang#131405) r? bootstrap

rustbot assigned albertlarsan68 Oct 8, 2024

This comment was marked as resolved.

Sign in to view

jieyouxu reviewed Oct 8, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/dist.rs Outdated Show resolved Hide resolved

lqd reviewed Oct 8, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/compile.rs Outdated Show resolved Hide resolved

davidtwco force-pushed the hardcoded-strip-macos branch from 159f31c to eadf0dd Compare October 8, 2024 16:08

lqd reviewed Oct 8, 2024

View reviewed changes

This was referenced Oct 10, 2024

Fix hardcoded strip path when cross-compiling from Linux to Darwin #131480

Merged

Hard-coded strip path breaks Linux -> Darwin builds #131206

Closed

This comment was marked as resolved.

Sign in to view

davidtwco force-pushed the hardcoded-strip-macos branch from eadf0dd to d9b8a78 Compare October 11, 2024 13:01

davidtwco force-pushed the hardcoded-strip-macos branch from d9b8a78 to 89cd82d Compare October 11, 2024 13:13

albertlarsan68 reviewed Oct 13, 2024

View reviewed changes

rustbot assigned cjgillot and unassigned albertlarsan68 Oct 13, 2024

This comment was marked as resolved.

Sign in to view

davidtwco force-pushed the hardcoded-strip-macos branch from 89cd82d to a0b6f83 Compare October 22, 2024 12:51

This comment was marked as resolved.

Sign in to view

rustbot assigned jieyouxu and unassigned estebank Nov 2, 2024

jieyouxu approved these changes Nov 2, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2024

davidtwco added 2 commits November 5, 2024 11:49

bootstrap: include llvm-objcopy in dist

d8ab230

codegen_ssa: use llvm-objcopy for macOS strip

f745467

davidtwco force-pushed the hardcoded-strip-macos branch from a0b6f83 to f745467 Compare November 5, 2024 12:06

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2024

matthiaskrgr mentioned this pull request Nov 5, 2024

Rollup of 5 pull requests #132664

Merged

bors merged commit 8dee3e9 into rust-lang:master Nov 6, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Nov 6, 2024

davidtwco deleted the hardcoded-strip-macos branch November 6, 2024 11:30

bjorn3 reviewed Nov 7, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/compile.rs Show resolved Hide resolved

This was referenced Nov 7, 2024

Shipping llvm-strip w/ a symlink breaks when bootstrap compiler doesn't contain any LLVM tools #132719

Closed

Only copy, rename and link llvm-objcopy if llvm tools are enabled #132720

Merged

Unify sysroot_target_{bin,lib}dir handling #132723

Open

durin42 mentioned this pull request Nov 7, 2024

link: remove unused if check #132742

Closed

ehuss mentioned this pull request Nov 11, 2024

Bootstrap fails "No such file" for llvm-objcopy #132903

Closed

jieyouxu mentioned this pull request Nov 19, 2024

Bootstrap without creating a config.toml fails with missing rust-objcopy #133195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

davidtwco commented Oct 8, 2024

rustbot commented Oct 8, 2024

davidtwco commented Oct 8, 2024

This comment was marked as resolved.

lqd Oct 8, 2024

workingjubilee commented Oct 8, 2024

taiki-e commented Oct 8, 2024

lqd Oct 8, 2024 •

edited

Loading

davidtwco Oct 11, 2024

lqd Oct 22, 2024

davidtwco Oct 22, 2024

lqd Oct 22, 2024

jclulow commented Oct 8, 2024

bzEq commented Oct 8, 2024 •

edited

Loading

This comment was marked as resolved.

davidtwco commented Oct 11, 2024

albertlarsan68 left a comment

albertlarsan68 commented Oct 13, 2024

This comment was marked as resolved.

This comment was marked as resolved.

jieyouxu commented Nov 2, 2024

jieyouxu left a comment

jieyouxu Nov 2, 2024

jieyouxu Nov 7, 2024

jieyouxu commented Nov 4, 2024

davidtwco commented Nov 5, 2024

bors commented Nov 5, 2024

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

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

Conversation

davidtwco commented Oct 8, 2024

rustbot commented Oct 8, 2024

davidtwco commented Oct 8, 2024

This comment was marked as resolved.

lqd Oct 8, 2024

Choose a reason for hiding this comment

workingjubilee commented Oct 8, 2024

taiki-e commented Oct 8, 2024

lqd Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

davidtwco Oct 11, 2024

Choose a reason for hiding this comment

lqd Oct 22, 2024

Choose a reason for hiding this comment

davidtwco Oct 22, 2024

Choose a reason for hiding this comment

lqd Oct 22, 2024

Choose a reason for hiding this comment

jclulow commented Oct 8, 2024

bzEq commented Oct 8, 2024 • edited Loading

This comment was marked as resolved.

davidtwco commented Oct 11, 2024

albertlarsan68 left a comment

Choose a reason for hiding this comment

albertlarsan68 commented Oct 13, 2024

This comment was marked as resolved.

This comment was marked as resolved.

jieyouxu commented Nov 2, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Nov 2, 2024

Choose a reason for hiding this comment

jieyouxu Nov 7, 2024

Choose a reason for hiding this comment

jieyouxu commented Nov 4, 2024

davidtwco commented Nov 5, 2024

bors commented Nov 5, 2024

lqd Oct 8, 2024 •

edited

Loading

bzEq commented Oct 8, 2024 •

edited

Loading