Skip to content

Commit dc78f01

Browse files
committed
[ABLD-464] Strip debug symbols at packaging time (Plan B)
**what this does** Adds a packaging-time filter, `dd_pkg_files_stripped` (in `bazel/rules/dd_packaging/dd_pkg_strip_transform.bzl`), that replaces plain `pkg_files` calls for shipped binaries. It strips object files as they're collected into a package and splits the removed debug info into a parallel `name + "_debug"` sibling target, without requiring any cooperation from the binary's own build rule -- it works directly against prebuilt-file-backed targets like `@agent_binary//:agent`, which is this design's main advantage over the alternative provider/rule-based approach considered in the same design doc (Plan A, implemented separately). A single `_dd_strip_split` action runs strip/objcopy/dsymutil once per file and declares both the "stripped" and "debug" outputs at once; the public `dd_pkg_strip_transform` rule only projects one half of that shared result, so building both the normal package and its debug sibling never doubles the work. File-type detection (ELF/Mach-O/PE) happens at action-execution time in a new Python driver (`dd_strip_driver.py`), since Starlark can't inspect file contents during analysis; the Starlark side only applies a cheap extension/mode-bit heuristic to skip spawning actions for obviously non-binary files. Tool paths come from a new `//bazel/toolchains/dd_strip` toolchain: Linux and Windows source `strip`/`objcopy` off the existing `cc_toolchain`; macOS hardcodes `/usr/bin/strip` and `/usr/bin/dsymutil` (modeled after `//bazel/rules/rewrite_rpath`'s hardcoded-path pattern rather than `//bazel/toolchains/codesign`'s auto-detecting repository_rule, since that pattern generates a `config_setting` per tool and new config_settings need separate design review). Platform semantics match `omnibus-ruby/lib/omnibus/stripper.rb`: Linux does the 3-step objcopy --only-keep-debug / strip / objcopy --add-gnu-debuglink; macOS runs `strip -x` plus `dsymutil` into a `.dSYM` bundle; Windows ships a `strip`'d binary alongside the unstripped original (mingw's toolchain has no `objcopy`, so there's no split-DWARF story there, matching the existing `windows_symbol_stripping_file` behavior). Wired into `packages/agent/product/BUILD.bazel` (the four `dda_built_*_binary` targets, plus a new `all_files_debug` pkg_filegroup) and `packages/installer/windows/BUILD.bazel` (`installer_binary` plus a new `installer_components_debug`). Removed the one-shot `-Cstrip=symbols` rustc flag from `bazel/configs/system_probe_lite.bazelrc`, which used to destroy debug info before packaging ever saw it. Updated `packages/AGENTS.md` and `packages/installer/MIGRATION_PLAN.md` to reflect that symbol stripping is now migrated. **testing** Built and inspected output on macOS (arm64) at each stage: - A throwaway target wrapping `//cmd/loader:loader` confirmed both the "stripped" and "debug_only" modes produce sane output: `strip -x` removed local symbols, `dsymutil` produced a `.dSYM` bundle with a DWARF resource, and building both mode targets together registered the `DdStripSplit` action exactly once (verified in the verbose build log). - `bazel build //packages/agent/product:all_files //packages/agent/product:all_files_debug` and `//packages/installer/windows:whole_distro_tar //packages/installer/windows:installer_components_debug` succeed against the real prebuilt targets (`@agent_binary`, `@trace_agent_binary`, `@process_agent_binary`, `@privateactionrunner_binary`, `@installer_binary`). - Along the way, fixed two real bugs surfaced by this: `Args.add()` can't take a directory (needed `.path` for the `.dSYM` output), and the passthrough path crashed when the declared debug output was a directory but the driver couldn't recognize the input's format (hit when packaging a Linux ELF prebuilt from a macOS host, since the macOS toolchain has no objcopy -- this is a local-only condition, not a bug in the Linux path itself). - `bazel test //bazel/rules/dd_packaging:_dd_packaging_tests` (the existing suite) still passes -- no regression in the unrelated packaging rules this shares a package with. - Could NOT verify on this machine: the actual Linux ELF split (no Linux sandbox/CI available locally -- the `@trace_agent_binary` prebuilt is a real Linux ELF, but macOS's `dd_strip` toolchain has no `objcopy`, so it fell through to passthrough rather than exercising `_strip_elf`), and whether the produced `.dSYM`/`.debug` artifacts are actually useful to a debugger (no symbolication round-trip was attempted). - No unit tests were added for `dd_pkg_strip_transform`/`_dd_strip_split` themselves (analysistest-style, like `dd_packaging_test.bzl`) -- only manual `bazel build` + filesystem inspection. **next steps** - The Linux objcopy path needs validation in CI or a Linux sandbox before this can be trusted end-to-end. - The macOS `dSYM`-splitting approach hasn't been reviewed by the build team; `strip -x` semantics (keeps global symbols) match omnibus but deserve a second look. - Whatever builds `bin/agent/agent` and friends outside Bazel (dda/omnibus) must also stop pre-stripping, or there will be no debug info left for this filter to split off -- that's a separate, out-of-scope dependency for this PR (the Rust/system-probe-lite case is fixed here; the Go/dda case is not). - Plan A (implemented separately, in parallel) builds its own independent copy of the same toolchain shape under `//bazel/toolchains/dd_strip` by design -- deduplicating the two toolchains is a deliberate follow-up, not done here.
1 parent fec92d8 commit dc78f01

12 files changed

Lines changed: 695 additions & 11 deletions

File tree

MODULE.bazel

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,12 @@ register_toolchains(
205205
"//bazel/toolchains/rpath_rewriter:patchelf_toolchain",
206206
)
207207

208+
register_toolchains(
209+
"//bazel/toolchains/dd_strip:dd_strip_linux_toolchain",
210+
"//bazel/toolchains/dd_strip:dd_strip_macos_toolchain",
211+
"//bazel/toolchains/dd_strip:dd_strip_windows_toolchain",
212+
)
213+
208214
find_rpmbuild = use_extension("@rules_pkg//toolchains/rpm:rpmbuild_configure.bzl", "find_system_rpmbuild_bzlmod", dev_dependency = True)
209215
use_repo(find_rpmbuild, "rules_pkg_rpmbuild")
210216

bazel/configs/system_probe_lite.bazelrc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,10 @@
33
build:system-probe-lite-release --@rules_rust//rust/settings:lto=fat # Enable fat LTO
44
build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Copt-level=z # Optimize for size
55
build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Ccodegen-units=1 # Single codegen unit for maximum optimization
6-
build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Cstrip=symbols # Strip debug symbols
76
build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Cpanic=abort # Remove stack unwinding
7+
# NOTE (ABLD-464): symbol stripping used to happen here as a one-shot rustc
8+
# flag (-Cstrip=symbols), which destroyed debug info before packaging ever
9+
# saw it. Stripping is now a packaging-time concern -- see
10+
# //bazel/rules/dd_packaging:dd_pkg_strip_transform.bzl -- so debug info must
11+
# survive the compile step for the debug/stripped split to have anything to
12+
# split. Do not re-add a compile-time strip flag here.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
load("@rules_python//python:py_binary.bzl", "py_binary")
12
load(":dd_packaging_test.bzl", "dd_packaging_test_suite")
23

34
dd_packaging_test_suite(name = "_dd_packaging_tests")
5+
6+
py_binary(
7+
name = "dd_strip_driver",
8+
srcs = ["dd_strip_driver.py"],
9+
visibility = ["//visibility:public"],
10+
)
Lines changed: 304 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,304 @@
1+
"""dd_pkg_strip_transform — packaging-time strip/debug-split filter (ABLD-464 Plan B).
2+
3+
Consumes a PackageFilesInfo (the same `dest_src_map: {dest_path: File}`
4+
structure dd_cc_packaged.bzl and dd_collect_dependencies.bzl already operate
5+
on) and emits a new PackageFilesInfo pointing at transformed files:
6+
7+
- mode = "stripped" (default): object files have symbol tables/debug info
8+
removed; everything else (configs, licenses, scripts, directories, ...)
9+
passes through byte-for-byte unchanged.
10+
- mode = "debug_only": only the split-off debug artifacts remain --
11+
non-object-file entries are dropped from the dest_src_map entirely, since
12+
e.g. a config file has no debug info and should not appear in the
13+
debug-only sibling package.
14+
15+
Because this operates on the already-flattened, merged dest_src_map (after
16+
pkg_files has resolved srcs to Files), it needs no cooperation from the
17+
binary's own build rule -- it works against `prebuilt_file`-backed filegroups
18+
like `@agent_binary//:agent` today, which is this design's main advantage
19+
over the provider/rule-based alternative (see the ABLD-464 design doc).
20+
21+
IMPLEMENTATION NOTE on avoiding double work: the "stripped" and "debug_only"
22+
modes must not each independently run strip/objcopy/dsymutil on the same
23+
file -- that would double the work for every packaging build that produces
24+
both a package and its debug sibling. To guarantee this, the actual
25+
strip/split actions live in the private `_dd_strip_split` rule (one action
26+
per file, declaring BOTH outputs at once); `dd_pkg_strip_transform` itself
27+
creates no actions of its own -- it only *selects* which half of
28+
`_dd_strip_split`'s already-declared outputs to expose. As long as both the
29+
"stripped" and "debug_only" `dd_pkg_strip_transform` instances point at the
30+
same `_dd_strip_split` target (which `dd_pkg_files_stripped` guarantees by
31+
construction), Bazel analyzes that shared target once, so the action runs at
32+
most once per file no matter how many of its consumers are built.
33+
34+
File-type detection (ELF vs Mach-O vs PE vs "not an object file at all") is
35+
necessarily a runtime concern -- Starlark cannot inspect file contents during
36+
the analysis phase -- so it happens in dd_strip_driver.py, not here. This
37+
file's `_looks_like_object_file` is only a cheap, best-effort Starlark-side
38+
heuristic (extension + the pkg_files group's executable-bit attribute) used
39+
to skip spawning an action at all for files that are obviously not object
40+
code; the driver script is the authoritative fallback for anything else.
41+
"""
42+
43+
load("@rules_pkg//pkg:mappings.bzl", "pkg_files")
44+
load("@rules_pkg//pkg:providers.bzl", "PackageFilesInfo")
45+
46+
_DD_STRIP_TOOLCHAIN = "//bazel/toolchains/dd_strip:toolchain_type"
47+
48+
# Extensions that are unambiguously not machine code. Skipping these avoids
49+
# spawning a strip action (and, in debug_only mode, a spurious dest_src_map
50+
# entry) for the configs/licenses/docs/scripts that make up most of a
51+
# package tree. This is only a fast-path optimization -- see the module
52+
# docstring; dd_strip_driver.py's magic-byte sniffing is authoritative.
53+
_NEVER_BINARY_EXTENSIONS = (
54+
".yaml",
55+
".yml",
56+
".json",
57+
".txt",
58+
".md",
59+
".cfg",
60+
".conf",
61+
".ini",
62+
".py",
63+
".sh",
64+
".rb",
65+
".pem",
66+
".crt",
67+
".key",
68+
".png",
69+
".svg",
70+
".html",
71+
".example",
72+
".license",
73+
)
74+
75+
_ALWAYS_BINARY_SUFFIXES = (".so", ".dylib", ".dll", ".exe")
76+
77+
def _looks_like_object_file(dest, executable):
78+
"""Best-effort guess at whether `dest` is worth spawning a strip action for.
79+
80+
Errs toward "maybe" -- a false positive here just costs a wasted action
81+
that the driver script turns into a no-op passthrough. A false negative
82+
would silently skip stripping a real binary, so extensionless files
83+
(typical for Go/Rust/C binaries shipped as e.g. "bin/agent/agent") are
84+
treated as plausible whenever the enclosing pkg_files group is marked
85+
executable.
86+
"""
87+
lower = dest.lower()
88+
if lower.endswith(_ALWAYS_BINARY_SUFFIXES) or ".so." in lower:
89+
return True
90+
for ext in _NEVER_BINARY_EXTENSIONS:
91+
if lower.endswith(ext):
92+
return False
93+
basename = lower.rsplit("/", 1)[-1]
94+
if "." not in basename:
95+
return True
96+
return executable
97+
98+
_DdStripSplitInfo = provider(
99+
doc = "Internal: the per-mode dest_src_maps produced by one shared strip/split pass.",
100+
fields = {
101+
"stripped_dest_src_map": "dict: dest path -> stripped File, for every entry in the source PackageFilesInfo.",
102+
"debug_dest_src_map": "dict: dest path -> debug File/directory, for entries that were actually strippable object files.",
103+
"attributes": "the source PackageFilesInfo.attributes, forwarded as-is.",
104+
"debug_attributes": "attributes to apply to the debug_only dest_src_map (mode forced non-executable).",
105+
},
106+
)
107+
108+
def _debug_attributes(attributes):
109+
# Debug artifacts (.debug files / .dSYM bundles / unstripped originals)
110+
# are inspected by debuggers, not executed -- ship them non-executable
111+
# regardless of what mode the shipped binary itself uses.
112+
result = dict(attributes)
113+
result["mode"] = "0644"
114+
return result
115+
116+
def _dd_strip_split_impl(ctx):
117+
toolchain = ctx.toolchains[_DD_STRIP_TOOLCHAIN]
118+
strip_info = toolchain.dd_strip_info if toolchain else None
119+
src_info = ctx.attr.src[PackageFilesInfo]
120+
executable = "x" in src_info.attributes.get("mode", "")
121+
122+
# macOS debug output is a dsymutil .dSYM bundle (a directory); Linux's
123+
# objcopy --only-keep-debug and Windows' "copy of the unstripped
124+
# original" are both single files.
125+
debug_is_dir = bool(strip_info and strip_info.dsymutil_path)
126+
debug_suffix = ".dSYM" if debug_is_dir else (".debug" if strip_info and strip_info.objcopy_path else "")
127+
128+
stripped_dest_src_map = {}
129+
debug_dest_src_map = {}
130+
all_outputs = []
131+
132+
for dest, file in src_info.dest_src_map.items():
133+
# Directories (TreeArtifacts) aren't split element-by-element by this
134+
# rule; ship them unstripped and exclude them from the debug sibling.
135+
# This is a known limitation of the packaging-time-filter approach --
136+
# see the ABLD-464 design doc.
137+
if file.is_directory or not _looks_like_object_file(dest, executable) or strip_info == None:
138+
stripped_dest_src_map[dest] = file
139+
continue
140+
141+
stripped_out = ctx.actions.declare_file("dd_strip/stripped/" + dest)
142+
if debug_is_dir:
143+
debug_out = ctx.actions.declare_directory("dd_strip/debug/" + dest + debug_suffix)
144+
else:
145+
debug_out = ctx.actions.declare_file("dd_strip/debug/" + dest + debug_suffix)
146+
147+
args = ctx.actions.args()
148+
args.add("--input", file)
149+
args.add("--stripped-out", stripped_out)
150+
args.add("--debug-out", debug_out.path)
151+
if debug_is_dir:
152+
args.add("--debug-out-is-dir")
153+
if strip_info.strip_path:
154+
args.add("--strip", strip_info.strip_path)
155+
if strip_info.objcopy_path:
156+
args.add("--objcopy", strip_info.objcopy_path)
157+
if strip_info.dsymutil_path:
158+
args.add("--dsymutil", strip_info.dsymutil_path)
159+
160+
ctx.actions.run(
161+
executable = ctx.executable._driver,
162+
arguments = [args],
163+
inputs = depset([file], transitive = [strip_info.tool_files]),
164+
outputs = [stripped_out, debug_out],
165+
mnemonic = "DdStripSplit",
166+
progress_message = "Splitting debug symbols from %s" % dest,
167+
toolchain = _DD_STRIP_TOOLCHAIN,
168+
)
169+
170+
stripped_dest_src_map[dest] = stripped_out
171+
debug_dest_src_map[dest] = debug_out
172+
all_outputs.extend([stripped_out, debug_out])
173+
174+
return [
175+
_DdStripSplitInfo(
176+
stripped_dest_src_map = stripped_dest_src_map,
177+
debug_dest_src_map = debug_dest_src_map,
178+
attributes = src_info.attributes,
179+
debug_attributes = _debug_attributes(src_info.attributes),
180+
),
181+
DefaultInfo(files = depset(all_outputs)),
182+
]
183+
184+
_dd_strip_split = rule(
185+
implementation = _dd_strip_split_impl,
186+
doc = """Internal: runs the actual strip/split action once per (strippable) file.
187+
188+
Not meant to be used directly -- use dd_pkg_files_stripped. Kept separate
189+
from dd_pkg_strip_transform so that "stripped" and "debug_only" mode
190+
instances can share a single set of actions; see the module docstring.
191+
""",
192+
attrs = {
193+
"src": attr.label(
194+
mandatory = True,
195+
providers = [PackageFilesInfo],
196+
),
197+
"_driver": attr.label(
198+
default = Label("//bazel/rules/dd_packaging:dd_strip_driver"),
199+
executable = True,
200+
cfg = "exec",
201+
),
202+
},
203+
toolchains = [config_common.toolchain_type(_DD_STRIP_TOOLCHAIN, mandatory = False)],
204+
)
205+
206+
def _dd_pkg_strip_transform_impl(ctx):
207+
split_info = ctx.attr.split[_DdStripSplitInfo]
208+
if ctx.attr.mode == "debug_only":
209+
dest_src_map = split_info.debug_dest_src_map
210+
attributes = split_info.debug_attributes
211+
else:
212+
dest_src_map = dict(split_info.stripped_dest_src_map)
213+
attributes = split_info.attributes
214+
215+
return [
216+
PackageFilesInfo(
217+
dest_src_map = dest_src_map,
218+
attributes = attributes,
219+
),
220+
# Without an explicit DefaultInfo, `bazel build`/`cquery --output=files`
221+
# against this target directly would request the (empty) implicit
222+
# default output group and never actually force the underlying
223+
# _dd_strip_split action to run. Packaging rules only look at
224+
# PackageFilesInfo, but this makes `bazel build` on the label alone
225+
# (as in local iteration, or a debug-package pkg_filegroup) do
226+
# something observable.
227+
DefaultInfo(files = depset(dest_src_map.values())),
228+
]
229+
230+
dd_pkg_strip_transform = rule(
231+
implementation = _dd_pkg_strip_transform_impl,
232+
doc = """Projects one mode's worth of a shared _dd_strip_split's outputs into a PackageFilesInfo.
233+
234+
Not meant to be used directly -- use dd_pkg_files_stripped.
235+
""",
236+
attrs = {
237+
"split": attr.label(
238+
mandatory = True,
239+
providers = [_DdStripSplitInfo],
240+
),
241+
"mode": attr.string(
242+
mandatory = True,
243+
values = ["stripped", "debug_only"],
244+
),
245+
},
246+
provides = [PackageFilesInfo],
247+
)
248+
249+
def dd_pkg_files_stripped(name, srcs, prefix = "", mode = "stripped", **kwargs):
250+
"""A pkg_files-alike whose binaries are stripped at packaging time.
251+
252+
Behaves like `pkg_files(name, srcs, prefix, **kwargs)`, except object
253+
files (recognized as ELF/Mach-O/PE by dd_strip_driver.py) have their
254+
debug info split off instead of shipping unmodified. `name` carries the
255+
requested `mode`'s worth of the result (default "stripped", i.e. the
256+
normal packaging behavior with debug info removed); a `name + "_debug"`
257+
sibling target is always created as well (mode="debug_only"), containing
258+
only the split-off debug artifacts -- reference that label from a debug
259+
package's pkg_filegroup. Both labels are backed by the same underlying
260+
strip/split actions, so building both never runs strip/objcopy/dsymutil
261+
twice on the same file (see dd_pkg_strip_transform.bzl for why that
262+
matters).
263+
264+
Args:
265+
name: name of the "stripped"-mode target (or whatever `mode` requests).
266+
srcs: same as pkg_files' srcs.
267+
prefix: same as pkg_files' prefix.
268+
mode: "stripped" (default) or "debug_only". Only pass "debug_only"
269+
directly if you don't need the normal stripped-package variant
270+
at all -- in that case no "_debug" sibling is created, since
271+
`name` already is the debug-only variant.
272+
**kwargs: forwarded to the underlying pkg_files call (e.g. attributes).
273+
"""
274+
files_name = name + "_pkg_files"
275+
split_name = name + "_split"
276+
277+
pkg_files(
278+
name = files_name,
279+
srcs = srcs,
280+
prefix = prefix,
281+
tags = ["manual"],
282+
visibility = ["//visibility:private"],
283+
**kwargs
284+
)
285+
286+
_dd_strip_split(
287+
name = split_name,
288+
src = ":" + files_name,
289+
tags = ["manual"],
290+
visibility = ["//visibility:private"],
291+
)
292+
293+
dd_pkg_strip_transform(
294+
name = name,
295+
split = ":" + split_name,
296+
mode = mode,
297+
)
298+
299+
if mode != "debug_only":
300+
dd_pkg_strip_transform(
301+
name = name + "_debug",
302+
split = ":" + split_name,
303+
mode = "debug_only",
304+
)

0 commit comments

Comments
 (0)