diff --git a/MODULE.bazel b/MODULE.bazel index 519387366b87..d3ba1bd3d329 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -205,6 +205,12 @@ register_toolchains( "//bazel/toolchains/rpath_rewriter:patchelf_toolchain", ) +register_toolchains( + "//bazel/toolchains/dd_strip:dd_strip_linux_toolchain", + "//bazel/toolchains/dd_strip:dd_strip_macos_toolchain", + "//bazel/toolchains/dd_strip:dd_strip_windows_toolchain", +) + find_rpmbuild = use_extension("@rules_pkg//toolchains/rpm:rpmbuild_configure.bzl", "find_system_rpmbuild_bzlmod", dev_dependency = True) use_repo(find_rpmbuild, "rules_pkg_rpmbuild") diff --git a/bazel/configs/system_probe_lite.bazelrc b/bazel/configs/system_probe_lite.bazelrc index 1ecc123f72cb..591b659f8023 100644 --- a/bazel/configs/system_probe_lite.bazelrc +++ b/bazel/configs/system_probe_lite.bazelrc @@ -3,5 +3,10 @@ build:system-probe-lite-release --@rules_rust//rust/settings:lto=fat # Enable fat LTO build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Copt-level=z # Optimize for size build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Ccodegen-units=1 # Single codegen unit for maximum optimization -build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Cstrip=symbols # Strip debug symbols build:system-probe-lite-release --@rules_rust//rust/settings:extra_rustc_flag=-Cpanic=abort # Remove stack unwinding +# NOTE (ABLD-464): symbol stripping used to happen here as a one-shot rustc +# flag (-Cstrip=symbols), which destroyed debug info before packaging ever +# saw it. Stripping is now a packaging-time concern -- see +# //bazel/rules/dd_packaging:dd_pkg_strip_transform.bzl -- so debug info must +# survive the compile step for the debug/stripped split to have anything to +# split. Do not re-add a compile-time strip flag here. diff --git a/bazel/rules/dd_packaging/BUILD.bazel b/bazel/rules/dd_packaging/BUILD.bazel index 11e9c022363b..9465ce5a188b 100644 --- a/bazel/rules/dd_packaging/BUILD.bazel +++ b/bazel/rules/dd_packaging/BUILD.bazel @@ -1,3 +1,10 @@ +load("@rules_python//python:py_binary.bzl", "py_binary") load(":dd_packaging_test.bzl", "dd_packaging_test_suite") dd_packaging_test_suite(name = "_dd_packaging_tests") + +py_binary( + name = "dd_strip_driver", + srcs = ["dd_strip_driver.py"], + visibility = ["//visibility:public"], +) diff --git a/bazel/rules/dd_packaging/dd_pkg_strip_transform.bzl b/bazel/rules/dd_packaging/dd_pkg_strip_transform.bzl new file mode 100644 index 000000000000..0ec89bf14c47 --- /dev/null +++ b/bazel/rules/dd_packaging/dd_pkg_strip_transform.bzl @@ -0,0 +1,304 @@ +"""dd_pkg_strip_transform — packaging-time strip/debug-split filter (ABLD-464 Plan B). + +Consumes a PackageFilesInfo (the same `dest_src_map: {dest_path: File}` +structure dd_cc_packaged.bzl and dd_collect_dependencies.bzl already operate +on) and emits a new PackageFilesInfo pointing at transformed files: + + - mode = "stripped" (default): object files have symbol tables/debug info + removed; everything else (configs, licenses, scripts, directories, ...) + passes through byte-for-byte unchanged. + - mode = "debug_only": only the split-off debug artifacts remain -- + non-object-file entries are dropped from the dest_src_map entirely, since + e.g. a config file has no debug info and should not appear in the + debug-only sibling package. + +Because this operates on the already-flattened, merged dest_src_map (after +pkg_files has resolved srcs to Files), it needs no cooperation from the +binary's own build rule -- it works against `prebuilt_file`-backed filegroups +like `@agent_binary//:agent` today, which is this design's main advantage +over the provider/rule-based alternative (see the ABLD-464 design doc). + +IMPLEMENTATION NOTE on avoiding double work: the "stripped" and "debug_only" +modes must not each independently run strip/objcopy/dsymutil on the same +file -- that would double the work for every packaging build that produces +both a package and its debug sibling. To guarantee this, the actual +strip/split actions live in the private `_dd_strip_split` rule (one action +per file, declaring BOTH outputs at once); `dd_pkg_strip_transform` itself +creates no actions of its own -- it only *selects* which half of +`_dd_strip_split`'s already-declared outputs to expose. As long as both the +"stripped" and "debug_only" `dd_pkg_strip_transform` instances point at the +same `_dd_strip_split` target (which `dd_pkg_files_stripped` guarantees by +construction), Bazel analyzes that shared target once, so the action runs at +most once per file no matter how many of its consumers are built. + +File-type detection (ELF vs Mach-O vs PE vs "not an object file at all") is +necessarily a runtime concern -- Starlark cannot inspect file contents during +the analysis phase -- so it happens in dd_strip_driver.py, not here. This +file's `_looks_like_object_file` is only a cheap, best-effort Starlark-side +heuristic (extension + the pkg_files group's executable-bit attribute) used +to skip spawning an action at all for files that are obviously not object +code; the driver script is the authoritative fallback for anything else. +""" + +load("@rules_pkg//pkg:mappings.bzl", "pkg_files") +load("@rules_pkg//pkg:providers.bzl", "PackageFilesInfo") + +_DD_STRIP_TOOLCHAIN = "//bazel/toolchains/dd_strip:toolchain_type" + +# Extensions that are unambiguously not machine code. Skipping these avoids +# spawning a strip action (and, in debug_only mode, a spurious dest_src_map +# entry) for the configs/licenses/docs/scripts that make up most of a +# package tree. This is only a fast-path optimization -- see the module +# docstring; dd_strip_driver.py's magic-byte sniffing is authoritative. +_NEVER_BINARY_EXTENSIONS = ( + ".yaml", + ".yml", + ".json", + ".txt", + ".md", + ".cfg", + ".conf", + ".ini", + ".py", + ".sh", + ".rb", + ".pem", + ".crt", + ".key", + ".png", + ".svg", + ".html", + ".example", + ".license", +) + +_ALWAYS_BINARY_SUFFIXES = (".so", ".dylib", ".dll", ".exe") + +def _looks_like_object_file(dest, executable): + """Best-effort guess at whether `dest` is worth spawning a strip action for. + + Errs toward "maybe" -- a false positive here just costs a wasted action + that the driver script turns into a no-op passthrough. A false negative + would silently skip stripping a real binary, so extensionless files + (typical for Go/Rust/C binaries shipped as e.g. "bin/agent/agent") are + treated as plausible whenever the enclosing pkg_files group is marked + executable. + """ + lower = dest.lower() + if lower.endswith(_ALWAYS_BINARY_SUFFIXES) or ".so." in lower: + return True + for ext in _NEVER_BINARY_EXTENSIONS: + if lower.endswith(ext): + return False + basename = lower.rsplit("/", 1)[-1] + if "." not in basename: + return True + return executable + +_DdStripSplitInfo = provider( + doc = "Internal: the per-mode dest_src_maps produced by one shared strip/split pass.", + fields = { + "stripped_dest_src_map": "dict: dest path -> stripped File, for every entry in the source PackageFilesInfo.", + "debug_dest_src_map": "dict: dest path -> debug File/directory, for entries that were actually strippable object files.", + "attributes": "the source PackageFilesInfo.attributes, forwarded as-is.", + "debug_attributes": "attributes to apply to the debug_only dest_src_map (mode forced non-executable).", + }, +) + +def _debug_attributes(attributes): + # Debug artifacts (.debug files / .dSYM bundles / unstripped originals) + # are inspected by debuggers, not executed -- ship them non-executable + # regardless of what mode the shipped binary itself uses. + result = dict(attributes) + result["mode"] = "0644" + return result + +def _dd_strip_split_impl(ctx): + toolchain = ctx.toolchains[_DD_STRIP_TOOLCHAIN] + strip_info = toolchain.dd_strip_info if toolchain else None + src_info = ctx.attr.src[PackageFilesInfo] + executable = "x" in src_info.attributes.get("mode", "") + + # macOS debug output is a dsymutil .dSYM bundle (a directory); Linux's + # objcopy --only-keep-debug and Windows' "copy of the unstripped + # original" are both single files. + debug_is_dir = bool(strip_info and strip_info.dsymutil_path) + debug_suffix = ".dSYM" if debug_is_dir else (".debug" if strip_info and strip_info.objcopy_path else "") + + stripped_dest_src_map = {} + debug_dest_src_map = {} + all_outputs = [] + + for dest, file in src_info.dest_src_map.items(): + # Directories (TreeArtifacts) aren't split element-by-element by this + # rule; ship them unstripped and exclude them from the debug sibling. + # This is a known limitation of the packaging-time-filter approach -- + # see the ABLD-464 design doc. + if file.is_directory or not _looks_like_object_file(dest, executable) or strip_info == None: + stripped_dest_src_map[dest] = file + continue + + stripped_out = ctx.actions.declare_file("dd_strip/stripped/" + dest) + if debug_is_dir: + debug_out = ctx.actions.declare_directory("dd_strip/debug/" + dest + debug_suffix) + else: + debug_out = ctx.actions.declare_file("dd_strip/debug/" + dest + debug_suffix) + + args = ctx.actions.args() + args.add("--input", file) + args.add("--stripped-out", stripped_out) + args.add("--debug-out", debug_out.path) + if debug_is_dir: + args.add("--debug-out-is-dir") + if strip_info.strip_path: + args.add("--strip", strip_info.strip_path) + if strip_info.objcopy_path: + args.add("--objcopy", strip_info.objcopy_path) + if strip_info.dsymutil_path: + args.add("--dsymutil", strip_info.dsymutil_path) + + ctx.actions.run( + executable = ctx.executable._driver, + arguments = [args], + inputs = depset([file], transitive = [strip_info.tool_files]), + outputs = [stripped_out, debug_out], + mnemonic = "DdStripSplit", + progress_message = "Splitting debug symbols from %s" % dest, + toolchain = _DD_STRIP_TOOLCHAIN, + ) + + stripped_dest_src_map[dest] = stripped_out + debug_dest_src_map[dest] = debug_out + all_outputs.extend([stripped_out, debug_out]) + + return [ + _DdStripSplitInfo( + stripped_dest_src_map = stripped_dest_src_map, + debug_dest_src_map = debug_dest_src_map, + attributes = src_info.attributes, + debug_attributes = _debug_attributes(src_info.attributes), + ), + DefaultInfo(files = depset(all_outputs)), + ] + +_dd_strip_split = rule( + implementation = _dd_strip_split_impl, + doc = """Internal: runs the actual strip/split action once per (strippable) file. + + Not meant to be used directly -- use dd_pkg_files_stripped. Kept separate + from dd_pkg_strip_transform so that "stripped" and "debug_only" mode + instances can share a single set of actions; see the module docstring. + """, + attrs = { + "src": attr.label( + mandatory = True, + providers = [PackageFilesInfo], + ), + "_driver": attr.label( + default = Label("//bazel/rules/dd_packaging:dd_strip_driver"), + executable = True, + cfg = "exec", + ), + }, + toolchains = [config_common.toolchain_type(_DD_STRIP_TOOLCHAIN, mandatory = False)], +) + +def _dd_pkg_strip_transform_impl(ctx): + split_info = ctx.attr.split[_DdStripSplitInfo] + if ctx.attr.mode == "debug_only": + dest_src_map = split_info.debug_dest_src_map + attributes = split_info.debug_attributes + else: + dest_src_map = dict(split_info.stripped_dest_src_map) + attributes = split_info.attributes + + return [ + PackageFilesInfo( + dest_src_map = dest_src_map, + attributes = attributes, + ), + # Without an explicit DefaultInfo, `bazel build`/`cquery --output=files` + # against this target directly would request the (empty) implicit + # default output group and never actually force the underlying + # _dd_strip_split action to run. Packaging rules only look at + # PackageFilesInfo, but this makes `bazel build` on the label alone + # (as in local iteration, or a debug-package pkg_filegroup) do + # something observable. + DefaultInfo(files = depset(dest_src_map.values())), + ] + +dd_pkg_strip_transform = rule( + implementation = _dd_pkg_strip_transform_impl, + doc = """Projects one mode's worth of a shared _dd_strip_split's outputs into a PackageFilesInfo. + + Not meant to be used directly -- use dd_pkg_files_stripped. + """, + attrs = { + "split": attr.label( + mandatory = True, + providers = [_DdStripSplitInfo], + ), + "mode": attr.string( + mandatory = True, + values = ["stripped", "debug_only"], + ), + }, + provides = [PackageFilesInfo], +) + +def dd_pkg_files_stripped(name, srcs, prefix = "", mode = "stripped", **kwargs): + """A pkg_files-alike whose binaries are stripped at packaging time. + + Behaves like `pkg_files(name, srcs, prefix, **kwargs)`, except object + files (recognized as ELF/Mach-O/PE by dd_strip_driver.py) have their + debug info split off instead of shipping unmodified. `name` carries the + requested `mode`'s worth of the result (default "stripped", i.e. the + normal packaging behavior with debug info removed); a `name + "_debug"` + sibling target is always created as well (mode="debug_only"), containing + only the split-off debug artifacts -- reference that label from a debug + package's pkg_filegroup. Both labels are backed by the same underlying + strip/split actions, so building both never runs strip/objcopy/dsymutil + twice on the same file (see dd_pkg_strip_transform.bzl for why that + matters). + + Args: + name: name of the "stripped"-mode target (or whatever `mode` requests). + srcs: same as pkg_files' srcs. + prefix: same as pkg_files' prefix. + mode: "stripped" (default) or "debug_only". Only pass "debug_only" + directly if you don't need the normal stripped-package variant + at all -- in that case no "_debug" sibling is created, since + `name` already is the debug-only variant. + **kwargs: forwarded to the underlying pkg_files call (e.g. attributes). + """ + files_name = name + "_pkg_files" + split_name = name + "_split" + + pkg_files( + name = files_name, + srcs = srcs, + prefix = prefix, + tags = ["manual"], + visibility = ["//visibility:private"], + **kwargs + ) + + _dd_strip_split( + name = split_name, + src = ":" + files_name, + tags = ["manual"], + visibility = ["//visibility:private"], + ) + + dd_pkg_strip_transform( + name = name, + split = ":" + split_name, + mode = mode, + ) + + if mode != "debug_only": + dd_pkg_strip_transform( + name = name + "_debug", + split = ":" + split_name, + mode = "debug_only", + ) diff --git a/bazel/rules/dd_packaging/dd_strip_driver.py b/bazel/rules/dd_packaging/dd_strip_driver.py new file mode 100644 index 000000000000..921107ed54a3 --- /dev/null +++ b/bazel/rules/dd_packaging/dd_strip_driver.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +"""Driver for dd_pkg_strip_transform: strips a binary and splits off debug info. + +One invocation always produces BOTH the "stripped" and "debug" outputs for a +single input file -- dd_pkg_strip_transform.bzl registers exactly one of +these actions per file and lets its "stripped"/"debug_only" rule instances +pick which declared output they reference, so the actual strip/objcopy/ +dsymutil work never runs twice for the same file. + +File-type detection is a runtime decision on purpose: Starlark cannot read +file contents during the analysis phase, so this script -- which inspects +actual magic bytes -- is the authoritative source of truth for "is this +really an ELF/Mach-O/PE object file". The Starlark side only applies a cheap +extension/mode-bit heuristic to avoid spawning this action at all for files +that are obviously not object code (configs, licenses, docs); anything that +heuristic lets through but that turns out not to be a recognized binary +format is passed through unchanged here. + +Platform semantics (matching omnibus's lib/omnibus/stripper.rb): + - ELF (Linux): objcopy --only-keep-debug -> debug-out + strip --strip-debug --strip-unneeded (on a copy) + objcopy --add-gnu-debuglink=debug-out -> stripped-out + - Mach-O (macOS): dsymutil -> debug-out (a .dSYM bundle directory) + strip -x (in place on a copy) -> stripped-out + - PE (Windows): debug-out = the unstripped original (no split-DWARF story + for this toolchain, matching omnibus's + windows_symbol_stripping_file); stripped-out = strip'd copy. + - Anything else: passthrough. stripped-out is a copy of the input; + debug-out is an empty marker file (there is no debug info + to extract from e.g. a shell script or config file). +""" + +import argparse +import os +import shutil +import subprocess +import sys + +_ELF_MAGIC = b"\x7fELF" +_MACHO_MAGICS = ( + b"\xfe\xed\xfa\xce", # 32-bit, big-endian + b"\xce\xfa\xed\xfe", # 32-bit, little-endian + b"\xfe\xed\xfa\xcf", # 64-bit, big-endian + b"\xcf\xfa\xed\xfe", # 64-bit, little-endian + b"\xca\xfe\xba\xbe", # fat/universal, big-endian + b"\xbe\xba\xfe\xca", # fat/universal, little-endian +) +_PE_MAGIC = b"MZ" + + +def _detect_format(path): + try: + with open(path, "rb") as f: + head = f.read(4) + except OSError: + return None + if head.startswith(_ELF_MAGIC): + return "elf" + if head in _MACHO_MAGICS: + return "macho" + if head[:2] == _PE_MAGIC: + return "pe" + return None + + +def _run(cmd): + subprocess.run(cmd, check=True) + + +def _make_writable(path): + # Bazel declares action outputs read-only until the action completes + # (and the source input itself may be non-writable, e.g. a read-only + # `pkg_files`-collected binary). `copymode` below faithfully carries that + # bit over, which then makes in-place tools like `strip` fail with + # "Permission denied" on the copy. Force the write bit before invoking + # any tool that modifies the copy in place; final permissions are always + # reset from the original input via `shutil.copymode` afterwards. + os.chmod(path, os.stat(path).st_mode | 0o200) + + +def _passthrough(input_path, stripped_out, debug_out, debug_out_is_dir): + # Reached whenever the input isn't a format this driver recognizes as + # strippable -- either because it genuinely isn't (a script, a config + # file that slipped past the Starlark-side heuristic) or because it's an + # object file for a platform the *current* dd_strip toolchain doesn't + # know how to strip (e.g. a Linux ELF prebuilt binary being packaged from + # a macOS host during local iteration, where the toolchain has no + # objcopy). Either way there is nothing to split off, so ship the input + # unmodified and leave an empty placeholder as the "debug" artifact. + # `debug_out`'s declared shape (file vs. directory) is fixed by the + # Starlark side before the input is ever inspected, based solely on + # which toolchain is in play -- so this has to honor whichever shape was + # requested rather than assuming a file. + shutil.copyfile(input_path, stripped_out) + shutil.copymode(input_path, stripped_out) + if debug_out_is_dir: + os.makedirs(debug_out, exist_ok=True) + else: + open(debug_out, "wb").close() + + +def _strip_elf(input_path, stripped_out, debug_out, strip, objcopy): + if not objcopy: + sys.exit("dd_strip_driver: ELF input but no objcopy tool is configured") + if not strip: + sys.exit("dd_strip_driver: ELF input but no strip tool is configured") + tmp_stripped = stripped_out + ".tmp" + _run([objcopy, "--only-keep-debug", input_path, debug_out]) + shutil.copyfile(input_path, tmp_stripped) + _make_writable(tmp_stripped) + _run([strip, "--strip-debug", "--strip-unneeded", tmp_stripped]) + _run([objcopy, "--add-gnu-debuglink=" + debug_out, tmp_stripped, stripped_out]) + shutil.copymode(input_path, stripped_out) + os.remove(tmp_stripped) + + +def _strip_macho(input_path, stripped_out, debug_out, strip): + # dsymutil is invoked by the caller before this script runs on macOS -- + # see the note in dd_pkg_strip_transform.bzl about declare_directory + # outputs needing to not already exist when dsymutil starts. + if not strip: + sys.exit("dd_strip_driver: Mach-O input but no strip tool is configured") + shutil.copyfile(input_path, stripped_out) + _make_writable(stripped_out) + _run([strip, "-x", stripped_out]) + shutil.copymode(input_path, stripped_out) + _ = debug_out # already populated by the caller's dsymutil invocation + + +def _strip_pe(input_path, stripped_out, debug_out, strip): + shutil.copyfile(input_path, debug_out) + shutil.copymode(input_path, debug_out) + shutil.copyfile(input_path, stripped_out) + if strip: + _make_writable(stripped_out) + _run([strip, stripped_out]) + shutil.copymode(input_path, stripped_out) + + +def main(argv): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--input", required=True) + parser.add_argument("--stripped-out", required=True) + parser.add_argument("--debug-out", required=True) + parser.add_argument("--debug-out-is-dir", action="store_true") + parser.add_argument("--strip", default="") + parser.add_argument("--objcopy", default="") + parser.add_argument("--dsymutil", default="") + args = parser.parse_args(argv) + + fmt = _detect_format(args.input) + + if fmt == "macho" and args.dsymutil: + if not args.debug_out_is_dir: + sys.exit("dd_strip_driver: Mach-O debug output must be a directory (.dSYM)") + # Bazel pre-creates declared directories empty; dsymutil wants to + # create the bundle itself. + if os.path.isdir(args.debug_out): + os.rmdir(args.debug_out) + _run([args.dsymutil, args.input, "-o", args.debug_out]) + _strip_macho(args.input, args.stripped_out, args.debug_out, args.strip) + elif fmt == "elf" and args.objcopy: + _strip_elf(args.input, args.stripped_out, args.debug_out, args.strip, args.objcopy) + elif fmt == "pe": + _strip_pe(args.input, args.stripped_out, args.debug_out, args.strip) + else: + _passthrough(args.input, args.stripped_out, args.debug_out, args.debug_out_is_dir) + + +if __name__ == "__main__": + main(sys.argv[1:]) diff --git a/bazel/toolchains/dd_strip/BUILD.bazel b/bazel/toolchains/dd_strip/BUILD.bazel new file mode 100644 index 000000000000..62824d420bc9 --- /dev/null +++ b/bazel/toolchains/dd_strip/BUILD.bazel @@ -0,0 +1,43 @@ +"""Toolchains providing strip/objcopy/dsymutil for debug-symbol splitting.""" + +load(":configure.bzl", "dd_strip_macos_toolchain") +load(":toolchain.bzl", "dd_strip_cc_toolchain") + +# Provides paths to strip/objcopy/dsymutil, sourced either from the resolved +# cc_toolchain (Linux, Windows) or hardcoded to macOS system paths. +toolchain_type( + name = "toolchain_type", + visibility = ["//visibility:public"], +) + +dd_strip_cc_toolchain( + name = "dd_strip_cc_impl", +) + +dd_strip_macos_toolchain( + name = "dd_strip_macos_impl", +) + +toolchain( + name = "dd_strip_linux_toolchain", + exec_compatible_with = ["@platforms//os:linux"], + target_compatible_with = ["@platforms//os:linux"], + toolchain = ":dd_strip_cc_impl", + toolchain_type = ":toolchain_type", +) + +toolchain( + name = "dd_strip_windows_toolchain", + exec_compatible_with = ["@platforms//os:windows"], + target_compatible_with = ["@platforms//os:windows"], + toolchain = ":dd_strip_cc_impl", + toolchain_type = ":toolchain_type", +) + +toolchain( + name = "dd_strip_macos_toolchain", + exec_compatible_with = ["@platforms//os:macos"], + target_compatible_with = ["@platforms//os:macos"], + toolchain = ":dd_strip_macos_impl", + toolchain_type = ":toolchain_type", +) diff --git a/bazel/toolchains/dd_strip/configure.bzl b/bazel/toolchains/dd_strip/configure.bzl new file mode 100644 index 000000000000..826374e20967 --- /dev/null +++ b/bazel/toolchains/dd_strip/configure.bzl @@ -0,0 +1,32 @@ +"""macOS instantiation of the dd_strip toolchain. + +macOS ships `strip` and `dsymutil` as part of Xcode's command-line tools, not +as part of any Bazel cc_toolchain, so there is nothing to source these paths +from — they are hardcoded absolute paths, the same way +//bazel/rules/rewrite_rpath hardcodes /usr/bin/otool for macOS rpath +rewriting. This intentionally avoids the auto-detecting +`make_toolchain_repository_rule` pattern used by +//bazel/toolchains/codesign: that generator also emits a `config_setting` +per tool (`have_`), and new config_settings require a separate design +review per .claude/rules/bazel.md — this toolchain doesn't need one. +""" + +load(":toolchain.bzl", "DdStripToolchainInfo") + +_STRIP_PATH = "/usr/bin/strip" +_DSYMUTIL_PATH = "/usr/bin/dsymutil" + +def _dd_strip_macos_toolchain_impl(ctx): + return [platform_common.ToolchainInfo( + dd_strip_info = DdStripToolchainInfo( + strip_path = _STRIP_PATH, + objcopy_path = None, + dsymutil_path = _DSYMUTIL_PATH, + tool_files = depset(), + ), + )] + +dd_strip_macos_toolchain = rule( + implementation = _dd_strip_macos_toolchain_impl, + doc = "Hardcodes the macOS strip/dsymutil paths for the dd_strip toolchain type.", +) diff --git a/bazel/toolchains/dd_strip/toolchain.bzl b/bazel/toolchains/dd_strip/toolchain.bzl new file mode 100644 index 000000000000..9de5284bdcf5 --- /dev/null +++ b/bazel/toolchains/dd_strip/toolchain.bzl @@ -0,0 +1,71 @@ +"""dd_strip toolchain: strip/objcopy/dsymutil tools for debug-symbol splitting. + +Type: //bazel/toolchains/dd_strip:toolchain_type + +Toolchains: +- dd_strip_cc_toolchain — sources `strip`/`objcopy` from the resolved + `cc_toolchain` (Linux and Windows/mingw both already declare these tools; + see bazel/toolchains/gcc/toolchain.bzl and bazel/toolchains/mingw/toolchain.bzl). +- dd_strip_macos_toolchain (configure.bzl) — hardcodes /usr/bin/strip and + /usr/bin/dsymutil, the same way //bazel/rules/rewrite_rpath hardcodes + /usr/bin/otool for macOS. macOS ships neither tool as part of a Bazel + cc_toolchain, so there is nothing to source them from. + +All fields are plain path strings (not File objects) because that's what +`cc_common.get_tool_for_action`-style tool sourcing and hardcoded system +paths both produce. `tool_files` carries whatever File objects must be added +to action `inputs` for the path to resolve (empty for hardcoded system +paths, since those live outside the exec root entirely). +""" + +load("@rules_cc//cc:defs.bzl", "cc_common") +load("@rules_cc//cc:find_cc_toolchain.bzl", "CC_TOOLCHAIN_ATTRS", "find_cc_toolchain", "use_cc_toolchain") + +DdStripToolchainInfo = provider( + doc = "Paths to the strip/objcopy/dsymutil tools used to split debug symbols from a binary.", + fields = { + "strip_path": "string path to the `strip` executable, or None if unavailable.", + "objcopy_path": "string path to the `objcopy` executable (Linux only), or None.", + "dsymutil_path": "string path to `dsymutil` (macOS only), or None.", + "tool_files": "depset of File that must be included as action inputs for the paths above to resolve.", + }, +) + +def _dd_strip_cc_toolchain_impl(ctx): + cc_toolchain = find_cc_toolchain(ctx) + feature_configuration = cc_common.configure_features( + ctx = ctx, + cc_toolchain = cc_toolchain, + requested_features = ctx.features, + unsupported_features = ctx.disabled_features, + ) + + # Silence the unused-variable warning; kept for parity with other + # cc_toolchain-sourced rules and in case a future tool needs + # get_tool_for_action-style resolution instead of the plain executable + # path fields below. + _ = feature_configuration + + strip_path = cc_toolchain.strip_executable + objcopy_path = cc_toolchain.objcopy_executable + return [platform_common.ToolchainInfo( + dd_strip_info = DdStripToolchainInfo( + strip_path = strip_path if strip_path else None, + objcopy_path = objcopy_path if objcopy_path else None, + dsymutil_path = None, + tool_files = cc_toolchain.all_files, + ), + )] + +dd_strip_cc_toolchain = rule( + implementation = _dd_strip_cc_toolchain_impl, + doc = """Sources strip/objcopy paths from the resolved cc_toolchain. + + Used to register the dd_strip toolchain on Linux and Windows, where the + cc_toolchain already wires up both tools (gcc-toolchain ships objcopy and + strip; the mingw toolchain declares strip via tool_paths, and Windows has + no split-DWARF story so objcopy is simply left unset there).""", + attrs = CC_TOOLCHAIN_ATTRS, + toolchains = use_cc_toolchain(), + fragments = ["cpp"], +) diff --git a/dbg_symbol.md b/dbg_symbol.md new file mode 100644 index 000000000000..a2f5d683cdf0 --- /dev/null +++ b/dbg_symbol.md @@ -0,0 +1,115 @@ +# ABLD-464: Debug-symbol ("dbg") packages for Bazel builds + +Context and design record for [ABLD-464](https://datadoghq.atlassian.net/browse/ABLD-464) +("Process for creating 'dbg' packages"). Kept alongside the code so the rationale +survives past the PR description. + +## Problem + +Omnibus produces stripped release binaries plus a separate "-dbg" package/archive +containing the removed debug symbols, for post-mortem debugging of crash dumps from +stripped production binaries. Bazel packaging (`packages/`, built on `rules_pkg`) has +no equivalent yet — `packages/AGENTS.md` and `packages/installer/MIGRATION_PLAN.md` +both flag this as an open gap. + +Requirements from the ticket: +- A normal `bazel build` of a target keeps debug symbols (dev/test use). +- A packaging build produces stripped binaries. +- A sibling packaging target produces *only* the debug symbols, no other files. +- No shadow dependency tree duplicated top-down to every binary/library target. +- Works for Go, C, and Rust objects; works on Linux, macOS, Windows. +- Callable from `rules_pkg` `pkg_files` construction. + +## Two strategies, two PRs + +The ticket names two candidate strategies. Rather than pick one up front, we +implemented both as independent PRs in separate worktrees of this repo, so they can be +compared in review and in CI before committing to one for the real migration: + +- **Plan A — provider/rule-based** (`/Users/tony.aiuto/datadog-agent`, branch + `aiuto/454-a`): a new `dd_strip_debug` rule wraps each binary/library's own build + rule directly, adding a `DdStripInfo` provider and + `OutputGroupInfo(stripped=..., debug=...)`. No aspect is needed for the core case — + the default output stays unstripped, and a `pkg_files` transform just checks whether + its `srcs[i]` carries `DdStripInfo`. + See `bazel/rules/dd_strip/`, `bazel/toolchains/dd_strip/`, + `bazel/rules/dd_packaging/dd_pkg_files_stripped.bzl`. +- **Plan B — packaging-time filter** (this repo, branch `aiuto/454-b`): a new + `dd_pkg_strip_transform` rule intercepts files as they're + assembled into a `pkg_files`/`PackageFilesInfo` tree and strips/splits them there, + with no cooperation needed from the binary's own rule. This is what lets it work + directly against today's `prebuilt_file.bzl`-backed product binaries + (`@agent_binary//:agent`, etc.), which aren't real Bazel `go_binary`/`cc_binary` + targets yet — Plan A can't wrap a provider around those until that migration lands. + +Both use the same platform semantics, matching omnibus's `Stripper` +(`omnibus-ruby/lib/omnibus/stripper.rb`): +- **Linux**: `objcopy --only-keep-debug` → `.dbg`, `strip --strip-debug + --strip-unneeded` in place, `objcopy --add-gnu-debuglink`. +- **macOS**: `strip` the shipped copy, `dsymutil` produces a `.dSYM` bundle. No prior + art for this in omnibus or this repo (omnibus skips stripping on macOS entirely) — + flagged as needing build-team confirmation before either PR merges. +- **Windows**: debug artifact = the *unstripped original* binary (not split DWARF), + matching omnibus's `windows_symbol_stripping_file` semantics, since this repo's + Go/Rust/mingw toolchains don't reliably produce standalone PDBs. + +Both PRs independently build their own copy of the `bazel/toolchains/dd_strip` +toolchain rather than one depending on the other (explicit choice, made so both could +start immediately) — deduping them is a deliberate follow-up once one strategy is +chosen. + +Full original design doc (file lists, verification steps, trade-off analysis): +`/Users/tony.aiuto/.claude/plans/we-are-going-to-flickering-pizza.md` (local to the +machine this was planned on, not checked in — this file is the durable summary). + +## Known issue: macOS codesign ordering + +`dd_cc_packaged` (`bazel/rules/dd_packaging/dd_cc_packaged.bzl`) runs `dd_strip_debug` +**after** `rewrite_rpath`. `rewrite_rpath`'s macOS implementation +(`bazel/toolchains/rpath_rewriter/rewrite_with_install_name_tool.sh`) ends with: + +```sh +# Re-sign with an ad-hoc signature after modification as install_name_tool invalidates +# any existing code signature. +/usr/bin/codesign --sign - --force "$OUTPUT" +``` + +`install_name_tool` invalidates any existing signature, so `rewrite_rpath` re-signs +ad-hoc as its last step. `dd_strip_debug` then runs `strip`/`objcopy`/`dsymutil` on +that *already re-signed* output — and stripping/objcopy also invalidates a Mach-O code +signature, but **`dd_strip_debug` does not re-sign afterward**. The result: on macOS, +the final `.stripped` binary that actually ships out of `dd_cc_packaged` carries a +stale/invalid ad-hoc signature. + +This was chosen deliberately to match omnibus's ordering ("strip is the last finalize +step"), but omnibus's ordering rationale is Linux-centric (objcopy debuglink chains) +and doesn't account for macOS's codesign-after-every-mutation requirement. + +Two ways to fix, not yet decided: +1. Add a `codesign --sign - --force` re-sign step to `dd_strip_debug`'s macOS driver, + after the strip step, so it always leaves a valid ad-hoc signature — keeps + omnibus's "strip last" ordering. +2. Reorder so `dd_strip_debug` runs *before* `rewrite_rpath` on macOS specifically, + since `rewrite_rpath` already re-signs as its last step — avoids adding a second + codesign invocation, but only applies to the `dd_cc_packaged` call chain (Plan A); + for Plan B's flattened packaging-time model there's no equivalent single choke + point to reorder around, so option 1 (re-sign after strip) is likely the more + portable fix across both PRs. + +This affects Plan A directly (found during its implementation/testing). Plan B should +be checked for the same issue if/when it starts producing real signed macOS binaries +through `dd_cc_packaged`-style consumers. + +## Other open items (both PRs) + +- Linux `objcopy` path is unverified locally — both PRs were implemented and tested on + macOS arm64 with no Linux exec platform available; needs CI or a Linux sandbox. +- `packages/agent/product/BUILD.bazel`'s real binaries come from `prebuilt_file.bzl` + (built by `dda`, not a real Bazel target) — Plan A can only demonstrate against + `//cmd/agent:agent` and `rtloader` directly, not the full product bundle, until that + migration lands. Plan B has no such blocker. +- Plan A: `dsymutil` produced an empty `.dSYM` for at least one pure-Go binary during + testing — needs investigation (Go's DWARF layout vs. dsymutil's expectations). +- Whatever builds release binaries outside Bazel (`dda inv agent.build` / omnibus) + must not pre-strip them, or there's nothing left for either strategy to split. + Out of scope for both PRs; flagged as a cross-cutting dependency. diff --git a/packages/AGENTS.md b/packages/AGENTS.md index 28f3062d6b6b..b5dc522e3922 100644 --- a/packages/AGENTS.md +++ b/packages/AGENTS.md @@ -214,7 +214,21 @@ This target is removed once the omnibus recipe is deleted. - **Version stamping**: all products currently hardcode `version = "7"`. In the future we will get that from the pipeine (equivalent to `build_version ENV['PACKAGE_VERSION']`). - **RHEL vs SUSE constraint**: omnibus distinguishes RHEL and SUSE builds in different pipelines. Going forward we'll just have different targets for them, and always build both in the same pipeline. -- **Symbol stripping / signing**: `strip_build`, `windows_symbol_stripping_file`, - `inspect_binary`, and `sign_file` are not yet migrated. +- **Symbol stripping**: migrated. `pkg_files` for shipped binaries has been + replaced with `dd_pkg_files_stripped` (see + `//bazel/rules/dd_packaging:dd_pkg_strip_transform.bzl`), which strips + object files at packaging time and splits debug info into a parallel + `name + "_debug"` target (Linux: `objcopy`-split `.debug` + debuglink; + macOS: `strip -x` + a `dsymutil` `.dSYM`; Windows: `strip`'d binary plus the + unstripped original, matching `windows_symbol_stripping_file` -- mingw has + no split-DWARF story). `packages/agent/product:all_files_debug` and + `packages/installer/windows:installer_components_debug` collect the debug + siblings. `inspect_binary` (verifying a shipped binary is actually + stripped) and `sign_file` are still not migrated. + Depends on: whatever builds the underlying binary must not pre-strip it + (there is nothing left to split debug info from otherwise) -- see + `bazel/configs/system_probe_lite.bazelrc`'s note on this for the Rust + build; the `dda`/omnibus-built Go binaries have the same requirement but it + is out of scope for this change. - **Transitive deps**: `//packages/agent/linux:transitive_deps` is a temporary catch-all. It shrinks as deps grow proper Bazel targets. See ABLD-363. diff --git a/packages/agent/product/BUILD.bazel b/packages/agent/product/BUILD.bazel index 2ebd26b145e1..c596a96d8f28 100644 --- a/packages/agent/product/BUILD.bazel +++ b/packages/agent/product/BUILD.bazel @@ -12,6 +12,7 @@ load( "pkg_files", "pkg_mkdirs", ) +load("//bazel/rules/dd_packaging:dd_pkg_strip_transform.bzl", "dd_pkg_files_stripped") load("//packages/agent:defs.bzl", "ETC_DIR_SELECTOR") package(default_visibility = ["//packages:__subpackages__"]) @@ -79,6 +80,23 @@ pkg_filegroup( }), ) +# The debug-symbol counterpart to :all_files: for every binary that +# dd_pkg_files_stripped split debug info off of, this carries the debug +# artifact (a .debug file, a .dSYM bundle, or -- on Windows, which has no +# split-DWARF story -- the unstripped original) instead of the stripped +# binary itself. Non-binary files (configs, dirs, python sources, ...) have +# no debug counterpart and are intentionally not included here; see +# dd_pkg_strip_transform.bzl's "debug_only" mode. +pkg_filegroup( + name = "all_files_debug", + srcs = [ + ":dda_built_agent_binary_debug", + ":dda_built_privateactionrunner_binary_debug", + ":dda_built_process_agent_binary_debug", + ":dda_built_trace_agent_binary_debug", + ], +) + # All the components to be installed in {install_dir} during the build during the migration. # This is the subset of :all_files that we need to place in install_dir so the omnibus # packaging can pick them up. When we can build the packages with bazel rules, this goes @@ -131,7 +149,7 @@ pkg_files( }, ) -pkg_files( +dd_pkg_files_stripped( name = "dda_built_agent_binary", srcs = [ "@agent_binary//:agent", @@ -140,7 +158,7 @@ pkg_files( prefix = "bin/agent", ) -pkg_files( +dd_pkg_files_stripped( name = "dda_built_process_agent_binary", srcs = [ "@process_agent_binary//:process_agent", @@ -152,7 +170,7 @@ pkg_files( }), ) -pkg_files( +dd_pkg_files_stripped( name = "dda_built_privateactionrunner_binary", srcs = [ "@privateactionrunner_binary//:privateactionrunner", @@ -161,7 +179,7 @@ pkg_files( prefix = "embedded/bin", ) -pkg_files( +dd_pkg_files_stripped( name = "dda_built_trace_agent_binary", srcs = [ "@trace_agent_binary//:trace_agent", diff --git a/packages/installer/MIGRATION_PLAN.md b/packages/installer/MIGRATION_PLAN.md index 9d23b6a6b27e..cf0d98b2557d 100644 --- a/packages/installer/MIGRATION_PLAN.md +++ b/packages/installer/MIGRATION_PLAN.md @@ -373,6 +373,5 @@ Once the diff output is clean (or remaining diffs are explicitly accepted): — the `prebuilt_file` approach is sufficient for the packaging migration - Windows MSI (no Bazel rule exists yet) - macOS PKG (no Bazel rule exists yet) -- Symbol stripping / debug package split - Code signing (Windows `sign_file`, macOS `code_signing_identity`) - Removing omnibus installer code (happens after Phase 7 completes) diff --git a/packages/installer/windows/BUILD.bazel b/packages/installer/windows/BUILD.bazel index 543ead24db9f..152cf0414b1f 100644 --- a/packages/installer/windows/BUILD.bazel +++ b/packages/installer/windows/BUILD.bazel @@ -3,17 +3,24 @@ load( "@rules_pkg//pkg:mappings.bzl", "pkg_filegroup", - "pkg_files", ) load("@rules_pkg//pkg:tar.bzl", "pkg_tar") load("//bazel/rules:compression.bzl", "get_compression_level") +load("//bazel/rules/dd_packaging:dd_pkg_strip_transform.bzl", "dd_pkg_files_stripped") load("//compliance:package_licenses.bzl", "package_licenses") package(default_visibility = ["//packages:__subpackages__"]) # On Windows the binary lives at the install dir root as datadog-installer.exe, # not under bin/installer/ as on Linux/macOS. -pkg_files( +# +# dd_pkg_files_stripped also creates ":installer_binary_debug" (mode = +# "debug_only"), the unstripped-original sibling referenced by +# :installer_components_debug below -- see dd_pkg_strip_transform.bzl for why +# Windows' "debug" artifact is the unstripped original rather than a +# split-DWARF file (mingw's toolchain has no objcopy, matching omnibus's +# windows_symbol_stripping_file semantics). +dd_pkg_files_stripped( name = "installer_binary", srcs = ["@installer_binary//:installer"], ) @@ -25,6 +32,14 @@ pkg_filegroup( prefix = "C:/opt/datadog-installer", ) +# Debug-symbol counterpart to :installer_components -- the unstripped +# installer_binary, at the same layout, for the debug package. +pkg_filegroup( + name = "installer_components_debug", + srcs = [":installer_binary_debug"], + prefix = "C:/opt/datadog-installer", +) + # Intermediate node for the license-gathering aspect. pkg_filegroup( name = "everything", @@ -48,4 +63,3 @@ pkg_tar( ) # TODO: MSI target when a Bazel MSI rule is available. -# TODO: Symbol stripping equivalent of windows_symbol_stripping_file.