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

Pass objcopy args for stripping on OSX #135034

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Jan 2, 2025

When -Cstrip was changed in #131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, -S actually means --strip-all, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass --strip-debug and --strip-all.

fixes #135028

try-job: aarch64-apple
try-job: dist-aarch64-apple

@Noratrieb
Copy link
Member Author

@bors try

@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2025
Pass objcopy args for stripping on OSX

When `-Cstrip` was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, `-S` actually means `--strip-all`, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass `--strip-debug` and `--strip-all`.

fixes rust-lang#135028

try-jobs: aarch64-apple
@bors
Copy link
Contributor

bors commented Jan 2, 2025

⌛ Trying commit 26309e5 with merge cc677e6...

@Noratrieb Noratrieb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 2, 2025
@klensy
Copy link
Contributor

klensy commented Jan 2, 2025

We want to strip binaries, so we would use llvm-objcopy but not llvm-strip. Can this use llvm-strip instead?

@Noratrieb
Copy link
Member Author

That is a possible alternative, but I would prefer to avoid changing bootstrap to ship it as llvm-strip, especially for something that's backported. Just changing the flags works fine.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, the changes LGTM with two nits. r=me if try job comes back green. I don't have access to a native macOS environment to try this locally though.

compiler/rustc_codegen_ssa/src/back/link.rs Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/link.rs Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@Noratrieb
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jan 2, 2025

⌛ Trying commit 75a5355 with merge efbd1f63fafc97e79d87330dba4f3d04396ecdf3...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2025
Pass objcopy args for stripping on OSX

When `-Cstrip` was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, `-S` actually means `--strip-all`, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass `--strip-debug` and `--strip-all`.

fixes rust-lang#135028

try-jobs: aarch64-apple
try-jobs: dist-aarch64-apple
@bors
Copy link
Contributor

bors commented Jan 2, 2025

☀️ Try build successful - checks-actions
Build commit: efbd1f6 (efbd1f63fafc97e79d87330dba4f3d04396ecdf3)

@Noratrieb
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2025
Pass objcopy args for stripping on OSX

When `-Cstrip` was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, `-S` actually means `--strip-all`, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass `--strip-debug` and `--strip-all`.

fixes rust-lang#135028

try-job: aarch64-apple
try-job: dist-aarch64-apple
@bors
Copy link
Contributor

bors commented Jan 2, 2025

⌛ Trying commit 75a5355 with merge 3f1985a...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 2, 2025

💔 Test failed - checks-actions

@bors bors 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 Jan 2, 2025
When `-Cstrip` was changed to use the bundled rust-objcopy instead of
/usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they
have different defaults depending on which binary they are.
Notably, strip strips everything by default, and objcopy doesn't strip
anything by default.

Additionally, `-S` actually means `--strip-all`, so debuginfo stripped
everything and symbols didn't strip anything.

We now correctly pass `--strip-debug` and `--strip-all`.
@bors
Copy link
Contributor

bors commented Jan 2, 2025

⌛ Trying commit 4da3aed with merge 05ab192...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2025
Pass objcopy args for stripping on OSX

When `-Cstrip` was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, `-S` actually means `--strip-all`, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass `--strip-debug` and `--strip-all`.

fixes rust-lang#135028

try-job: aarch64-apple
try-job: dist-aarch64-apple
@bors
Copy link
Contributor

bors commented Jan 2, 2025

☀️ Try build successful - checks-actions
Build commit: 05ab192 (05ab192fe0df5911298fa2b06c24e558f7d153cc)

@Noratrieb
Copy link
Member Author

@orlp did some local testing of this commit and I did as well, it appears to work correctly, doing precisely as much stripping as we asked for. I observed the same working behavior on stable, and the reported broken behavior on beta.

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Jan 3, 2025

📌 Commit 4da3aed has been approved by jieyouxu

It is now in the queue for this repository.

@bors 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 Jan 3, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Jan 3, 2025

@bors p=5 rollup=iffy (macos-specific strip, fixes a P-critical regression)

@Noratrieb
Copy link
Member Author

@bors p=15 to jump ahead of rollups

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 3, 2025
Pass objcopy args for stripping on OSX

When `-Cstrip` was changed in rust-lang#131405 to use the bundled rust-objcopy instead of /usr/bin/strip on OSX, strip-like arguments were preserved.

But strip and objcopy are, while being the same binary, different, they have different defaults depending on which binary they are. Notably, strip strips everything by default, and objcopy doesn't strip anything by default.

