Conversation
`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 <argp.h>`, 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: libbpf/libbpf-sys#137
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ck-musl # Conflicts: # src/executor/config.rs
Merging this PR will not alter performance
|
… config The spike proved memtrack builds on musl, but the recipe lived entirely in out-of-band env vars. Split it: what is a property of the repo moves in, what is a property of the build machine stays out. The three `ac_cv_search_*` cache seeds go in unconditionally rather than per-target. `argp_parse`, `_obstack_free` and `fts_close` all live in glibc's libc, so "none required" is the answer a gnu host reaches on its own — seeding it only skips three `configure` probes there and cannot change the outcome. The aarch64 `-lgcc` becomes `[target.aarch64-unknown-linux-musl] rustflags`, which cargo scopes natively. rustc links with `-nodefaultlibs`, so gcc never pulls in libgcc, and libbpf's C code needs the outline-atomic helpers that live there. What is deliberately NOT here is the include flags. `[env]` cannot express `-I<absolute path>` — its `relative = true` form resolves a bare path, with nowhere to put the `-I` — but the real reason is that `-idirafter /usr/include/<triplet>` describes Debian's header layout, not ours. An Arch host needs different values. Checking one in would just be picking a distro. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moha-bekh
changed the title
`libc_allocator_symbols_resolve_to_offsets` read `/proc/self/maps` and required a mapped `libc.so.6`. That only ever worked because the test binary happens to be dynamically linked against glibc: under static musl there is no such mapping by construction, and the test failed for a reason that said nothing about symbol resolution — which is why the musl check workflow had to `--skip` it. Read a spawned child's maps instead. That is static-safe, and it is also what the production path does: symbols are resolved in a *traced* process, never in memtrack's own. The child is killed and reaped on the panic path too, so a failed assertion does not leak a `sleep`. Passes on both aarch64 gnu and aarch64 static musl (19/19 `--lib`). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Still throwaway, still must not reach main. Three changes. The seeds and `-lgcc` are gone from here — they now live in `.cargo/config.toml`, so the sudo `env` passthrough no longer has to smuggle them into the test run. The include flags move from `CFLAGS` to `CFLAGS_<target>`. cc-rs reads `CFLAGS_<target>`, `TARGET_CFLAGS` and `CFLAGS` and accumulates them, so a target-scoped name reaches the musl build and is invisible to the gnu one; the `unset CFLAGS` dance before the comparison build is gone. It also subsumes `LIBBPF_SYS_EXTRA_CFLAGS`: libbpf-sys forwards `compiler.cflags_env()` to elfutils' configure, to zlib's, and to libbpf's make, appending `LIBBPF_SYS_EXTRA_CFLAGS` only to the last of the three. And every job now runs on both `ubuntu-latest` and `ubuntu-24.04-arm`. `arch` has to be a real matrix dimension rather than something the `include` entries introduce: an include entry whose keys are all new merges into *every* combination, so the second would have overwritten the first and both jobs would have ended up aarch64. The unit job no longer skips `libc_allocator_symbols_resolve_to_offsets`, which no longer assumes a dynamically linked test binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preparation for bundling both into the `codspeed` binary (COD-3440 phase 1). Follows what samply already does here: `samply::run` drives the library, and the entry point is a wrapper over it. Each crate gains a `cli` module exposing `run_cli(argv)`, and its `main.rs` shrinks to the two things that only make sense when the crate owns the whole process: installing the global logger, and turning the result into an exit code. A bundled subcommand can then call `run_cli` directly and cannot drift from the standalone binary, because there is only one implementation. The logger split is the point, not an accident. Only one global logger can exist per process; when these run bundled, the host CLI has already installed its own. Keeping `env_logger::init` in `main.rs` rather than in `run_cli` is what makes the bundled path safe. memtrack's `run_cli` returns the exit code instead of calling `std::process::exit` itself, so the caller stays in charge of teardown. memtrack's module is gated on `ebpf` for the same reason its `[[bin]]` is: without that feature there is no `Tracker` to drive. No behaviour change. 25 + 19 unit tests pass, clippy and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing it COD-3440 phase 1, second half. Follows samply exactly: a hidden `InternalCommands` variant, `get_command_builder()` re-execing the current binary, and `SELF_EXE_ENV_VAR` for the cases where `current_exe` lies. One thing samply did not need: exec-harness is handed its targets through a heredoc, so its invocation is spliced into a string that `bash -c` runs rather than driven by a `CommandBuilder`. Hence `get_shell_command()`, which renders the same re-exec through `shell_words::join`. Two tests pin that down — a self-exe path containing a space has to come back out as one word, and the heredoc delimiter has to stay quoted so nothing in the JSON payload is expanded. Unquoted, the tail of the path would silently become exec-harness's first argument and the run would fail with a parse error rather than a missing-file one. `ensure_binary_installed` for exec-harness is gone, which leaves `EXEC_HARNESS_INSTALLER`, `EXEC_HARNESS_VERSION`, `EXEC_HARNESS_COMMAND` and the `PinnedBinary::ExecHarnessInstaller` variant dead. `clippy -D warnings` rejects dead code, and keeping them behind an `allow` would have advertised a download path that no longer exists, so they are removed. No automation writes `binary_pins.rs`, so nothing else has to move with them. Verified: `codspeed exec-harness --version` reports `exec-harness 1.3.0` from the bundled binary. clippy strict, fmt, and the workspace `--lib` suite (5 binaries, 0 failures) all pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
COD-3440 phase 1c. Same shape as samply and exec-harness: a hidden `InternalCommands` variant, `get_command_builder()` re-execing the current binary. The root manifest loses `default-features = false` on memtrack so the tracker itself is linked in, not just its IPC types. Verified rather than assumed. From the **statically linked musl** build: `codspeed memtrack track` loads its BPF programs and writes a 10 249-byte artifact. That closes the roadmap's open risk — memtrack's musl port was only ever validated standalone, never linked into the same binary as the runner. Size went the right way. On the dist profile for aarch64 musl, the merged binary compresses to 11 049 967 bytes against 13 522 798 for the three artifacts published today: one download instead of three, and ~2.5 MB smaller, because the code each binary carried its own copy of costs more than linking everything once. `memtrack_path()` now returns `self_exe()`, resolved by a function shared with `get_command_builder()`. That sharing is deliberate: `setcap` on a path that is not the one later exec'd succeeds and changes nothing, so resolving it twice by hand would be a silent footgun. The consequence is that the five capabilities, `CAP_SYS_ADMIN` among them, now sit on the `codspeed` executable. They are `+ep` with no inheritable set, so a spawned benchmark does not receive them and the elevation stops at the CLI process. `get_memtrack_status` stops probing a version — a bundled subcommand cannot be out of step with its host — and `install_memtrack` becomes a no-op. With the memtrack pin gone, `src/binary_installer/` had no callers left and is deleted; `download_pinned_file` stays, since valgrind and mongo-tracer use it directly. memtrack's apt build dependencies move to the root package, which is now the one being built for release. The memory tests needed `SELF_EXE_ENV_VAR`, as the valgrind and walltime ones already did: under `cargo test`, `current_exe` is the test harness, which rejects `memtrack track --output …` and leaves the executor waiting on an IPC connection that never comes. `MEMORY_INIT` needs it too — `grant_privileges()` runs outside the per-test scope and would otherwise setcap the throwaway test binary. 389 tests pass across the workspace, clippy `-D warnings` and fmt are clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A bare `cargo build --target <arch>-unknown-linux-musl` now works with no environment variables at all. The include flags used to be exported by the caller, on the grounds that `-idirafter /usr/include/<triplet>` describes Debian's header layout rather than this repo's; they are in fact harmless everywhere, because a `-idirafter` naming a directory that does not exist is ignored silently, and on a glibc host those directories are already on the search path. Checking both multiarch triplets in costs nothing and removes the last thing a release build would have had to inject. Two mechanisms make it expressible, and both cost a failed build to find. `CPATH` rather than `CFLAGS -I<path>`: `[env]`'s `relative = true` can only make a *bare* path absolute, and a `CFLAGS` value has nowhere to put the `-I`. `CPATH` takes bare directories. It resolves against the project root, the directory holding `.cargo/` and not `.cargo/` itself, which the cargo reference words ambiguously. The argp stub therefore sits on the include path for the gnu build too, so it has to defer to the real <argp.h> wherever one exists. `__has_include_next` is the obvious way to write that and it is wrong: the same config puts `-idirafter /usr/include` on the musl build, which makes glibc's argp.h reachable from a musl compilation, and the build then dies on `__THROW`. Including <limits.h> for <features.h> and branching on `__GLIBC__` tests the libc instead, which is the thing that actually matters. One caveat is documented rather than fixed: cargo's `[env]` does not override a variable already present in the environment unless the entry sets `force = true`, so a shell exporting `CFLAGS` or `CPATH` loses these values. No CI job exports either, and a caller who sets them deliberately should keep them. The check workflow drops its export steps, which is what proves the config stands on its own, and builds `--bin codspeed` rather than `-p memtrack`, since the merged binary is what the release ships. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ship inside the `codspeed` binary as hidden subcommands, so a standalone artifact for either is something nothing consumes and one more thing that can be out of step. Dropping their `[package.metadata.dist]` makes one tag produce one artifact set. Their `[[bin]]` targets stay: development and the tests still build them to exercise the standalone path. memtrack's apt build dependencies moved to the root package in the bundling commit, which is the one cargo-dist now builds. Its `features = ["libbpf-rs/static"]` is not lost either: libbpf-rs is pulled in with `vendored`, which builds libbpf from source and links it statically anyway, and the merged musl binary has no NEEDED, no RPATH and no RUNPATH. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither is resolved off PATH any more, so three steps were provisioning binaries nothing would use: `cargo install --path crates/memtrack`, the `setup --mode memory` that setcap'd it, and the macOS `cargo install --path crates/exec-harness`. The setcap step is not merely redundant but wrong now that the capabilities belong on `codspeed` itself: it would grant them to whichever `target/debug/codspeed` cargo last wrote, which is not necessarily the one the tests re-exec. The memory tests already grant them in `MEMORY_INIT`, pointed through `CODSPEED_SELF_EXE` at the binary they run. The other direction: linking memtrack in means building its vendored libbpf-sys, so the libbpf toolchain became a build dependency of anything that compiles the runner. `lint` (which runs `generate_config_schema`) and `basic-run-test` build it and had no such step; both get one, gated to Linux on `lint` since memtrack is Linux-only in the root manifest. The COD-3218 check workflow would otherwise have gone green while testing nothing. Its musl leg proved itself by installing a musl exec-harness on PATH and pinning its hash across the run, both of which hinge on a PATH lookup that no longer happens -- so the install was inert, the guard guarded nothing, and the leg ran a gnu `cargo run` runner exactly like the gnu leg. It now builds `codspeed` for the leg's libc, asserts `codspeed exec-harness --version` and the static properties, and invokes that binary directly. In the cost sweep the install and the tamper guard survive but only on the `main` variant, which is still the PATH-based world. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cargo release -p memtrack` and the post-release bump of `MEMTRACK_INSTALLER`, `EXEC_HARNESS_INSTALLER`, `MEMTRACK_VERSION` and `EXEC_HARNESS_VERSION` all describe constants that were deleted with the download machinery. Following those steps today would mean publishing an artifact nothing consumes and editing symbols that are not there. One crate is released now; the other two are linked into its binary and keep their `version` field only as what `--version` reports. `binary_pins.rs` holds the valgrind .deb and the mongo-tracer installer, and the pinned-hash section says so. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cargo test` never returned: every memory test sat there indefinitely, and two
`exec-harness` processes were left sleeping in anon_pipe_read for ten hours,
orphaned, holding
3 -> /tmp/runner.ctl.fifo (deleted)
4 -> /tmp/runner.ack.fifo (deleted)
`RUNNER_CTL_FIFO` and `RUNNER_ACK_FIFO` are one fixed pair of paths -- a
protocol constant shared with the integrations, not something a run can
relocate -- and `RunnerFifo::new` unlinks and recreates both. Two executions
overlapping pull the FIFO out from under each other: the first keeps its fds on
an inode that is now unlinked, the second's child opens the replacement by path,
and they never meet again. Nothing there times out, so `handle_fifo_messages`
loops waiting for a child that is itself blocked reading an ack, and the test
hangs instead of failing.
The per-mode semaphores could not prevent it, because the two tests that collide
hold different ones: walltime took `WALLTIME_SEMAPHORE`, memory took
`MEMORY_SEMAPHORE`, and both then called `RunnerFifo::new`. Replace the pair with
a single permit. It keeps what each was for -- perf is not thread-safe, the
memory tracker cannot overlap with itself -- and adds the exclusion that was
missing. The BPF lock is always taken after the FIFO permit; valgrind takes only
the BPF lock and walltime only the FIFO permit, so nothing wants the two in
opposite orders.
This is a test-level fix for a race that also exists outside the tests: two
`codspeed` runs on one machine deadlock each other the same way. Closing that
needs either a negotiated per-run FIFO path or a watchdog, neither of which
belongs in this commit.
Also guards a second way these tests can hang, unrelated to the above and not
reachable on a machine with passwordless sudo: `MEMORY_INIT` setcaps the binary,
and on a machine that prompts, sudo blocks on a password nobody can see, after
every rebuild -- file capabilities are an xattr on the inode and cargo writes a
new `codspeed` each relink. Assert up front that the grant needs no prompt and
print the exact `setcap` line if it does. `memtrack_setcap_spec` becomes
`pub(crate)` so that line cannot drift from the real one.
Refs COD-3440
Refs COD-3560
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`create_git_repo_with_remote` ran five `git` commands with `.output().unwrap()`, which only unwraps the spawn: a git that runs and exits non-zero was ignored silently. A failing `git commit` then surfaced several frames later as an `UnbornBranch` error on `refs/heads/main`, which says nothing about the cause. Route all five through one helper that asserts on the exit status and prints the command, stdout and stderr on failure. No behaviour change when the commands succeed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both said in their own header that they must not reach main. They have done their job: the COD-3218 check confirmed the preload removal on real x86_64 CI for gnu and static musl, and the COD-3440 check confirmed the musl build and the eBPF load on both architectures, last on the merged binary. Nothing they cover is unique to them any more -- `ci.yml` builds and tests the bundled binary on every PR. Refs COD-3218 Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both explain the diff rather than the code: a paragraph on what the `LD_PRELOAD` shared library used to do, and a paragraph weighing an alternative that was not taken. What a reader of these two functions needs stays -- that nothing is injected into the benchmarked executable, and the measured consequence of `--instr-atstart=no`. Kept out of the commits that introduced them because those sit below two merges of main, and folding would have flattened them. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moha-bekh
force-pushed
the
spike/cod-3440-memtrack-musl
branch
from
September 17, 2026 14:33
ad8eca1 to
2d8d983
Compare
moha-bekh
marked this pull request as ready for review
September 17, 2026 14:33
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
`self_exe()` resolves the binary that internal subcommands are re-invoked through, and the memory executor hands that same path to `sudo setcap <caps>+ep` so the capabilities land on the binary that is actually exec'd. Reading an environment variable there means anyone able to set one variable chooses which file receives CAP_SYS_ADMIN and CAP_BPF. The override exists for the tests, where `current_exe()` is the test harness and cannot dispatch a subcommand. Nothing in production sets it -- the doc comment justified it with a launcher scenario that has no caller. Putting it behind `cfg(test)`, constant included, removes the escalation path outright while keeping the tests working; a release build now always resolves `current_exe()`. Reported by Greptile on #531. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`codspeed memtrack` and `codspeed exec-harness` are a re-exec of this binary and share nothing with the runner, but they were dispatched at the bottom of `run()` -- after the profile config is loaded, after the API client is built, and after `DiscoveredProjectConfig::discover_and_load` walks the filesystem. That last one is the problem: the re-exec runs in the benchmark's working directory, which is the user's project. A malformed `codspeed.yaml` there aborts the subcommand, so a measurement fails for a reason that has nothing to do with the measurement, and a `--config` given to the outer run is not forwarded to the inner one to override it. Move them into `run_internal`, called right after `Cli::parse()`. The logger match loses its internal arms for the same reason it had them. Reported by Greptile on #531. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The released Linux artifacts are `aarch64-unknown-linux-musl` and `x86_64-unknown-linux-musl`, and nothing in CI built either: a break in the argp stub, in the kernel-header paths or in the aarch64 `-lgcc` link flag would have surfaced for the first time during a tag-triggered release. The throwaway spike workflow used to cover this and was deleted with the spike. Both legs build on a native runner, with no environment variables, which is also what keeps `.cargo/config.toml` honest -- it has to carry the whole recipe on its own. The assertions are `readelf`-based rather than a `file` string, since rustc emits a static-PIE for x86_64 musl and spells it differently from aarch64, and `codspeed exec-harness --version` / `codspeed memtrack --version` answer only if both CLIs really are linked in. Reported by Greptile on #531. Refs COD-3440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ck-musl One conflict, in `src/binary_pins.rs`, resolved in favour of this branch. main's `chore: pin memtrack 1.5.1 (#541)` bumps the `MEMTRACK_INSTALLER` version and hash; this branch deletes that pin, along with `EXEC_HARNESS_INSTALLER` and the whole download machinery, because memtrack and exec-harness are linked into the `codspeed` binary and reached as hidden subcommands. There is nothing left to download, so there is no version to bump. Kept the deletion. Nothing else refers to `MEMTRACK_INSTALLER`, `EXEC_HARNESS_INSTALLER`, `MEMTRACK_VERSION` or `EXEC_HARNESS_VERSION` after the merge. `binary_pins.rs` keeps the valgrind .deb and the mongo-tracer installer, both still downloaded. main also brings the valgrind build-from-source work and the 5.3.1 / memtrack 1.5.1 releases, which merged cleanly. `cargo fmt --check` and `cargo clippy --workspace --all-targets -- -D warnings` are clean on the result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
exec-harnessandmemtrackare compiled intocodspeedand reached as hidden subcommands, andexec-harnessno longer injectslibcodspeed_preload.so. The two are one change: a staticallylinked musl binary cannot be preloaded into a glibc process, so the single binary was blocked on
removing the preload.
One release artifact instead of three, 11.0 MB compressed against 13.5 MB today. The download
machinery and both installer pins are gone.
Three things worth a reviewer's attention:
fork, so it also covers fork/exec/wait and the child's pre-
mainstartup. It is a constant~940k Ir per exec-harness benchmark, not a percentage — stable to 0.22% across a 1000× range of
benchmark size. Shipping as is: no forced baseline, no history surgery.
benchmark URI, since
LD_PRELOADis what used to be inherited by every descendant.setcapnow lands oncodspeeditself, so memtrack's five capabilities sit on the CLI.They are
+epwith no inheritable set, so a spawned benchmark does not receive them.Also removes the user-facing "CPU Simulation mode does not support statically linked binaries"
error: nothing is injected into the benchmarked executable any more.
Closes COD-3218
Closes COD-3440