feat: Strip debug info from opt builds #2513

matte1 · 2024-02-22T23:42:43Z

Attempts to follow the cargo proposal linked below to remove debug info for release builds. Furthermore this should uphold the expected behavior of bazel for cc builds which automatically strips debug symbols for optimized builds.

rust-lang/cargo#4122 (comment)

google-cla · 2024-02-22T23:42:47Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

matte1 · 2024-02-23T00:01:52Z

rust/toolchain.bzl

+        if not k in ctx.attr.strip_level:
+            fail("Compilation mode {} is not defined in strip_level but is defined opt_level".format(k))
+        compilation_mode_opts[k] = struct(debug_info = ctx.attr.debug_info[k], opt_level = opt_level, strip_level = ctx.attr.strip_level[k])
+    for k in ctx.attr.debug_info.keys():


I think v here wasn't actually being used but the linter wasn't catching it because v was defined and used above.

matte1 · 2024-02-23T00:02:15Z

rust/toolchain.bzl

+        "strip_level": attr.string_dict(
+            doc = "Rustc strip levels.",


strip might be more appropriate but there is three levels.

related to #2513 (comment)

Suggested change

"strip_level": attr.string_dict(

doc = "Rustc strip levels.",

"strip_level": attr.string_dict(

doc = "Rustc strip levels. For all potential options, see https://doc.rust-lang.org/rustc/codegen-options/index.html#strip",

@matte1 would you mind applying this or doing something to add this link for users?

riking · 2024-03-05T21:36:29Z

docs/rust_repositories.md

@@ -118,6 +119,7 @@ See `@rules_rust//rust:repositories.bzl` for examples of defining the `@rust_cpu
 | <a id="rust_toolchain-rustfmt"></a>rustfmt |  **Deprecated**: Instead see [rustfmt_toolchain](#rustfmt_toolchain)   | <a href="https://bazel.build/concepts/labels">Label</a> | optional | <code>None</code> |
 | <a id="rust_toolchain-staticlib_ext"></a>staticlib_ext |  The extension for static libraries created from rustc.   | String | required |  |
 | <a id="rust_toolchain-stdlib_linkflags"></a>stdlib_linkflags |  Additional linker flags to use when Rust standard library is linked by a C++ linker (rustc will deal with these automatically). Subject to location expansion with respect to the srcs of the <code>rust_std</code> attribute.   | List of strings | required |  |
+| <a id="rust_toolchain-strip_level"></a>strip_level |  Rustc strip levels.   | <a href="https://bazel.build/rules/lib/dict">Dictionary: String -> String</a> | optional | <code>{"dbg": "none", "fastbuild": "none", "opt": "debuginfo"}</code> |


Documentation should list the three valid values

This is autogenerated I believe.

Can you clarify what you mean it should list the three valid values? I think the
{"dbg": "none", "fastbuild": "none", "opt": "debuginfo"} from the docs here are the three valid values.

riking · 2024-03-05T21:38:27Z

rust/private/rustc.bzl

@@ -958,6 +958,8 @@ def construct_arguments(
    compilation_mode = get_compilation_mode_opts(ctx, toolchain)
    rustc_flags.add(compilation_mode.opt_level, format = "--codegen=opt-level=%s")
    rustc_flags.add(compilation_mode.debug_info, format = "--codegen=debuginfo=%s")
+    if toolchain.target_os.startswith("linux") or toolchain.target_os.startswith("none"):


why is this platform specific, and also that seems like an extremely strange choice of check. why not != macos?

On macOS, debug symbols are stripped not via the linker, but by invoking a separate strip binary. This can have some performance overhead, but it seems that it might not be large - one experiment showed around a 1% slowdown when compiling cargo itself. ehuss has also mentioned some problems with finding the strip binary on macOS.

If you take a peak at the link I posted in the comments they mention this issue with macos. I can add this as comment if you would like.

I've only tested this with bare-metal and linux and was being defence in to applying to everything besides macos. If bazel only ever has linux, none, and macos I can change the if statement to be more concise.

Actually, I looked, it was merged without platform checks and there were no follow up complaints.

Also the Bazel rules should probably pull in its own copy of strip through the toolchain...

Okay I removed the conditional statement to only strip if its linux or bare metal. Do you have a recommendation for how to approach using its own copy of strip? Its not clear to me how strip is actually being called by rustc.

UebelAndre · 2024-03-26T18:05:02Z

Looks like this change needs a rebase.

matte1 · 2024-03-26T19:16:10Z

@UebelAndre can you point me in the right direction for the CI error I'm getting?

UebelAndre · 2024-03-26T20:52:27Z

Looks like a CI infra issue. @meteorcloudy @scentini

I see this on other PRs

UebelAndre

This seems good to me! But would you be able to add a test for this? Something like a suite at //test/unit/toolchain_strip_level that transitions to dbg, fastbuild, and `opt explicitly and assert that you see the new copt where expected for the default case?

meteorcloudy · 2024-03-27T10:14:23Z

Sorry about the breakage, we fixed the CI and I re-triggered your presubmit.

matte1 · 2024-03-27T13:24:20Z

@UebelAndre what did you have in mind for checking that it transitions correctly?

I could do something a little hacky where I compile for copt and then use output of that in a test where I check for debug symbols using file or something? But seems like there is probably a cleaner way to do this test

UebelAndre · 2024-03-27T13:48:45Z

@UebelAndre what did you have in mind for checking that it transitions correctly?

I could do something a little hacky where I compile for copt and then use output of that in a test where I check for debug symbols using file or something? But seems like there is probably a cleaner way to do this test

#2578 is what I was thinking (though this is for opt_level). Something like this but that covers strip_level.

scentini

Looks good modulo @UebelAndre 's request for testing.

matte1 · 2024-03-29T18:20:40Z

@UebelAndre I added a new strip_level test tho it might be cleaner to just include that in the opt_level test. Could go either way here.

It looks like the windows builds are broken. I don't have a windows machine and have little insight here on how it wants to handle the debug symbols which seem to be in a separate file. @UebelAndre @scentini any thoughts here? I could just disable this feature for windows builds but that seems sad.

UebelAndre

Thank you!

UebelAndre · 2024-04-02T13:31:31Z

test/unit/strip_level/strip_level_test_suite.bzl

+        name = "bin",
+        srcs = [":main.rs"],
+        edition = "2021",
+        target_compatible_with = [


What is the failure on windows? I would expect this to "just work" given that this PR is adding plumbing for an existing rustc flag.

(13:36:05) ERROR: C:/b/wmjdfsig/external/rules_rust/util/process_wrapper/BUILD.bazel:31:36: output 'external/rules_rust/util/process_wrapper/process_wrapper.pdb' was not created

(13:36:16) WARNING: Remote Cache: Expected output util/process_wrapper/process_wrapper.pdb was not created locally. java.io.IOException: Expected output util/process_wrapper/process_wrapper.pdb was not created locally. at com.google.devtools.build.lib.remote.RemoteExecutionService.lambda$buildUploadManifestAsync$6(RemoteExecutionService.java:1339) at io.reactivex.rxjava3.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:43) at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4855) at io.reactivex.rxjava3.core.Single.blockingGet(Single.java:3644) at com.google.devtools.build.lib.remote.RemoteExecutionService.buildUploadManifest(RemoteExecutionService.java:1365) at com.google.devtools.build.lib.remote.RemoteExecutionService.uploadOutputs(RemoteExecutionService.java:1420) at com.google.devtools.build.lib.remote.RemoteSpawnCache$1.store(RemoteSpawnCache.java:188) at com.google.devtools.build.lib.exec.AbstractSpawnStrategy.exec(AbstractSpawnStrategy.java:164) at com.google.devtools.build.lib.exec.AbstractSpawnStrategy.exec(AbstractSpawnStrategy.java:119) at com.google.devtools.build.lib.exec.SpawnStrategyResolver.exec(SpawnStrategyResolver.java:45) at com.google.devtools.build.lib.analysis.actions.SpawnAction.execute(SpawnAction.java:261) at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$ActionRunner.executeAction(SkyframeActionExecutor.java:1144) at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$ActionRunner.run(SkyframeActionExecutor.java:1061) at com.google.devtools.build.lib.skyframe.ActionExecutionState.runStateMachine(ActionExecutionState.java:165) at com.google.devtools.build.lib.skyframe.ActionExecutionState.getResultOrDependOnFuture(ActionExecutionState.java:94) at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.executeAction(SkyframeActionExecutor.java:558) at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.checkCacheAndExecuteIfNeeded(ActionExecutionFunction.java:859) at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.computeInternal(ActionExecutionFunction.java:333) at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.compute(ActionExecutionFunction.java:171) at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:461) at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:414) at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(Unknown Source) at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source) at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source) at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) (13:36:16) ERROR: C:/b/bk-windows-x2t8/bazel/rules-rust-rustlang/util/process_wrapper/BUILD.bazel:31:36: output 'util/process_wrapper/process_wrapper.pdb' was not created (13:36:16) ERROR: C:/b/bk-windows-x2t8/bazel/rules-rust-rustlang/util/process_wrapper/BUILD.bazel:31:36: Compiling Rust (without process_wrapper) bin process_wrapper (6 files) [for tool] failed: not all outputs were created or valid (13:36:16) ERROR: C:/b/bk-windows-x2t8/bazel/rules-rust-rustlang/util/process_wrapper/BUILD.bazel:31:36: output 'util/process_wrapper/process_wrapper.pdb' was not created (13:36:16) ERROR: C:/b/bk-windows-x2t8/bazel/rules-rust-rustlang/util/process_wrapper/BUILD.bazel:31:36: Compiling Rust (without process_wrapper) bin process_wrapper (6 files) [for tool] failed: not all outputs were created or valid