Additionally, `-S` actually means `--strip-all`, so debuginfo stripped everything and symbols didn't strip anything.

We now correctly pass `--strip-debug` and `--strip-all`.

fixes rust-lang#135028

try-job: aarch64-apple
try-job: dist-aarch64-apple
@bors
Copy link
Contributor

bors commented Jan 3, 2025

⌛ Testing commit 4da3aed with merge 436ab06...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[2025-01-03T13:07:42Z DEBUG collector::compile::benchmark] Benchmark iteration 1/1
[2025-01-03T13:07:42Z INFO  collector::compile::execute] run_rustc with incremental=false, profile=Debug, scenario=Some(Full), patch=None, backend=Llvm, phase=benchmark
[2025-01-03T13:07:42Z DEBUG collector::compile::execute] "\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "rustc" "--manifest-path" "Cargo.toml" "-p" "path+file:///C:/a/_temp/msys64/tmp/.tmpWkhKxW#[email protected]" "--bin" "token-stream-stress-bin" "--" "--wrap-rustc-with" "Eprintln"
[2025-01-03T13:07:43Z INFO  collector::compile::execute] run_rustc with incremental=true, profile=Debug, scenario=Some(IncrFull), patch=None, backend=Llvm, phase=benchmark
[2025-01-03T13:07:43Z DEBUG collector::compile::execute] "\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "rustc" "--manifest-path" "Cargo.toml" "-p" "path+file:///C:/a/_temp/msys64/tmp/.tmpWkhKxW#[email protected]" "--bin" "token-stream-stress-bin" "--" "--wrap-rustc-with" "Eprintln" "-C" "incremental=C:\\a\\_temp\\msys64\\tmp\\.tmpWkhKxW\\incremental-state"
[2025-01-03T13:07:43Z INFO  collector::compile::execute] run_rustc with incremental=true, profile=Debug, scenario=Some(IncrUnchanged), patch=None, backend=Llvm, phase=benchmark
[2025-01-03T13:07:43Z DEBUG collector::compile::execute] "\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "rustc" "--manifest-path" "Cargo.toml" "-p" "path+file:///C:/a/_temp/msys64/tmp/.tmpWkhKxW#[email protected]" "--bin" "token-stream-stress-bin" "--" "--wrap-rustc-with" "Eprintln" "-C" "incremental=C:\\a\\_temp\\msys64\\tmp\\.tmpWkhKxW\\incremental-state"
[2025-01-03T13:07:43Z DEBUG collector::compile::benchmark] Benchmark iteration 1/1
[2025-01-03T13:07:43Z INFO  collector::compile::execute] run_rustc with incremental=false, profile=Opt, scenario=Some(Full), patch=None, backend=Llvm, phase=benchmark
[2025-01-03T13:07:43Z DEBUG collector::compile::execute] "\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "rustc" "--manifest-path" "Cargo.toml" "-p" "path+file:///C:/a/_temp/msys64/tmp/.tmpeSuckL#[email protected]" "--release" "--bin" "token-stream-stress-bin" "--" "--wrap-rustc-with" "Eprintln"
[2025-01-03T13:07:44Z INFO  collector::compile::execute] run_rustc with incremental=true, profile=Opt, scenario=Some(IncrFull), patch=None, backend=Llvm, phase=benchmark
---
   Compiling build_helper v0.1.0 (C:\a\rust\rust\src\build_helper)
   Compiling xz2 v0.1.7
