From ac6b0d3196b10dbfba757968d955cf5a04dfa6a2 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Fri, 4 Sep 2026 17:48:02 +0200 Subject: [PATCH 01/10] chore(memtrack): add argp.h stub for musl builds `libbpf-sys` vendors elfutils, whose `configure` aborts on a musl target because `argp_parse` is a glibc extension that musl does not implement: checking for library containing argp_parse... no configure: error: failed to find argp_parse libelf does not actually need those symbols -- they are used by the elfutils CLI tools for argument parsing, but `configure.ac` checks for them unconditionally, even when only the library is being built. Seeding autoconf's cache (`ac_cv_search_argp_parse="none required"`) skips the check, but the elfutils sources still `#include `, so the header has to exist. Nothing that gets compiled calls into it, hence declarations only. Lives inside the crate, next to `wrapper.h` and `src/ebpf/c`, rather than at the repo root: it is a memtrack build input, and it needs its own directory because the path goes on the include path via `-I`. Refs: https://github.com/libbpf/libbpf-sys/issues/137 Co-Authored-By: Claude Opus 5 (1M context) --- crates/memtrack/musl/argp.h | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 crates/memtrack/musl/argp.h diff --git a/crates/memtrack/musl/argp.h b/crates/memtrack/musl/argp.h new file mode 100644 index 00000000..26a03a69 --- /dev/null +++ b/crates/memtrack/musl/argp.h @@ -0,0 +1,43 @@ +/* crates/memtrack/musl/argp.h — stub for musl builds of libbpf-sys' vendored elfutils. + Declarations only: a libelf-only build never calls into argp, but the + elfutils sources still `#include `, which musl does not ship. + If compilation complains about a missing type or macro, add it here. */ +#ifndef CODSPEED_STUB_ARGP_H +#define CODSPEED_STUB_ARGP_H + +#include + +typedef int error_t; + +struct argp_option { + const char *name; + int key; + const char *arg; + int flags; + const char *doc; + int group; +}; + +struct argp_state { + const char *name; +}; + +typedef error_t (*argp_parser_t)(int key, char *arg, struct argp_state *state); + +struct argp { + const struct argp_option *options; + argp_parser_t parser; + const char *args_doc; + const char *doc; + const void *children; + void *help_filter; + const char *argp_domain; +}; + +#define OPTION_ARG_OPTIONAL 0x1 +#define ARGP_HELP_SEE 0x40 +#define ARGP_ERR_UNKNOWN 1 + +int argp_help(const struct argp *argp, FILE *stream, unsigned int flags, char *name); + +#endif /* CODSPEED_STUB_ARGP_H */ From 099bab45f1aa840379b3237de970799ba2b7ed79 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Fri, 4 Sep 2026 18:00:51 +0200 Subject: [PATCH 02/10] ci: add throwaway COD-3440 musl check workflow Closes the one gap the local COD-3440 spike could not: whether the musl build of memtrack actually loads its BPF programs on a real x86_64 kernel. The spike was done on an aarch64 host, where an x86_64 build can only be cross-compiled -- its skeleton targets the x86_64 ABI and cannot load against an aarch64 kernel. `workflow_dispatch` only, so it never runs on its own, and it touches nothing in `release.yml`, `dist-workspace.toml` or any `Cargo.toml`. Beyond the autoconf cache seeds and the argp.h stub, Debian needs one thing the spike host did not: `LIBBPF_SYS_EXTRA_CFLAGS` with `-idirafter /usr/include/x86_64-linux-gnu -idirafter /usr/include`. Its musl-gcc runs with `-nostdinc` and only the musl include directory, so libbpf cannot find the kernel UAPI headers it includes directly: bpf.c:28:10: fatal error: asm/unistd.h: No such file or directory ../include/linux/types.h:12:10: fatal error: asm/types.h: No such file **This must not reach `main`.** Delete it once the question is answered. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/cod-3440-musl-check.yml | 206 ++++++++++++++++++++++ 1 file changed, 206 insertions(+) create mode 100644 .github/workflows/cod-3440-musl-check.yml diff --git a/.github/workflows/cod-3440-musl-check.yml b/.github/workflows/cod-3440-musl-check.yml new file mode 100644 index 00000000..a0d6ec28 --- /dev/null +++ b/.github/workflows/cod-3440-musl-check.yml @@ -0,0 +1,206 @@ +# COD-3440 spike — throwaway workflow, NOT for merging. +# +# Purpose: prove the musl recipe on a real x86_64 kernel, which is the one gap +# the local spike could not close (the local host is aarch64; an x86_64 build +# cross-compiled there cannot load its BPF skeleton against an aarch64 kernel). +# +# To use it: push the spike branch, then +# +# gh workflow run cod-3440-musl-check.yml --ref spike/cod-3440-memtrack-musl +# +# Manual trigger only, so it never fires on its own. Note that the very first +# dispatch of a workflow that lives only on a non-default branch 404s until +# GitHub has registered it; once it has appeared in the Actions list, --ref +# dispatch works. Delete the file once the question is answered -- it must not +# reach main. +# +# Deliberately does NOT touch release.yml, dist-workspace.toml or any +# Cargo.toml — it only adds one manually-triggered job. +# +# Two differences from the local spike recipe, both simplifications: +# - musl-tools provides x86_64-linux-musl-gcc, which cc-rs finds on its own, +# so no compiler wrapper and no -lgcc / -fno-link-libatomic. +# - no zig, so none of the zig artifacts (crt duplication, UBSan trap, +# unknown warning options) apply. +# The only thing carried over is what the spike is actually about: the three +# autoconf cache seeds and the crates/memtrack/musl/argp.h stub. + +name: COD-3440 musl check + +on: + workflow_dispatch: + +env: + TARGET: x86_64-unknown-linux-musl + # Absolute, because the test step runs with working-directory: crates/memtrack, + # where a $PWD-relative path would resolve to the wrong place. + STUB_INCLUDE: ${{ github.workspace }}/crates/memtrack/musl + # libbpf includes and ; Debian's musl-gcc runs with + # -nostdinc and only /usr/include/x86_64-linux-musl on the include path, so the + # kernel UAPI headers from linux-libc-dev have to be added back. -idirafter puts + # them last, behind musl's own headers. build.rs forwards this to libbpf's make; + # CFLAGS alone would not reach it the same way. + LIBBPF_SYS_EXTRA_CFLAGS: -idirafter /usr/include/x86_64-linux-gnu -idirafter /usr/include + # Approach A: pre-seed autoconf's cache so the vendored elfutils never runs + # the checks musl cannot satisfy. "none required" = available with no -l flag. + ac_cv_search_argp_parse: none required + ac_cv_search__obstack_free: none required + ac_cv_search_fts_close: none required + +jobs: + build: + name: build ${{ matrix.profile }} + runs-on: ubuntu-latest # x86_64 + strategy: + fail-fast: false + matrix: + profile: [dev, dist] + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: musl-${{ matrix.profile }} + - uses: ./.github/actions/install-bpf-deps + + - name: Install musl toolchain + run: | + sudo apt-get install -y musl-tools pkg-config linux-libc-dev + rustup target add "$TARGET" + + - name: Build + # elfutils only honours CFLAGS/CPPFLAGS, not LIBBPF_SYS_EXTRA_CFLAGS. + # The stub header exists because elfutils #include even though + # a libelf-only build never calls into it. + run: | + export CFLAGS="-I$STUB_INCLUDE" + cargo build -p memtrack --profile ${{ matrix.profile }} --target "$TARGET" + + - name: Verify the artifact is genuinely static + run: | + BIN=target/$TARGET/${{ matrix.profile == 'dev' && 'debug' || matrix.profile }}/codspeed-memtrack + file "$BIN" + ldd "$BIN" || true # expected: "not a dynamic executable" + readelf -d "$BIN" || true # expected: no dynamic section at all + echo "size: $(stat -c %s "$BIN") bytes" + # Fail loudly if anything reintroduced a dynamic dependency or an rpath. + # `if` rather than `grep ... && exit 1`, because a grep that matches + # nothing exits 1 and would fail the step under bash -e. + if readelf -d "$BIN" 2>/dev/null | grep -qE 'NEEDED|RPATH|RUNPATH'; then + echo "unexpected dynamic dependency or rpath" + exit 1 + fi + echo "OK: static, no NEEDED, no RPATH/RUNPATH" + + - name: Compare against the gnu build + if: matrix.profile == 'dist' + run: | + unset CFLAGS + cargo build -p memtrack --profile dist + echo "musl: $(stat -c %s target/$TARGET/dist/codspeed-memtrack) bytes" + echo "gnu: $(stat -c %s target/dist/codspeed-memtrack) bytes" + + - name: Smoke test the BPF path + run: | + BIN=$PWD/target/$TARGET/${{ matrix.profile == 'dev' && 'debug' || matrix.profile }}/codspeed-memtrack + mkdir -p /tmp/memtrack-out + sudo env "RUST_LOG=info" "$BIN" track -o /tmp/memtrack-out "/bin/ls /tmp" + ls -la /tmp/memtrack-out + # A run that loads no probes still exits 0 but writes nothing. + test -n "$(ls -A /tmp/memtrack-out)" || { echo "no artifact written"; exit 1; } + + tests: + name: ${{ matrix.test }} (musl) + runs-on: ubuntu-latest # x86_64 + strategy: + fail-fast: false + # Each memtrack integration test binary runs its cases serially (the eBPF + # tracker can't overlap with itself in one process), so shard by binary. + matrix: + test: [c_tests, cpp_tests, rust_tests, spawn_tests, dlopen_tests, rss_tests] + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + lfs: true + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: musl-${{ matrix.test }} + - uses: ./.github/actions/install-bpf-deps + + - name: Install musl toolchain + run: | + sudo apt-get install -y musl-tools pkg-config linux-libc-dev + rustup target add "$TARGET" + + - name: Install additional allocators + run: sudo apt-get install -y libmimalloc-dev libjemalloc-dev + + # Built separately from the run because test-with's env(GITHUB_ACTIONS) + # gate is evaluated at COMPILE time. GITHUB_ACTIONS is set by the runner, + # so this is automatic here -- but if the tests are ever built outside + # Actions, the sudo-gated cases silently become #[ignore]d and the run + # reports a green "0 passed; N ignored". + - name: Build tests + run: | + export CFLAGS="-I$STUB_INCLUDE" + cargo test -p memtrack --target "$TARGET" --no-run + + - name: Run tests + env: + RUST_LOG: debug + # Ubuntu 26.04 ships sudo-rs, which ignores `-E`; pass the env the + # rustup shims and the test gate need through `env` instead. + run: | + sudo env \ + "HOME=$HOME" \ + "PATH=$PATH" \ + "CARGO_HOME=${CARGO_HOME:-$HOME/.cargo}" \ + "RUSTUP_HOME=${RUSTUP_HOME:-$HOME/.rustup}" \ + "CARGO_INCREMENTAL=$CARGO_INCREMENTAL" \ + "RUST_LOG=$RUST_LOG" \ + "GITHUB_ACTIONS=$GITHUB_ACTIONS" \ + "CFLAGS=-I$STUB_INCLUDE" \ + "LIBBPF_SYS_EXTRA_CFLAGS=$LIBBPF_SYS_EXTRA_CFLAGS" \ + "TARGET=$TARGET" \ + "ac_cv_search_argp_parse=$ac_cv_search_argp_parse" \ + "ac_cv_search__obstack_free=$ac_cv_search__obstack_free" \ + "ac_cv_search_fts_close=$ac_cv_search_fts_close" \ + $(which cargo) test --target "$TARGET" --test ${{ matrix.test }} \ + -- --test-threads 1 --nocapture + working-directory: crates/memtrack + + # Since we ran the tests with sudo, the build artifacts will have root ownership + - name: Clean up + run: sudo chown -R $USER:$USER . ~/.cargo + + unit: + name: unit tests (musl) + runs-on: ubuntu-latest # x86_64 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: musl-unit + - uses: ./.github/actions/install-bpf-deps + + - name: Install musl toolchain + run: | + sudo apt-get install -y musl-tools pkg-config linux-libc-dev + rustup target add "$TARGET" + + # Split out from the sharded job because of one known failure: + # ebpf::memtrack::tests::libc_allocator_symbols_resolve_to_offsets reads + # /proc/self/maps of the TEST BINARY and requires a mapped libc.so.6, + # which a statically linked musl binary does not have by construction. + # The production path resolves symbols in the *traced* process, so this is + # a test assumption, not a defect (report §4). Drop the --skip to check + # whether it has since been fixed; everything else must stay green. + - name: Run unit tests + run: | + export CFLAGS="-I$STUB_INCLUDE" + cargo test -p memtrack --target "$TARGET" --lib \ + -- --skip libc_allocator_symbols_resolve_to_offsets From c1639395010a422d84e6950b0b9cb5ab4c699cca Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 7 Sep 2026 12:29:50 +0200 Subject: [PATCH 03/10] feat(exec-harness)!: remove the LD_PRELOAD hack The harness used to inject a `libcodspeed_preload.so` into the benchmark process so the callgrind client requests were issued from inside it. That was only necessary because instrumentation state did not propagate across `fork`, which COD-2349 has since fixed. Instrumentation is now toggled in exec-harness itself, around the spawn of each benchmark command. The benchmarked child inherits the live state across `fork`/`exec`, callgrind records the spawn edge on the dump part live at fork time, and `set_executed_benchmark` names that same part with the benchmark URI, so the backend can attribute the child's whole trace to the benchmark. Dropping the preload removes the "CPU Simulation mode does not support statically linked binaries" limitation, since nothing has to be injected into the benchmarked executable any more. It also unblocks building exec-harness for musl (COD-3440), which a preloaded `.so` made impossible. `--instr-atstart=inherit` becomes unconditional: it is what makes the benchmark measurable at all now, so it can no longer hang off the opt-in `--simulation-track-subprocess`, which keeps its name but from now on only selects `--separate-threads`. Since every way this can go wrong is silent -- the harness runs, the benchmark completes, and the measurement is empty -- the harness now fails loudly when it finds itself uninstrumented. BREAKING CHANGE: the measured region of an exec-harness benchmark now begins in exec-harness before the fork rather than in the child's ELF constructor, so it also covers the fork/exec/wait path and the child's pre-main startup. Absolute numbers shift in a step and history is not comparable across this change. A post-preload exec-harness also requires a runner that passes `--instr-atstart=inherit`, valgrind-codspeed >= iteration 6, and a backend with spawn-chain attribution. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 2 - crates/exec-harness/Cargo.toml | 4 - crates/exec-harness/build.rs | 168 +----------------- .../exec-harness/preload/codspeed_preload.c | 87 --------- .../src/analysis/ld_preload_check.rs | 120 ------------- crates/exec-harness/src/analysis/mod.rs | 84 ++++----- .../src/analysis/preload_lib_file.rs | 46 ----- crates/exec-harness/src/constants.rs | 7 +- crates/exec-harness/src/lib.rs | 7 +- src/cli/shared.rs | 6 +- src/executor/valgrind/measure.rs | 9 +- 11 files changed, 66 insertions(+), 474 deletions(-) delete mode 100644 crates/exec-harness/preload/codspeed_preload.c delete mode 100644 crates/exec-harness/src/analysis/ld_preload_check.rs delete mode 100644 crates/exec-harness/src/analysis/preload_lib_file.rs diff --git a/Cargo.lock b/Cargo.lock index 1acec08f..33488aca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1179,13 +1179,11 @@ name = "exec-harness" version = "1.3.0" dependencies = [ "anyhow", - "cc", "clap", "env_logger", "humantime", "instrument-hooks-bindings", "log", - "object", "runner-shared", "serde", "serde_json", diff --git a/crates/exec-harness/Cargo.toml b/crates/exec-harness/Cargo.toml index f73c631c..277f0155 100644 --- a/crates/exec-harness/Cargo.toml +++ b/crates/exec-harness/Cargo.toml @@ -20,10 +20,6 @@ serde = { workspace = true } humantime = "2.3" runner-shared = { path = "../runner-shared" } tempfile = { workspace = true } -object = { workspace = true } - -[build-dependencies] -cc = "1" [package.metadata.dist] targets = ["aarch64-unknown-linux-gnu", "x86_64-unknown-linux-gnu"] diff --git a/crates/exec-harness/build.rs b/crates/exec-harness/build.rs index bf65ef7e..8988c463 100644 --- a/crates/exec-harness/build.rs +++ b/crates/exec-harness/build.rs @@ -1,170 +1,18 @@ //! Build script for exec-harness //! -//! This script compiles the `libcodspeed_preload.so` shared library that is used -//! to inject instrumentation into child processes via LD_PRELOAD. -//! -//! The library is built using the `core.c` and headers from the `instrument-hooks-bindings` -//! crate's `instrument-hooks` directory. - -use std::env; -use std::path::PathBuf; +//! Exports the constants shared between the crate's modules as environment +//! variables, so `src/constants.rs` can read them through `env!()` and there is +//! a single source of truth for the integration identity reported to CodSpeed. -/// Shared constants for the preload library. -/// These are passed as C defines during compilation and exported as environment -/// variables for the Rust code to use via `env!()`. -struct PreloadConstants { - /// Environment variable name for the benchmark URI. - uri_env: &'static str, - /// Integration name reported to CodSpeed. - integration_name: &'static str, - /// Integration version reported to CodSpeed. - integration_version: &'static str, - /// Filename for the preload shared library. - preload_lib_filename: &'static str, -} +/// Integration name reported to CodSpeed. +const INTEGRATION_NAME: &str = "exec-harness"; fn main() { - println!("cargo:rerun-if-changed=preload/codspeed_preload.c"); - println!("cargo:rerun-if-env-changed=CODSPEED_INSTRUMENT_HOOKS_DIR"); - - let preload_constants: PreloadConstants = PreloadConstants::default(); + println!("cargo:rerun-if-changed=build.rs"); - // Export constants as environment variables for the Rust code - println!( - "cargo:rustc-env=CODSPEED_URI_ENV={}", - preload_constants.uri_env - ); - println!( - "cargo:rustc-env=CODSPEED_INTEGRATION_NAME={}", - preload_constants.integration_name - ); + println!("cargo:rustc-env=CODSPEED_INTEGRATION_NAME={INTEGRATION_NAME}"); println!( "cargo:rustc-env=CODSPEED_INTEGRATION_VERSION={}", - preload_constants.integration_version - ); - println!( - "cargo:rustc-env=CODSPEED_PRELOAD_LIB_FILENAME={}", - preload_constants.preload_lib_filename + env!("CARGO_PKG_VERSION") ); - - let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); - - // Try to get the instrument-hooks directory from the environment variable first, - // otherwise use the one from the instrument-hooks-bindings crate - let instrument_hooks_dir = manifest_dir - .parent() - .unwrap() - .join("instrument-hooks-bindings/instrument-hooks"); - - // Build the preload shared library - let paths = PreloadBuildPaths { - preload_c: manifest_dir.join("preload/codspeed_preload.c"), - core_c: instrument_hooks_dir.join("dist/core.c"), - includes_dir: instrument_hooks_dir.join("includes"), - }; - println!("cargo:rerun-if-changed={}", paths.core_c.display()); - paths.check_sources_exist(); - build_shared_library(&paths, &preload_constants); -} - -/// Build the shared library using the cc crate -fn build_shared_library(paths: &PreloadBuildPaths, constants: &PreloadConstants) { - let uri_env_val = format!("\"{}\"", constants.uri_env); - let integration_name_val = format!("\"{}\"", constants.integration_name); - let integration_version_val = format!("\"{}\"", constants.integration_version); - let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); - let out_file = out_dir.join(constants.preload_lib_filename); - - let mut build = cc::Build::new(); - build - .file(&paths.preload_c) - .file(&paths.core_c) - .include(&paths.includes_dir) - .pic(true) - .opt_level(3) - // There's no need to output cargo metadata as we are just building a shared library - // that will be copied to disk and loaded through LD_PRELOAD at runtime - .cargo_metadata(false) - // Pass constants as C defines - .define("CODSPEED_URI_ENV", uri_env_val.as_str()) - .define("CODSPEED_INTEGRATION_NAME", integration_name_val.as_str()) - .define( - "CODSPEED_INTEGRATION_VERSION", - integration_version_val.as_str(), - ) - .std("gnu11") // need gnu11 instead of just c11 for setenv - // Suppress warnings from generated Zig code - .flag("-Wno-format") - .flag("-Wno-format-security") - .flag("-Wno-unused-but-set-variable") - .flag("-Wno-unused-const-variable") - .flag("-Wno-type-limits") - .flag("-Wno-uninitialized") - .flag("-Wno-overflow") - .flag("-Wno-unused-function") - .flag("-Wno-unterminated-string-initialization"); - - // Compile source files to object files - let objects = build.compile_intermediates(); - - // Link object files into shared library - let compiler = build.get_compiler(); - let mut link_cmd = compiler.to_command(); - link_cmd - .arg("-shared") - .arg("-o") - .arg(&out_file) - .args(&objects) - .arg("-lpthread"); - - let status = link_cmd.status().expect("Failed to run linker"); - if !status.success() { - panic!("Failed to link libcodspeed_preload.so"); - } -} - -impl Default for PreloadConstants { - fn default() -> Self { - Self { - uri_env: "CODSPEED_BENCH_URI", - integration_name: "exec-harness", - integration_version: env!("CARGO_PKG_VERSION"), - preload_lib_filename: "libcodspeed_preload.so", - } - } -} - -/// Paths required to build the preload shared library. -struct PreloadBuildPaths { - /// Path to the preload C source file (codspeed_preload.c). - preload_c: PathBuf, - /// Path to the core C source file from instrument-hooks. - core_c: PathBuf, - /// Path to the includes directory from instrument-hooks. - includes_dir: PathBuf, -} - -impl PreloadBuildPaths { - /// Verify that all required source files and directories exist. - /// Panics with a descriptive message if any path is missing. - fn check_sources_exist(&self) { - if !self.core_c.exists() { - panic!( - "core.c not found at {}. Make sure the instrument hooks submodule is available.", - self.core_c.display() - ); - } - if !self.includes_dir.exists() { - panic!( - "includes directory not found at {}. instrument hooks submodule is available.", - self.includes_dir.display() - ); - } - if !self.preload_c.exists() { - panic!( - "codspeed_preload.c not found at {}", - self.preload_c.display() - ); - } - } } diff --git a/crates/exec-harness/preload/codspeed_preload.c b/crates/exec-harness/preload/codspeed_preload.c deleted file mode 100644 index 418af143..00000000 --- a/crates/exec-harness/preload/codspeed_preload.c +++ /dev/null @@ -1,87 +0,0 @@ -// LD_PRELOAD library for enabling Valgrind instrumentation in child processes -// -// This library is loaded via LD_PRELOAD into benchmark processes spawned by -// exec-harness. It enables callgrind instrumentation on load and disables it on -// exit, allowing exec-harness to measure arbitrary commands without requiring -// them to link against instrument-hooks. -// -// Environment variables: -// CODSPEED_BENCH_URI - The benchmark URI to report (required) - -#include -#include - -#include "core.h" - -#ifndef RUNNING_ON_VALGRIND -// If somehow the core.h did not include the valgrind header, something is -// wrong, but still have a fallback -#warning "RUNNING_ON_VALGRIND not defined, headers may be missing" -#define RUNNING_ON_VALGRIND 0 -#endif - -// These constants are defined by the build script (build.rs) via -D flags -#ifndef CODSPEED_URI_ENV -#error "CODSPEED_URI_ENV must be defined by the build system" -#endif -#ifndef CODSPEED_INTEGRATION_NAME -#error "CODSPEED_INTEGRATION_NAME must be defined by the build system" -#endif -#ifndef CODSPEED_INTEGRATION_VERSION -#error "CODSPEED_INTEGRATION_VERSION must be defined by the build system" -#endif - -static const char *URI_ENV = CODSPEED_URI_ENV; -static const char *INTEGRATION_NAME = CODSPEED_INTEGRATION_NAME; -static const char *INTEGRATION_VERSION = CODSPEED_INTEGRATION_VERSION; - -static InstrumentHooks *g_hooks = NULL; -static const char *g_bench_uri = NULL; - -__attribute__((constructor)) static void codspeed_preload_init(void) { - // Skip initialization if not running under Valgrind yet. - // When using LD_PRELOAD with Valgrind, the constructor runs twice: - // once before Valgrind takes over, and once after. We only want to - // initialize when Valgrind is active. - // - // This is purely empirical, and is not (yet) backed up by documented - // behavior. - if (!RUNNING_ON_VALGRIND) { - return; - } - - g_bench_uri = getenv(URI_ENV); - if (!g_bench_uri) { - return; - } - - g_hooks = instrument_hooks_init(); - if (!g_hooks) { - return; - } - - instrument_hooks_set_integration(g_hooks, INTEGRATION_NAME, - INTEGRATION_VERSION); - - if (instrument_hooks_start_benchmark_inline(g_hooks) != 0) { - instrument_hooks_deinit(g_hooks); - g_hooks = NULL; - return; - } -} - -__attribute__((destructor)) static void codspeed_preload_fini(void) { - // If the process is not the owner of the lock, this means g_hooks was not - // initialized - if (!g_hooks) { - return; - } - - instrument_hooks_stop_benchmark_inline(g_hooks); - - int32_t pid = getpid(); - instrument_hooks_set_executed_benchmark(g_hooks, pid, g_bench_uri); - - instrument_hooks_deinit(g_hooks); - g_hooks = NULL; -} diff --git a/crates/exec-harness/src/analysis/ld_preload_check.rs b/crates/exec-harness/src/analysis/ld_preload_check.rs deleted file mode 100644 index 702f18d2..00000000 --- a/crates/exec-harness/src/analysis/ld_preload_check.rs +++ /dev/null @@ -1,120 +0,0 @@ -use crate::prelude::*; -use std::fs; -use std::path::Path; - -/// Checks if the given executable will honor LD_PRELOAD. -/// -/// Returns `Ok(())` if LD_PRELOAD will work, or an error with a descriptive message if not. -/// -/// LD_PRELOAD works for: -/// - Dynamically linked ELF binaries -/// - Scripts (the interpreter is typically dynamically linked) -/// -/// LD_PRELOAD does NOT work for: -/// - Statically linked ELF binaries (no dynamic linker involved) -pub fn check_ld_preload_compatible(executable: &str) -> Result<()> { - let path = resolve_executable(executable)?; - let data = fs::read(&path) - .with_context(|| format!("Failed to read executable: {}", path.display()))?; - - // Check ELF magic bytes - if data.len() >= 4 && &data[0..4] == b"\x7FELF" { - check_elf_is_dynamic(&data, &path) - } else { - // Not an ELF file - likely a script with a shebang. - // Scripts use an interpreter which is typically dynamically linked. - Ok(()) - } -} - -/// Resolve executable name to its full path using PATH lookup. -fn resolve_executable(executable: &str) -> Result { - let path = Path::new(executable); - - // If it's already an absolute or relative path, use it directly - if path.is_absolute() || executable.contains('/') { - return Ok(path.to_path_buf()); - } - - // Search in PATH - if let Ok(path_env) = std::env::var("PATH") { - for dir in path_env.split(':') { - let candidate = Path::new(dir).join(executable); - if candidate.is_file() { - return Ok(candidate); - } - } - } - - bail!("Executable not found in PATH: {executable}") -} - -/// Check if an ELF binary is dynamically linked. -fn check_elf_is_dynamic(data: &[u8], path: &Path) -> Result<()> { - use object::Endianness; - use object::read::elf::ElfFile; - - // Try parsing as 64-bit ELF first, then 32-bit - if let Ok(elf) = ElfFile::>::parse(data) { - return check_elf_has_interp(elf, path); - } - - if let Ok(elf) = ElfFile::>::parse(data) { - return check_elf_has_interp(elf, path); - } - - bail!("Failed to parse ELF file: {}", path.display()) -} - -/// Check if an ELF file has a PT_INTERP or PT_DYNAMIC segment, indicating dynamic linking. -fn check_elf_has_interp<'data, Elf>( - elf: object::read::elf::ElfFile<'data, Elf>, - path: &Path, -) -> Result<()> -where - Elf: object::read::elf::FileHeader, -{ - use object::read::elf::ProgramHeader; - - let endian = elf.endian(); - - for segment in elf.elf_program_headers() { - let p_type = segment.p_type(endian); - // Either PT_INTERP or PT_DYNAMIC indicates a dynamically linked binary - if p_type == object::elf::PT_INTERP || p_type == object::elf::PT_DYNAMIC { - return Ok(()); - } - } - - // No PT_INTERP found - this is a statically linked binary - bail!( - "The codspeed CLI in CPU Simulation mode does not support statically linked binaries.\n\n\ - Executable '{}' is statically linked.\n\n\ - Please either:\n\ - - Use a dynamically linked executable, or\n\ - - Use a different measurement mode, or\n\ - - Use one of the CodSpeed framework benchmark integrations", - path.display() - ) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_dynamic_binary() { - // /bin/sh or similar should be dynamically linked on most systems - let result = check_ld_preload_compatible("sh"); - assert!( - result.is_ok(), - "sh should be dynamically linked: {result:?}" - ); - } - - #[test] - fn test_nonexistent_binary() { - let result = check_ld_preload_compatible("nonexistent_binary_12345"); - assert!(result.is_err()); - } -} diff --git a/crates/exec-harness/src/analysis/mod.rs b/crates/exec-harness/src/analysis/mod.rs index 8bb4eaf4..23d73657 100644 --- a/crates/exec-harness/src/analysis/mod.rs +++ b/crates/exec-harness/src/analysis/mod.rs @@ -1,25 +1,62 @@ +use crate::MeasurementMode; use crate::constants::INTEGRATION_NAME; use crate::constants::INTEGRATION_VERSION; use crate::prelude::*; use crate::BenchmarkCommand; -use crate::constants; use crate::uri; use instrument_hooks_bindings::InstrumentHooks; use std::process::Command; -mod ld_preload_check; -mod preload_lib_file; - -pub fn perform(commands: Vec) -> Result<()> { +/// Executes the given benchmark commands, measuring each one through the +/// instrument hooks. +/// +/// Instrumentation is toggled in *this* process, around the spawn of each +/// benchmark command. Under Valgrind, the benchmarked child inherits the live +/// instrumentation state across `fork`/`exec`, and callgrind records the spawn +/// edge on the dump part that is live at fork time — the same part that +/// [`InstrumentHooks::set_executed_benchmark`] then names with the benchmark +/// URI. The backend walks that edge to attribute the child's trace to the +/// benchmark, so the measurement covers the whole spawned process tree. +/// +/// This replaces the previous `LD_PRELOAD` shared library, which started +/// instrumentation from inside the benchmark process because the state did not +/// use to propagate across `fork`. Dropping it means statically linked +/// executables are now supported, since nothing has to be injected into them. +pub fn perform(commands: Vec, mode: MeasurementMode) -> Result<()> { let hooks = InstrumentHooks::instance(INTEGRATION_NAME, INTEGRATION_VERSION); + if !hooks.is_instrumented() { + // Every way this mode can go wrong is silent: the harness runs, the + // benchmark completes, and the measurement is empty. Fail loudly + // instead. + // + // Note this only catches the absence of *any* instrument (no + // instrument-hooks support compiled in, or nothing to attach to). It + // cannot tell whether Valgrind will actually honour the instrumentation + // toggles, which depends on the `--instr-atstart` the runner passes. + bail!( + "exec-harness found no instrument to report to, so nothing would be measured.\n\ + This binary is meant to be run by the CodSpeed CLI, which sets up the \ + instrumentation around it." + ); + } + for benchmark_cmd in commands { let name_and_uri = uri::generate_name_and_uri(&benchmark_cmd.name, &benchmark_cmd.command); name_and_uri.print_executing(); let mut cmd = Command::new(&benchmark_cmd.command[0]); cmd.args(&benchmark_cmd.command[1..]); + + if mode == MeasurementMode::Simulation { + // Make sure python and node processes output perf maps, so the + // runner can resolve JIT-ed frames afterwards. For python this is + // usually done by `pytest-codspeed`. + cmd.env("PYTHONPERFSUPPORT", "1"); + crate::node::set_node_options(&mut cmd); + } + hooks.start_benchmark().unwrap(); let status = cmd.status(); hooks.stop_benchmark().unwrap(); @@ -34,40 +71,3 @@ pub fn perform(commands: Vec) -> Result<()> { Ok(()) } - -/// Executes the given benchmark commands using a preload based trick to handle valgrind control. -/// -/// This function is only supported on Unix-like platforms, as it relies on the -/// `LD_PRELOAD` environment variable and Unix file permissions for shared libraries. -/// It will not work on non-Unix platforms or with statically linked binaries. -pub fn perform_with_valgrind(commands: Vec) -> Result<()> { - let preload_lib_path = preload_lib_file::get_preload_lib_path()?; - - for benchmark_cmd in commands { - // Check if the executable will honor LD_PRELOAD before running - ld_preload_check::check_ld_preload_compatible(&benchmark_cmd.command[0])?; - - let name_and_uri = uri::generate_name_and_uri(&benchmark_cmd.name, &benchmark_cmd.command); - name_and_uri.print_executing(); - - let mut cmd = Command::new(&benchmark_cmd.command[0]); - cmd.args(&benchmark_cmd.command[1..]); - // Use LD_PRELOAD to inject instrumentation into the child process - cmd.env("LD_PRELOAD", preload_lib_path); - // Make sure python processes output perf maps. This is usually done by `pytest-codspeed` - cmd.env("PYTHONPERFSUPPORT", "1"); - cmd.env(constants::URI_ENV, &name_and_uri.uri); - - crate::node::set_node_options(&mut cmd); - - let mut child = cmd.spawn().context("Failed to spawn command")?; - - let status = child.wait().context("Failed to execute command")?; - - if !status.success() { - bail!("Command exited with non-zero status: {status}"); - } - } - - Ok(()) -} diff --git a/crates/exec-harness/src/analysis/preload_lib_file.rs b/crates/exec-harness/src/analysis/preload_lib_file.rs deleted file mode 100644 index 2d53804c..00000000 --- a/crates/exec-harness/src/analysis/preload_lib_file.rs +++ /dev/null @@ -1,46 +0,0 @@ -use crate::prelude::*; - -use std::io::Write; -use std::sync::OnceLock; - -/// Filename for the preload shared library. -const PRELOAD_LIB_FILENAME: &str = env!("CODSPEED_PRELOAD_LIB_FILENAME"); - -/// The preload library binary embedded at compile time. -const PRELOAD_LIB_BYTES: &[u8] = include_bytes!(concat!( - env!("OUT_DIR"), - "/", - env!("CODSPEED_PRELOAD_LIB_FILENAME") -)); - -/// Lazily initialized temp file containing the extracted preload library. -/// Kept in a static to prevent cleanup until process exit. -static PRELOAD_LIB_FILE: OnceLock = OnceLock::new(); - -/// Extracts the preload library to a temp file. -fn extract_preload_lib() -> Result { - let mut file = tempfile::Builder::new() - .suffix(PRELOAD_LIB_FILENAME) - .tempfile() - .context("Failed to create temp file for preload library")?; - - file.write_all(PRELOAD_LIB_BYTES) - .context("Failed to write preload library to temp file")?; - - debug!( - "Extracted preload library to temp file: {}", - file.path().display() - ); - - Ok(file) -} - -/// Returns the path to the preload library, extracting it to a temp file if needed. -pub(super) fn get_preload_lib_path() -> Result<&'static std::path::Path> { - if let Some(file) = PRELOAD_LIB_FILE.get() { - return Ok(file.path()); - } - - let file = extract_preload_lib()?; - Ok(PRELOAD_LIB_FILE.get_or_init(|| file).path()) -} diff --git a/crates/exec-harness/src/constants.rs b/crates/exec-harness/src/constants.rs index 9a47591c..f982f395 100644 --- a/crates/exec-harness/src/constants.rs +++ b/crates/exec-harness/src/constants.rs @@ -1,11 +1,8 @@ //! Shared constants for the exec-harness crate. //! //! These constants are defined in the build script (build.rs) and exported as -//! environment variables. The same values are passed to the C preload library -//! as compiler defines, ensuring both Rust and C code use the same source of truth. - -/// Environment variable name for the benchmark URI. -pub const URI_ENV: &str = env!("CODSPEED_URI_ENV"); +//! environment variables, so that the integration identity reported to CodSpeed +//! has a single source of truth. /// Integration name reported to CodSpeed. pub const INTEGRATION_NAME: &str = env!("CODSPEED_INTEGRATION_NAME"); diff --git a/crates/exec-harness/src/lib.rs b/crates/exec-harness/src/lib.rs index 30cb21b4..28e52d58 100644 --- a/crates/exec-harness/src/lib.rs +++ b/crates/exec-harness/src/lib.rs @@ -74,11 +74,8 @@ pub fn execute_benchmarks( Some(MeasurementMode::Walltime) | None => { walltime::perform(commands)?; } - Some(MeasurementMode::Memory) => { - analysis::perform(commands)?; - } - Some(MeasurementMode::Simulation) => { - analysis::perform_with_valgrind(commands)?; + Some(mode @ (MeasurementMode::Memory | MeasurementMode::Simulation)) => { + analysis::perform(commands, mode)?; } } diff --git a/src/cli/shared.rs b/src/cli/shared.rs index 1fe13474..b926af04 100644 --- a/src/cli/shared.rs +++ b/src/cli/shared.rs @@ -135,7 +135,11 @@ pub struct ExecAndRunSharedArgs { )] pub exclude_allocations: bool, - /// Measure the subprocesses spawned by the benchmarked process in simulation mode. + /// Emit per-thread dumps for the benchmarked process in simulation mode. + /// + /// Subprocesses spawned by the benchmarked process are now always measured, + /// so this only controls Valgrind's `--separate-threads`. The flag keeps its + /// name for compatibility; renaming it would break existing invocations. #[arg(long, env = "CODSPEED_SIMULATION_TRACK_SUBPROCESS")] pub simulation_track_subprocess: bool, diff --git a/src/executor/valgrind/measure.rs b/src/executor/valgrind/measure.rs index 62807b92..5c395753 100644 --- a/src/executor/valgrind/measure.rs +++ b/src/executor/valgrind/measure.rs @@ -33,11 +33,16 @@ fn get_valgrind_args(tool: &SimulationTool, config: &ExecutorConfig) -> Vec Date: Mon, 7 Sep 2026 12:29:59 +0200 Subject: [PATCH 04/10] fix(instrument-hooks): never fall back to the noop impl on Linux When cc-rs fails to compile the native library, the build script printed a `cargo:warning` and compiled the noop `InstrumentHooks` instead, in which every hook returns `Ok(())`. A build that landed there ran benchmarks and measured nothing, at exit code 0. That is reachable by accident: building for a musl target without a musl C compiler on PATH is enough, which the exec-harness musl port makes a routine thing to do. Make it a build failure on Linux, where we actually measure, and point at the missing toolchain. Other platforms keep the warning so macOS dev builds are unaffected. Co-Authored-By: Claude Opus 5 (1M context) --- crates/instrument-hooks-bindings/build.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/instrument-hooks-bindings/build.rs b/crates/instrument-hooks-bindings/build.rs index 63b46664..eb6611e8 100644 --- a/crates/instrument-hooks-bindings/build.rs +++ b/crates/instrument-hooks-bindings/build.rs @@ -1,3 +1,5 @@ +use std::env; + fn main() { println!("cargo:rustc-check-cfg=cfg(use_instrument_hooks)"); @@ -35,6 +37,24 @@ fn main() { Err(e) => { let compiler = build.try_get_compiler().expect("Failed to get C compiler"); + // Falling back to the noop implementation makes every hook a + // no-op, so a build that lands there runs benchmarks and reports + // no measurement at all, at exit code 0. Linux is where we + // actually measure, so fail the build instead of emitting a + // warning nobody reads. + if env::var("CARGO_CFG_TARGET_OS").as_deref() == Ok("linux") { + panic!( + "Failed to compile the instrument-hooks native library with cc-rs.\n\ + A Linux build must not fall back to the noop implementation: it \ + would run benchmarks and measure nothing.\n\ + Make sure a C compiler for the target is installed and reachable \ + by cc-rs (for musl targets, `musl-tools` provides \ + `-linux-musl-gcc`).\n\ + Compiler information: {compiler:?}\n\ + Compilation error: {e}" + ); + } + eprintln!("\n\nWARNING: Failed to compile instrument-hooks native library with cc-rs."); eprintln!( "The library will still compile, but instrument-hooks functionality will be disabled." From edec5719d27de13603615c4cbb36912d36c189f8 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 7 Sep 2026 12:36:43 +0200 Subject: [PATCH 05/10] ci: add throwaway COD-3218 exec-harness check workflow Removing the preload moves the callgrind client requests out of the benchmark child and up into exec-harness, so the measurement now rests on valgrind propagating instrumentation state across fork/exec and on the spawn edges valgrind-codspeed records. None of that is observable on the dev host (aarch64 Arch, no valgrind, and the CodSpeed .deb is Ubuntu-only), so this runs it on a real x86_64 runner. Asserts on the content of the .out files rather than the exit code, since the failure mode being guarded against is a run that completes happily and measures nothing: a part must carry the benchmark URI, that part must list the spawn edge, and every process in the chain must have its own .out with non-zero cost. The benchmark deliberately nests spawns (exec-harness -> sh -> seq/wc) so the chain is walked, not just one edge. Two guards against the check passing vacuously: the run is bracketed by a hash of the exec-harness on PATH, because the runner silently downloads the released preload build when the local one is missing, and the musl leg asserts the installed binary really is static. A baseline job runs the same benchmark on main to quantify the step change in reported cost that dropping the preload causes. Manual trigger only, and it must not reach main -- the header says so. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/cod-3218-exec-harness-check.yml | 298 ++++++++++++++++++ 1 file changed, 298 insertions(+) create mode 100644 .github/workflows/cod-3218-exec-harness-check.yml diff --git a/.github/workflows/cod-3218-exec-harness-check.yml b/.github/workflows/cod-3218-exec-harness-check.yml new file mode 100644 index 00000000..66cbfae5 --- /dev/null +++ b/.github/workflows/cod-3218-exec-harness-check.yml @@ -0,0 +1,298 @@ +# COD-3218 spike — throwaway workflow, NOT for merging. +# +# Purpose: close the one gap the local work could not. Removing the LD_PRELOAD +# hack moves the callgrind client requests from inside the benchmark child up +# into exec-harness, so the measurement now depends on valgrind propagating +# instrumentation state across fork/exec and on the spawn edges recorded by +# valgrind-codspeed. None of that is observable locally: this host is aarch64 +# Arch with no valgrind, and the CodSpeed valgrind .deb is published for Ubuntu +# only. It also runs the whole flow with a *musl* exec-harness, which is the +# COD-3440 half. +# +# Exit code 0 proves nothing here: the harness runs, valgrind runs, the +# benchmark completes, and the measurement can still be empty. So this workflow +# asserts on the CONTENT of the .out files, per the verification bar: +# - a part carries the benchmark URI, and lists `desc: Spawned pid: ` +# - every process in the spawn chain has its own .out with non-zero cost +# - the summed cost is printed, and the baseline job prints the same number +# from main so the step change can be quantified (breaking change #1) +# +# To use it: push this branch, then +# +# gh workflow run cod-3218-exec-harness-check.yml --ref spike/cod-3440-memtrack-musl +# +# Manual trigger only, so it never fires on its own. Note that the very first +# dispatch of a workflow living only on a non-default branch 404s until GitHub +# has registered it; once it shows up in the Actions list, --ref dispatch works. +# Delete the file once the question is answered -- it must not reach main. +# +# Deliberately does NOT touch ci.yml, release.yml, dist-workspace.toml or any +# Cargo.toml: it only adds manually-triggered jobs. + +name: COD-3218 exec-harness check + +on: + workflow_dispatch: + +env: + MUSL_TARGET: x86_64-unknown-linux-musl + # Distinctive so it can be grepped out of the .out files unambiguously. The + # harness derives the URI as `exec_harness::`. + BENCH_NAME: cod3218_probe + # Passed as `sh -c "$BENCH_SCRIPT"`, so it holds no quoting of its own -- an + # env var cannot carry shell quotes through word splitting. + # A nested spawn on purpose: the harness forks `sh`, which forks `seq` and + # `wc`. That exercises the intermediate-forwarding case in the backend's + # spawn-chain walk, not just a single parent -> child edge. + BENCH_SCRIPT: seq 1 50000 | wc -l + +jobs: + # The COD-3440 half: the artifact the preload removal unblocks. + build-musl: + name: musl build is static + runs-on: ubuntu-latest # x86_64 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: cod3218-musl + + - name: Install musl toolchain + # musl-tools provides x86_64-linux-musl-gcc, which cc-rs finds unaided, + # so instrument-hooks' core.c compiles for the target. Without it the + # bindings build script now fails the build outright rather than + # silently compiling the noop implementation. + run: | + sudo apt-get install -y musl-tools + rustup target add "$MUSL_TARGET" + + - name: Build + run: cargo build -p exec-harness --target "$MUSL_TARGET" + + - name: Verify the artifact is genuinely static + run: | + BIN=target/$MUSL_TARGET/debug/exec-harness + file "$BIN" + ldd "$BIN" || true # expected: "not a dynamic executable" + readelf -d "$BIN" || true # expected: no dynamic section at all + echo "size: $(stat -c %s "$BIN") bytes" + # `if` rather than `grep ... && exit 1`, because a grep that matches + # nothing exits 1 and would fail the step under bash -e. + if readelf -d "$BIN" 2>/dev/null | grep -qE 'NEEDED|RPATH|RUNPATH'; then + echo "unexpected dynamic dependency or rpath" + exit 1 + fi + echo "OK: static, no NEEDED, no RPATH/RUNPATH" + + - name: Verify no preload artifact is produced any more + run: | + if find target -name 'libcodspeed_preload*' | grep .; then + echo "the preload library is still being built" + exit 1 + fi + echo "OK: no preload library in the build output" + + # The COD-3218 half: does the measurement actually land anywhere? + instrumentation: + name: instrumentation (${{ matrix.libc }}) + runs-on: ubuntu-latest # x86_64 + strategy: + fail-fast: false + matrix: + libc: [gnu, musl] + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: cod3218-${{ matrix.libc }} + + - name: Install musl toolchain + if: matrix.libc == 'musl' + run: | + sudo apt-get install -y musl-tools + rustup target add "$MUSL_TARGET" + + # The runner resolves exec-harness off PATH and only downloads the + # released build when `which exec-harness` is missing or reports a + # version other than the pin (src/binary_installer/mod.rs). Installing + # our build first is therefore what makes this job test anything -- see + # the tamper guard in the next step. + - name: Install the exec-harness under test + run: | + if [ "${{ matrix.libc }}" = "musl" ]; then + cargo install --path crates/exec-harness --locked --target "$MUSL_TARGET" + else + cargo install --path crates/exec-harness --locked + fi + BIN=$(which exec-harness) + echo "$BIN" + exec-harness --version + file "$BIN" + # Prove the musl matrix leg is really exercising the musl artifact, + # rather than a leftover gnu build earlier on PATH. + if [ "${{ matrix.libc }}" = "musl" ]; then + file "$BIN" | grep -q 'statically linked' \ + || { echo "the installed exec-harness is not static"; exit 1; } + echo "OK: the exec-harness under test is statically linked" + fi + + - name: Run a simulation-mode benchmark + id: run + run: | + PROFILE_DIR=$RUNNER_TEMP/profile + mkdir -p "$PROFILE_DIR" + echo "profile_dir=$PROFILE_DIR" >> "$GITHUB_OUTPUT" + + # If the runner swapped in the *released* exec-harness, that would be + # the preload build and this whole job would pass while proving + # nothing. Pin the binary's hash across the run. + BEFORE=$(sha256sum "$(which exec-harness)" | cut -d' ' -f1) + + CODSPEED_LOG=debug cargo run -- exec \ + -m simulation \ + --skip-upload \ + --profile-folder "$PROFILE_DIR" \ + --name "$BENCH_NAME" \ + -- sh -c "$BENCH_SCRIPT" + + AFTER=$(sha256sum "$(which exec-harness)" | cut -d' ' -f1) + if [ "$BEFORE" != "$AFTER" ]; then + echo "the runner replaced exec-harness with the released (preload) build" + echo "=> this run measured the old code path, not the change under test" + exit 1 + fi + echo "OK: the exec-harness under test was the one used" + + - name: Show what was produced + if: always() && steps.run.outputs.profile_dir != '' + run: | + PROFILE_DIR=${{ steps.run.outputs.profile_dir }} + echo "=== files ===" + find "$PROFILE_DIR" -type f -printf '%10s %p\n' | sort -k2 + echo + echo "=== headers of every callgrind out file ===" + # Printed in full and unfiltered on purpose: the exact header spelling + # of a client-request dump is what the backend's parser keys off, and + # eyeballing it here is cheaper than guessing at it from this repo. + find "$PROFILE_DIR" -name '*.out*' -type f | sort | while read -r f; do + echo "--- $f" + grep -nE '^(version|creator|pid|part|desc|cmd|events|summary|totals):' "$f" || true + echo + done + echo "=== valgrind logs ===" + find "$PROFILE_DIR" -name 'valgrind.*.log' -type f -exec tail -n 30 {} + || true + + - name: Verify the URI is attributed and every spawned process has cost + run: | + PROFILE_DIR=${{ steps.run.outputs.profile_dir }} + URI="exec_harness::$BENCH_NAME" + + fail() { echo "FAIL: $*"; exit 1; } + # awk rather than `grep | awk`: awk always exits 0, so a benchmark + # that recorded nothing reaches the explicit check below instead of + # aborting the step through pipefail with no diagnosis. + cost_of() { awk '/^(summary|totals):/ {s+=$2} END {print s+0}' "$@"; } + + mapfile -t OUTS < <(find "$PROFILE_DIR" -name '*.out*' -type f | sort) + echo "found ${#OUTS[@]} callgrind out file(s)" + [ "${#OUTS[@]}" -ge 2 ] || fail \ + "expected at least two out files (exec-harness plus the benchmark child), got ${#OUTS[@]}" + + # 1. Some part must carry the benchmark URI. Without this the cost + # exists but is anonymous, and the backend has nothing to attribute + # it to. + mapfile -t URI_FILES < <(grep -lF -- "$URI" "${OUTS[@]}") + [ "${#URI_FILES[@]}" -ge 1 ] || fail "no out file mentions the benchmark URI '$URI'" + echo "OK: URI '$URI' found in: ${URI_FILES[*]}" + + # 2. That same file must record the spawn edge to the benchmark child. + # Asserted per file rather than per part: splitting parts apart in + # shell is not worth it, and the headers printed above show the + # part association for a human to confirm. + URI_FILE=${URI_FILES[0]} + mapfile -t SPAWNED < <(grep -oiE 'Spawned pid:[[:space:]]*[0-9]+' "$URI_FILE" \ + | grep -oE '[0-9]+' | sort -u) + [ "${#SPAWNED[@]}" -ge 1 ] || fail \ + "the URI-bearing part in $URI_FILE records no 'Spawned pid:' edge, so the child's cost cannot be attributed to the benchmark" + echo "OK: $URI_FILE spawned pid(s): ${SPAWNED[*]}" + + # 3. Walk the whole spawn chain. Each process must have its own out + # file with non-zero cost, and may itself have spawned more + # (here: exec-harness -> sh -> seq/wc). + declare -A SEEN=() + WORK=("${SPAWNED[@]}") + while [ "${#WORK[@]}" -gt 0 ]; do + PID=${WORK[0]} + WORK=("${WORK[@]:1}") + # `if` rather than `[ ... ] && continue`, which returns non-zero on + # the miss and would abort the step under bash -e. + if [ -n "${SEEN[$PID]:-}" ]; then + continue + fi + SEEN[$PID]=1 + + mapfile -t CHILD < <(find "$PROFILE_DIR" -name "$PID.out*" -type f) + [ "${#CHILD[@]}" -ge 1 ] || fail \ + "spawned pid $PID has no out file, so its cost was never recorded" + + COST=$(cost_of "${CHILD[@]}") + [ "$COST" -gt 0 ] || fail "spawned pid $PID recorded zero cost (file: ${CHILD[*]})" + echo "OK: pid $PID -> ${CHILD[*]} (Ir: $COST)" + + mapfile -t MORE < <(grep -hoiE 'Spawned pid:[[:space:]]*[0-9]+' "${CHILD[@]}" \ + | grep -oE '[0-9]+' | sort -u) + if [ "${#MORE[@]}" -gt 0 ]; then + WORK+=("${MORE[@]}") + fi + done + + echo + echo "PASS: URI attributed, ${#SEEN[@]} spawned process(es) all carry cost" + echo "TOTAL_IR(${{ matrix.libc }})=$(cost_of "${OUTS[@]}")" + + # The baseline for breaking change #1: the same benchmark on main, where the + # preload starts the measured region inside the child instead. The delta + # between this number and the one above IS the step change in reported cost. + baseline-main: + name: baseline on main (preload) + runs-on: ubuntu-latest # x86_64 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: main + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: cod3218-baseline + + - name: Install the exec-harness from main + run: | + cargo install --path crates/exec-harness --locked + exec-harness --version + + - name: Run the same benchmark + # main's exec-harness is the preload build, and its LD_PRELOAD check + # rejects statically linked executables -- so this leg is gnu only. + run: | + PROFILE_DIR=$RUNNER_TEMP/profile + mkdir -p "$PROFILE_DIR" + + CODSPEED_LOG=debug cargo run -- exec \ + -m simulation \ + --skip-upload \ + --profile-folder "$PROFILE_DIR" \ + --name "$BENCH_NAME" \ + -- sh -c "$BENCH_SCRIPT" + + find "$PROFILE_DIR" -type f -printf '%10s %p\n' | sort -k2 + find "$PROFILE_DIR" -name '*.out*' -type f | sort | while read -r f; do + echo "--- $f" + grep -nE '^(pid|part|desc|cmd|summary|totals):' "$f" || true + done + mapfile -t OUTS < <(find "$PROFILE_DIR" -name '*.out*' -type f | sort) + TOTAL=$(awk '/^(summary|totals):/ {s+=$2} END {print s+0}' "${OUTS[@]}") + echo "TOTAL_IR(main-preload)=$TOTAL" From 0e587540d9a1bb6b78af0d179173b012a5b5cfc4 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 7 Sep 2026 12:50:03 +0200 Subject: [PATCH 06/10] ci: trigger the COD-3218 check on spike branch pushes `gh workflow run` answers "HTTP 404: workflow not found on the default branch" for a workflow_dispatch-only file that has never existed on main, and it stays that way: GitHub does not register such a file on its own, so waiting and retrying the dispatch gets nowhere. A push trigger is what forces registration -- GitHub runs the file on push and assigns it an id, after which --ref dispatch works too. The COD-3440 workflow next door was registered exactly this way; its first run is a `push` one from a commit that temporarily added the same trigger. Recorded in the `on:` block so the next person does not rediscover it. Scoped to the spike branch, and it goes away with the file. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/cod-3218-exec-harness-check.yml | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/.github/workflows/cod-3218-exec-harness-check.yml b/.github/workflows/cod-3218-exec-harness-check.yml index 66cbfae5..697ed556 100644 --- a/.github/workflows/cod-3218-exec-harness-check.yml +++ b/.github/workflows/cod-3218-exec-harness-check.yml @@ -17,22 +17,36 @@ # - the summed cost is printed, and the baseline job prints the same number # from main so the step change can be quantified (breaking change #1) # -# To use it: push this branch, then +# To use it: push this branch. The push itself runs the workflow -- see the +# `on:` block for why a push trigger is required rather than optional. After +# that first run has registered the file, it can also be re-run by hand: # # gh workflow run cod-3218-exec-harness-check.yml --ref spike/cod-3440-memtrack-musl # -# Manual trigger only, so it never fires on its own. Note that the very first -# dispatch of a workflow living only on a non-default branch 404s until GitHub -# has registered it; once it shows up in the Actions list, --ref dispatch works. -# Delete the file once the question is answered -- it must not reach main. +# The push trigger is scoped to the spike branch, so it cannot fire anywhere +# else. Delete the file once the question is answered -- it must not reach main. # # Deliberately does NOT touch ci.yml, release.yml, dist-workspace.toml or any -# Cargo.toml: it only adds manually-triggered jobs. +# Cargo.toml: it only adds jobs that run on this branch. name: COD-3218 exec-harness check on: workflow_dispatch: + # The push trigger is what makes this workflow dispatchable at all, and it is + # not optional. A workflow_dispatch-only file that has never existed on the + # default branch is never registered by GitHub: `gh workflow run` answers + # "HTTP 404: workflow ... not found on the default branch" indefinitely, and + # it does NOT register itself over time -- retrying is useless. A push trigger + # forces registration, because GitHub runs the file on push and assigns it an + # id, after which `--ref` dispatch works too. The COD-3440 workflow next door + # was registered exactly this way; its first run is a `push` one from a commit + # that temporarily added this same trigger. + # + # Scoped to the spike branch so it cannot fire anywhere else, and it goes away + # when this throwaway file is deleted. + push: + branches: [spike/cod-3440-memtrack-musl] env: MUSL_TARGET: x86_64-unknown-linux-musl From eeba299762d385670384f3c5bfe588c01df68c5d Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 7 Sep 2026 13:01:19 +0200 Subject: [PATCH 07/10] ci: fix three wrong assertions in the COD-3218 check The first run found all three; `instrumentation (gnu)` passed and met the verification bar, so the change itself is fine. 1. The musl leg asserted `file` says "statically linked". rustc emits a static-PIE for x86_64 musl, which `file` calls "static-pie linked"; only aarch64 gets the non-PIE spelling, which is why this passed locally and failed on CI. The binary was static all along -- the `build-musl` job's readelf check confirms no NEEDED/RPATH. Assert through readelf instead: no DT_NEEDED and no interpreter, which is the property we mean. 2. Cost was summed over `summary:` AND `totals:`. Child dumps carry both with near-equal values, so their cost was counted twice -- and only for some files, which inflated the branch total to 11508827 against main's 4819368 and made the comparison meaningless. On `totals:` alone it is 5924410 vs 4688549. Same fix in the baseline job. 3. The URI and the spawn edge were only required to be in the same FILE. The real dumps put both on the same PART, which is the invariant the backend walks: attribution starts at the URI-bearing part and follows its edges, so an edge on a neighbouring part would not attribute anything. Extract the spawn pids from the URI-bearing part itself. Also prints a per-file cost breakdown, so the comparison can be read without digging through the headers. Re-tested against synthetic dumps in the shape the run actually produced: the happy path passes and six failure modes each fail with the right diagnosis, including the new same-file-different-part case that 3. adds. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/cod-3218-exec-harness-check.yml | 76 ++++++++++++++----- 1 file changed, 58 insertions(+), 18 deletions(-) diff --git a/.github/workflows/cod-3218-exec-harness-check.yml b/.github/workflows/cod-3218-exec-harness-check.yml index 697ed556..87b64467 100644 --- a/.github/workflows/cod-3218-exec-harness-check.yml +++ b/.github/workflows/cod-3218-exec-harness-check.yml @@ -147,11 +147,22 @@ jobs: exec-harness --version file "$BIN" # Prove the musl matrix leg is really exercising the musl artifact, - # rather than a leftover gnu build earlier on PATH. + # rather than a leftover gnu build earlier on PATH. Asserted through + # readelf and not a `file` string: rustc emits a static-PIE for + # x86_64 musl, which `file` calls "static-pie linked" rather than + # "statically linked" (aarch64 gets the non-PIE spelling), so matching + # that wording tests the wrong axis and fails on a perfectly static + # binary. What matters is that nothing is loaded at runtime. if [ "${{ matrix.libc }}" = "musl" ]; then - file "$BIN" | grep -q 'statically linked' \ - || { echo "the installed exec-harness is not static"; exit 1; } - echo "OK: the exec-harness under test is statically linked" + if readelf -d "$BIN" 2>/dev/null | grep -qE 'NEEDED|RPATH|RUNPATH'; then + echo "the installed exec-harness has a dynamic dependency" + exit 1 + fi + if readelf -lW "$BIN" 2>/dev/null | grep -q 'INTERP'; then + echo "the installed exec-harness requests a dynamic loader" + exit 1 + fi + echo "OK: no NEEDED and no interpreter -- nothing is loaded at runtime" fi - name: Run a simulation-mode benchmark @@ -206,10 +217,30 @@ jobs: URI="exec_harness::$BENCH_NAME" fail() { echo "FAIL: $*"; exit 1; } - # awk rather than `grep | awk`: awk always exits 0, so a benchmark - # that recorded nothing reaches the explicit check below instead of - # aborting the step through pipefail with no diagnosis. - cost_of() { awk '/^(summary|totals):/ {s+=$2} END {print s+0}' "$@"; } + + # `totals:` only, NOT `summary:`. Child dumps carry both, with nearly + # equal values (a part's summary and the file's totals), so summing + # both silently doubles the reported cost -- and it doubles it only + # for some files, which made the branch/main comparison meaningless. + # awk rather than `grep | awk` because awk always exits 0, so a + # benchmark that recorded nothing reaches the explicit check below + # instead of aborting the step through pipefail with no diagnosis. + cost_of() { awk '/^totals:/ {s+=$2} END {print s+0}' "$@"; } + + # Spawned pids of the dump part that carries the URI. This is the + # invariant the backend actually walks: the URI and the spawn edge + # have to be on the SAME part, not merely in the same file, since + # attribution starts from the URI-bearing part and follows its edges. + spawns_of_uri_part() { + awk -v uri="$1" ' + /^part: / { if (hasuri && pids != "") { print pids; found=1; exit } + hasuri=0; pids=""; next } + index($0, "Client Request: " uri) { hasuri=1 } + /^desc: Spawned pid:/ { p=$0; sub(/.*Spawned pid:[[:space:]]*/,"",p) + pids = pids " " p } + END { if (!found && hasuri && pids != "") print pids } + ' "$2" + } mapfile -t OUTS < <(find "$PROFILE_DIR" -name '*.out*' -type f | sort) echo "found ${#OUTS[@]} callgrind out file(s)" @@ -218,21 +249,18 @@ jobs: # 1. Some part must carry the benchmark URI. Without this the cost # exists but is anonymous, and the backend has nothing to attribute - # it to. + # it to. Post-preload this is exec-harness's own dump: the children + # are NOT labelled any more, which is exactly why 2. and 3. matter. mapfile -t URI_FILES < <(grep -lF -- "$URI" "${OUTS[@]}") [ "${#URI_FILES[@]}" -ge 1 ] || fail "no out file mentions the benchmark URI '$URI'" echo "OK: URI '$URI' found in: ${URI_FILES[*]}" - # 2. That same file must record the spawn edge to the benchmark child. - # Asserted per file rather than per part: splitting parts apart in - # shell is not worth it, and the headers printed above show the - # part association for a human to confirm. + # 2. The URI-bearing part must record the spawn edge to the child. URI_FILE=${URI_FILES[0]} - mapfile -t SPAWNED < <(grep -oiE 'Spawned pid:[[:space:]]*[0-9]+' "$URI_FILE" \ - | grep -oE '[0-9]+' | sort -u) + read -r -a SPAWNED <<< "$(spawns_of_uri_part "$URI" "$URI_FILE")" [ "${#SPAWNED[@]}" -ge 1 ] || fail \ - "the URI-bearing part in $URI_FILE records no 'Spawned pid:' edge, so the child's cost cannot be attributed to the benchmark" - echo "OK: $URI_FILE spawned pid(s): ${SPAWNED[*]}" + "the URI-bearing part of $URI_FILE records no 'Spawned pid:' edge, so the child's cost cannot be attributed to the benchmark" + echo "OK: the URI-bearing part of $URI_FILE spawned pid(s): ${SPAWNED[*]}" # 3. Walk the whole spawn chain. Each process must have its own out # file with non-zero cost, and may itself have spawned more @@ -266,6 +294,11 @@ jobs: echo echo "PASS: URI attributed, ${#SEEN[@]} spawned process(es) all carry cost" + # Per-file breakdown, so the branch/main comparison can be read + # without digging through the headers above. + for f in "${OUTS[@]}"; do + printf ' %-14s Ir=%s\n' "$(basename "$f")" "$(cost_of "$f")" + done echo "TOTAL_IR(${{ matrix.libc }})=$(cost_of "${OUTS[@]}")" # The baseline for breaking change #1: the same benchmark on main, where the @@ -308,5 +341,12 @@ jobs: grep -nE '^(pid|part|desc|cmd|summary|totals):' "$f" || true done mapfile -t OUTS < <(find "$PROFILE_DIR" -name '*.out*' -type f | sort) - TOTAL=$(awk '/^(summary|totals):/ {s+=$2} END {print s+0}' "${OUTS[@]}") + # `totals:` only, to match the instrumentation job -- see the comment + # on cost_of there. Summing `summary:` as well doubles the figure for + # some files and not others, which would make this comparison lie. + TOTAL=$(awk '/^totals:/ {s+=$2} END {print s+0}' "${OUTS[@]}") + for f in "${OUTS[@]}"; do + printf ' %-14s Ir=%s\n' "$(basename "$f")" \ + "$(awk '/^totals:/ {s+=$2} END {print s+0}' "$f")" + done echo "TOTAL_IR(main-preload)=$TOTAL" From ca81730499488f1819c6e3c9f4e341d80dcad085 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 7 Sep 2026 14:17:54 +0200 Subject: [PATCH 08/10] ci: sweep benchmark size to test the fixed-overhead model The single fixed-size probe reports +26.4%, which is dominated by fixed per-process startup and so says nothing useful about a real benchmark. Extrapolating ~+7% from the one process that did real work (`seq`) is a guess, not a measurement. This holds the process shape identical across sizes (exec-harness -> sh -> seq) and varies only the work, which makes the model falsifiable: if the shift really is a fixed per-process cost, `branch - main` stays roughly CONSTANT in absolute Ir as N grows while the ratio collapses towards 1. If the delta instead grows with N, the cost is proportional and the "only matters for tiny benchmarks" reading is wrong. Both variants run the same sizes through the same script, at pinned commits (github.sha rather than the branch name, which may move), so the pairs are directly comparable. Carries the same exec-harness tamper guard as the instrumentation job. Throwaway, like the rest of this workflow -- delete once the number is recorded on the ticket. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/cod-3218-exec-harness-check.yml | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/.github/workflows/cod-3218-exec-harness-check.yml b/.github/workflows/cod-3218-exec-harness-check.yml index 87b64467..4e4ba4a3 100644 --- a/.github/workflows/cod-3218-exec-harness-check.yml +++ b/.github/workflows/cod-3218-exec-harness-check.yml @@ -350,3 +350,69 @@ jobs: "$(awk '/^totals:/ {s+=$2} END {print s+0}' "$f")" done echo "TOTAL_IR(main-preload)=$TOTAL" + + # Quantifies breaking change #1 properly, which the single fixed-size probe + # above cannot: it reports one number (+26 %) that is dominated by fixed + # per-process startup and so says nothing about a real benchmark. + # + # The process shape is held IDENTICAL across sizes (exec-harness -> sh -> seq) + # and only the work varies, which makes the model falsifiable: if the shift + # really is a fixed per-process cost, then `branch - main` stays roughly + # CONSTANT in absolute Ir as N grows while the ratio collapses towards 1. If + # instead the delta grows with N, the cost is proportional and the whole + # "it only matters for tiny benchmarks" reading is wrong. + # + # Both variants run the same sizes through the same script, so the pairs are + # directly comparable. Delete this job once the number is recorded on the + # ticket. + cost-sweep: + name: cost sweep (${{ matrix.variant }}) + runs-on: ubuntu-latest # x86_64 + strategy: + fail-fast: false + matrix: + variant: [branch, main] + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + # github.sha rather than the branch name: the branch may have moved on + # by the time this runs, and the two variants must be pinned commits + # for the comparison to mean anything. + ref: ${{ matrix.variant == 'main' && 'main' || github.sha }} + submodules: true + - uses: ./.github/actions/install-rust + with: + cache-key: cod3218-sweep-${{ matrix.variant }} + + - name: Install the exec-harness under test + run: | + cargo install --path crates/exec-harness --locked + exec-harness --version + + - name: Sweep + run: | + # Same tamper guard as the instrumentation job: if the runner swapped + # in the released exec-harness, the sweep would silently measure the + # wrong binary on the branch variant. + BEFORE=$(sha256sum "$(which exec-harness)" | cut -d' ' -f1) + + for N in 1000 10000 100000 1000000; do + D=$RUNNER_TEMP/sweep-$N + mkdir -p "$D" + CODSPEED_LOG=warn cargo run -q -- exec \ + -m simulation \ + --skip-upload \ + --profile-folder "$D" \ + --name "sweep_$N" \ + -- sh -c "seq 1 $N > /dev/null" + + mapfile -t OUTS < <(find "$D" -name '*.out*' -type f | sort) + TOTAL=$(awk '/^totals:/ {s+=$2} END {print s+0}' "${OUTS[@]}") + echo "SWEEP variant=${{ matrix.variant }} N=$N files=${#OUTS[@]} TOTAL_IR=$TOTAL" + done + + AFTER=$(sha256sum "$(which exec-harness)" | cut -d' ' -f1) + if [ "$BEFORE" != "$AFTER" ]; then + echo "the runner replaced exec-harness mid-sweep; results are not trustworthy" + exit 1 + fi From 1e87124b67c2447aa717e7290abbccc680cff386 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 7 Sep 2026 16:16:31 +0200 Subject: [PATCH 09/10] revert: restore measure.rs and shared.rs to their state on main Keeps the change component-local. The goal is to unblock the musl build by removing the preload; forcing `--instr-atstart=inherit` on every simulation run was a bigger behavioural change than that needs, and it reached runs it had no business touching: Entrypoint runs (`cargo codspeed run`, pytest-codspeed) got `inherit` too. Harmless for a benchmark that never forks -- top-level `inherit` starts instrumentation off, same as `no` -- but an entrypoint benchmark that DOES fork would suddenly have its children instrumented and counted, silently changing its numbers. That is presumably why the flag was opt-in to begin with. It may also be unnecessary. Per COD-2349 the instrumentation state crosses `exec` by injecting `--instr-atstart=yes|no` into the child valgrind's argv (`VG_(needs_child_exec_args)`), not via the top-level flag; `inherit` covers the fork-only case. exec-harness spawns with `Command::status()`, i.e. fork + exec, so the child should pick the state up through the argv channel whatever the top level says. The check on this branch will confirm or refute that -- and with `src/` now identical to main, it isolates the exec-harness change on its own. If it turns out the runner does need a nudge, the shape to use is deriving it from `uses_exec_harness` (already threaded to `executor_config_for_command`) rather than hardcoding it here, so entrypoint runs keep their current behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- src/cli/shared.rs | 6 +----- src/executor/valgrind/measure.rs | 9 ++------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/cli/shared.rs b/src/cli/shared.rs index b926af04..1fe13474 100644 --- a/src/cli/shared.rs +++ b/src/cli/shared.rs @@ -135,11 +135,7 @@ pub struct ExecAndRunSharedArgs { )] pub exclude_allocations: bool, - /// Emit per-thread dumps for the benchmarked process in simulation mode. - /// - /// Subprocesses spawned by the benchmarked process are now always measured, - /// so this only controls Valgrind's `--separate-threads`. The flag keeps its - /// name for compatibility; renaming it would break existing invocations. + /// Measure the subprocesses spawned by the benchmarked process in simulation mode. #[arg(long, env = "CODSPEED_SIMULATION_TRACK_SUBPROCESS")] pub simulation_track_subprocess: bool, diff --git a/src/executor/valgrind/measure.rs b/src/executor/valgrind/measure.rs index 5c395753..62807b92 100644 --- a/src/executor/valgrind/measure.rs +++ b/src/executor/valgrind/measure.rs @@ -33,16 +33,11 @@ fn get_valgrind_args(tool: &SimulationTool, config: &ExecutorConfig) -> Vec Date: Mon, 7 Sep 2026 16:26:01 +0200 Subject: [PATCH 10/10] fix(valgrind): track subprocesses for exec-harness runs Removing the preload moved the instrumentation toggles out of the benchmark child and into exec-harness, which forks it. Measured on CI: with `--instr-atstart=no` the child dumps a single zero-cost `Trigger: Program termination` part, so the benchmark reports nothing at all -- the parent's live instrumentation state does not reach the child on its own. `--instr-atstart=inherit` is what enables that propagation; the argv-injection channel COD-2349 added for `exec` is not sufficient by itself. Derive it from `uses_exec_harness`, which the orchestrator already threads down to `executor_config_for_command`, rather than making `--instr-atstart=inherit` unconditional in `get_valgrind_args`. That leaves `measure.rs` untouched and keeps entrypoint runs on exactly their current behaviour, which matters: an entrypoint benchmark that forks would otherwise start having its children instrumented and counted, silently changing its numbers. The parameter was already `!uses_exec_harness` at the call site, for `enable_introspection`; it now passes the positive form and both derived values are computed inside. `ExecutorConfig::test()` passes `false`, which reproduces its previous field values exactly -- its target is an entrypoint one. Co-Authored-By: Claude Opus 5 (1M context) --- src/executor/config.rs | 26 ++++++++++++++++++++------ src/executor/orchestrator.rs | 2 +- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/executor/config.rs b/src/executor/config.rs index 07f99c82..6efea5a9 100644 --- a/src/executor/config.rs +++ b/src/executor/config.rs @@ -186,12 +186,26 @@ impl OrchestratorConfig { /// Produce a per-execution [`ExecutorConfig`] for the given command and mode. /// - /// `enable_introspection` controls whether language-level wrappers (Node.js, Go) - /// are injected into `PATH`. This should be `false` for exec-harness targets. + /// `uses_exec_harness` says whether this run is driven by exec-harness rather + /// than being a plain entrypoint command. Two things are derived from it: + /// + /// - Language-level wrappers (Node.js, Go) are injected into `PATH` only for + /// entrypoint runs. + /// - Subprocess tracking is forced on for exec-harness runs. exec-harness + /// toggles instrumentation in its own process and then forks the benchmark, + /// so the benchmarked child is measured only if valgrind propagates that + /// state across `fork`/`exec` — which is what `--instr-atstart=inherit` + /// enables. Measured: with `--instr-atstart=no` the child dumps a single + /// zero-cost part and the benchmark reports nothing at all. + /// + /// Deriving it here rather than making `--instr-atstart=inherit` + /// unconditional keeps entrypoint runs on their current behaviour. That + /// matters: an entrypoint benchmark that forks would otherwise start having + /// its children instrumented and counted, silently changing its numbers. pub fn executor_config_for_command( &self, command: String, - enable_introspection: bool, + uses_exec_harness: bool, ) -> ExecutorConfig { ExecutorConfig { working_directory: self.working_directory.clone(), @@ -205,11 +219,11 @@ impl OrchestratorConfig { allow_empty: self.allow_empty, go_runner_version: self.go_runner_version.clone(), extra_env: self.extra_env.clone(), - enable_introspection, + enable_introspection: !uses_exec_harness, fair_sched: self.fair_sched, cycle_estimation: self.cycle_estimation, exclude_allocations: self.exclude_allocations, - simulation_track_subprocess: self.simulation_track_subprocess, + simulation_track_subprocess: self.simulation_track_subprocess || uses_exec_harness, } } } @@ -253,7 +267,7 @@ impl OrchestratorConfig { impl ExecutorConfig { /// Constructs a new `ExecutorConfig` with default values for testing purposes pub fn test() -> Self { - OrchestratorConfig::test().executor_config_for_command("".into(), true) + OrchestratorConfig::test().executor_config_for_command("".into(), false) } } diff --git a/src/executor/orchestrator.rs b/src/executor/orchestrator.rs index ca2dbdf4..6d3646d8 100644 --- a/src/executor/orchestrator.rs +++ b/src/executor/orchestrator.rs @@ -143,7 +143,7 @@ impl Orchestrator { for (run_part_index, part) in run_parts.into_iter().enumerate() { let config = self .config - .executor_config_for_command(part.command, !part.uses_exec_harness); + .executor_config_for_command(part.command, part.uses_exec_harness); let mut executor = get_executor_from_mode(part.mode, self.config.walltime_profiler); let profile_folder = self.resolve_profile_folder(&executor.name(), run_part_index, total_parts)?;