https://buildkite.com/bazel/rules-rust-rustlang/builds/11059#018e9f04-6125-4737-b6a6-9cb63922c0af

here is a build with the windows failures.

I think this means there's an additional change required to detect when stripping would be enabled and prevent the capture of the .pdb file on Windows which seems to not get produced when strip-debug-info is enabled.

https://buildkite.com/bazel/rules-rust-rustlang/builds/11060#018e9f1c-bba4-4b8a-a5ca-d6a833a62074

Same errors which makes sense. its trying to declare a file thats not being produced.

Yeah, I think at the very least the defaults should be expected to work on all platforms. So we should fix the windows issue in this change.

See the Rust 1.77.1 release notes where this change is disabled on Windows in Cargo.

@riking is this going to break peoples workflow? We could just not apply the strip flags to windows

Attempts to follow the cargo proposal linked below to remove debug info for release builds. Furthermore this should uphold the expected behavior of bazel for cc builds which automatically strips debug symbols for optimized builds. rust-lang/cargo#4122 (comment)

UebelAndre · 2024-04-02T17:03:07Z

Feel free to ping me when this is ready for review!

Only generate pdb files for windows if the strip level is set to none.

matte1 · 2024-04-02T17:25:05Z

@UebelAndre I think this could use a look now. I had to do some special handling for the pdb tests and I just patterned matched what we did for the strip_level tests.