error: linking with `link.exe` failed: exit code: 1104
  |
  = note: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.42.34433\\bin\\HostX64\\x64\\link.exe" "/NOLOGO" "C:\\a\\_temp\\msys64\\tmp\\rustcflV56w\\symbols.o" "<25 object files omitted>" "C:\\a\\rust\\rust\\build\\bootstrap\\debug\\deps/{libcc-d5c8d35fb85b97b9.rlib,libshlex-2473e794facdeeb6.rlib}" "C:\\a\\rust\\rust\\build\\unpacked-dist\\rustc-nightly-x86_64-pc-windows-msvc\\rustc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib/{libstd-e5892338f3c052bf.rlib,libpanic_unwind-e2e1e25e9c29d5c1.rlib,libwindows_targets-650a9189f256d76e.rlib,librustc_demangle-0019317d0b181fcd.rlib,libstd_detect-b572621cbf6f273f.rlib,libhashbrown-e2aed45f16697a5f.rlib,librustc_std_workspace_alloc-2a3ec57dff547df2.rlib,libunwind-79dc983f2b548f92.rlib,libcfg_if-b8ed0a8dbe393c28.rlib,liballoc-ec76c8035bd3de2e.rlib,librustc_std_workspace_core-5a536bba43fbe272.rlib,libcore-45b7a05b67ddb8c6.rlib,libcompiler_builtins-5fe297e6720ad64e.rlib}" "kernel32.lib" "advapi32.lib" "ole32.lib" "oleaut32.lib" "kernel32.lib" "kernel32.lib" "advapi32.lib" "ntdll.lib" "userenv.lib" "ws2_32.lib" "dbghelp.lib" "/defaultlib:msvcrt" "/NXCOMPAT" "/LIBPATH:C:\\Users\\runneradmin\\.cargo\\registry\\src\\index.crates.io-1949cf8c6b5b557f\\windows_x86_64_msvc-0.52.6\\lib" "/LIBPATH:C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.42.34433\\atlmfc\\lib\\x64" "/LIBPATH:C:\\a\\rust\\rust\\build\\bootstrap\\debug\\build\\lzma-sys-673b8c12147494d3\\out" "/OUT:C:\\a\\rust\\rust\\build\\bootstrap\\debug\\deps\\sccache_plus_cl.exe" "/OPT:REF,NOICF" "/DEBUG" "/PDBALTPATH:%_PDB%" "/NATVIS:C:\\a\\rust\\rust\\build\\unpacked-dist\\rustc-nightly-x86_64-pc-windows-msvc\\rustc\\lib\\rustlib\\etc\\intrinsic.natvis" "/NATVIS:C:\\a\\rust\\rust\\build\\unpacked-dist\\rustc-nightly-x86_64-pc-windows-msvc\\rustc\\lib\\rustlib\\etc\\liballoc.natvis" "/NATVIS:C:\\a\\rust\\rust\\build\\unpacked-dist\\rustc-nightly-x86_64-pc-windows-msvc\\rustc\\lib\\rustlib\\etc\\libcore.natvis" "/NATVIS:C:\\a\\rust\\rust\\build\\unpacked-dist\\rustc-nightly-x86_64-pc-windows-msvc\\rustc\\lib\\rustlib\\etc\\libstd.natvis"
  = note: some arguments are omitted. use `--verbose` to show all linker arguments
  = note: LINK : fatal error LNK1104: cannot open file 'C:\a\rust\rust\build\bootstrap\debug\deps\sccache_plus_cl.exe'␍


error: could not compile `bootstrap` (bin "sccache-plus-cl") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
failed to run: C:/a/rust/rust/build/unpacked-dist/cargo-nightly-x86_64-pc-windows-msvc/cargo/bin/cargo.exe build --manifest-path C:\a\rust\rust\src/bootstrap/Cargo.toml -Zroot-dir=C:\a\rust\rust
[2025-01-03T14:12:53.455Z INFO  opt_dist::timer] Section `Run tests` ended: FAIL (42.95s)`
[2025-01-03T14:12:53.455Z INFO  opt_dist] Timer results
    -----------------------------------------------------------------
    Stage 1 (Rustc PGO):                            2749.04s (44.93%)

@bors
Copy link
Contributor

bors commented Jan 3, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 3, 2025
@Noratrieb
Copy link
Member Author

@bors retry get better at deleting files, my friend

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2025
@Noratrieb Noratrieb added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. CI-spurious-fail-msvc CI spurious failure: target env msvc and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 3, 2025
@apiraino
Copy link
Contributor

apiraino commented Jan 3, 2025

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 3, 2025
@bors
Copy link
Contributor

bors commented Jan 3, 2025

⌛ Testing commit 4da3aed with merge 3f43b1a...

@bors
Copy link
Contributor

bors commented Jan 3, 2025

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 3f43b1a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 3, 2025
@bors bors merged commit 3f43b1a into rust-lang:master Jan 3, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 3, 2025
@Noratrieb Noratrieb deleted the strip-correctly branch January 3, 2025 20:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3f43b1a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.5% [-1.5%, -1.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.5% [-1.5%, -1.5%] 1

Cycles

Results (secondary -7.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-7.4% [-8.5%, -6.4%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 762.379s -> 764.245s (0.24%)
Artifact size: 325.63 MiB -> 325.60 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. CI-spurious-fail-msvc CI spurious failure: target env msvc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
8 participants