UebelAndre

Awesome work, thank you so much!

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [rules_rust](https://togithub.com/bazelbuild/rules_rust) | http_archive | patch | `0.41.0` -> `0.41.1` | --- ### Release Notes <details> <summary>bazelbuild/rules_rust (rules_rust)</summary> ### [`v0.41.1`](https://togithub.com/bazelbuild/rules_rust/releases/tag/0.41.1) [Compare Source](https://togithub.com/bazelbuild/rules_rust/compare/0.41.0...0.41.1) ### 0.41.1 ```python load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "rules_rust", integrity = "sha256-mUV3N2A8ORVVZbrm3O9yepAe/Kv4MD2ob9YQhB8aOI8=", urls = ["https://github.com/bazelbuild/rules_rust/releases/download/0.41.1/rules_rust-v0.41.1.tar.gz"], ) ``` Additional documentation can be found at: https://bazelbuild.github.io/rules_rust/#setup #### What's Changed - Add extra_rustc_flags_for_crate_types. by [@granaghan](https://togithub.com/granaghan) in [bazelbuild/rules_rust#2431 - Android jobs should be using LTS Bazel releases by [@UebelAndre](https://togithub.com/UebelAndre) in [bazelbuild/rules_rust#2589 - BUG-FIX: host-triple str for bzl mod by [@ericmcbride](https://togithub.com/ericmcbride) in [bazelbuild/rules_rust#2587 - fix(cargo-bazel): ignore example crates when checking if proc-macro by [@qtica](https://togithub.com/qtica) in [bazelbuild/rules_rust#2596 - Deprecated `rust_bindgen.leak_symbols` by [@UebelAndre](https://togithub.com/UebelAndre) in [bazelbuild/rules_rust#2590 - Update test metadata for crate_universe by [@UebelAndre](https://togithub.com/UebelAndre) in [bazelbuild/rules_rust#2599 - Fixed bug where crate_universe could match aliases to bench/example deps by [@UebelAndre](https://togithub.com/UebelAndre) in [bazelbuild/rules_rust#2600 - Cleanup splicing utils by [@dzbarsky](https://togithub.com/dzbarsky) in [bazelbuild/rules_rust#2564 - Updated repository rules to notify users about non-reproducible repos. by [@UebelAndre](https://togithub.com/UebelAndre) in [bazelbuild/rules_rust#2593 - feat: Strip debug info from opt builds by [@matte1](https://togithub.com/matte1) in [bazelbuild/rules_rust#2513 - Release 0.41.1 by [@UebelAndre](https://togithub.com/UebelAndre) in [bazelbuild/rules_rust#2592 #### New Contributors - [@qtica](https://togithub.com/qtica) made their first contribution in [bazelbuild/rules_rust#2596 - [@matte1](https://togithub.com/matte1) made their first contribution in [bazelbuild/rules_rust#2513 **Full Changelog**: bazelbuild/rules_rust@0.41.0...0.41.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/bazel-contrib/toolchains_llvm).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

matte1 force-pushed the matte1/strip_debug_info branch 3 times, most recently from df91f38 to 8c0fb32 Compare February 22, 2024 23:58

matte1 commented Feb 23, 2024

View reviewed changes

scentini self-requested a review February 23, 2024 10:28

f0rmiga approved these changes Feb 23, 2024

View reviewed changes

riking reviewed Mar 5, 2024

View reviewed changes

DavidZbarsky-at mentioned this pull request Mar 26, 2024

Release 0.41.0 #2569

Merged

matte1 force-pushed the matte1/strip_debug_info branch 3 times, most recently from c3df08f to cf5d057 Compare March 26, 2024 19:13

UebelAndre requested changes Mar 27, 2024

View reviewed changes

scentini approved these changes Mar 27, 2024

View reviewed changes

matte1 force-pushed the matte1/strip_debug_info branch from 42230a6 to 0ad3a2a Compare March 29, 2024 18:19

UebelAndre approved these changes Mar 29, 2024

View reviewed changes

matte1 force-pushed the matte1/strip_debug_info branch 4 times, most recently from 7951636 to 4bf7ab3 Compare April 2, 2024 13:24

UebelAndre enabled auto-merge April 2, 2024 13:30

UebelAndre added this pull request to the merge queue Apr 2, 2024

UebelAndre requested changes Apr 2, 2024

View reviewed changes

UebelAndre removed this pull request from the merge queue due to a manual request Apr 2, 2024

matte1 force-pushed the matte1/strip_debug_info branch 7 times, most recently from 69dde88 to 6897a8e Compare April 2, 2024 16:04

matte1 force-pushed the matte1/strip_debug_info branch 3 times, most recently from ab55ada to 246de8a Compare April 2, 2024 16:57

matte1 added 2 commits April 2, 2024 11:12

Selectively generate pdb

fab9857

Only generate pdb files for windows if the strip level is set to none.

Regenerate documentation

f439777

matte1 force-pushed the matte1/strip_debug_info branch from 160e29e to f439777 Compare April 2, 2024 17:12

UebelAndre approved these changes Apr 2, 2024

View reviewed changes

UebelAndre added this pull request to the merge queue Apr 2, 2024

Merged via the queue into bazelbuild:main with commit b078bbd Apr 2, 2024
3 checks passed

		"strip_level": attr.string_dict(
		doc = "Rustc strip levels.",

feat: Strip debug info from opt builds #2513

feat: Strip debug info from opt builds #2513

Conversation

matte1 commented Feb 22, 2024

google-cla bot commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre commented Mar 26, 2024

matte1 commented Mar 26, 2024

UebelAndre commented Mar 26, 2024

UebelAndre left a comment

Choose a reason for hiding this comment

meteorcloudy commented Mar 27, 2024

matte1 commented Mar 27, 2024

UebelAndre commented Mar 27, 2024

scentini left a comment

Choose a reason for hiding this comment

matte1 commented Mar 29, 2024

UebelAndre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre commented Apr 2, 2024

matte1 commented Apr 2, 2024

UebelAndre left a comment

Choose a reason for hiding this comment

UebelAndre Apr 2, 2024 •

edited