Skip to content

[COD-2349] feat(callgrind): track subprocesses across fork and exec - #25

Open
GuillaumeLagrange wants to merge 3 commits into
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-osfrom
cod-2349-support-subprocesses-in-valgrind
Open

[COD-2349] feat(callgrind): track subprocesses across fork and exec#25
GuillaumeLagrange wants to merge 3 commits into
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-osfrom
cod-2349-support-subprocesses-in-valgrind

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown

Track subprocesses spawned by a benchmark so they can be measured and
attributed back to the benchmark that spawned them, across both fork and
exec.

Two independent pieces, one per commit:

1. Forward instrumentation state across a traced exec. A process spawned
via exec under --trace-children=yes gets a fresh valgrind whose
instrumentation state resets to --instr-atstart, so a benchmark reached
through an exec chain (e.g. cargo run) was measured from the wrong point. A
plain fork inherits the state with the address space, but an exec does not.
This adds a VG_(needs_child_exec_args) tool need: the core asks the tool for
extra valgrind arguments while building the child's argv at exec time and
appends them after VG_(args_for_valgrind) (later options win). Callgrind uses
it to forward --instr-atstart=<current state>. Wired into the exec argv
construction on Linux (generic), Darwin, and Solaris.

2. Record spawned subprocesses per dump part. When a benchmark spawns a
subprocess, the profile had no way to attribute that subprocess to the exact
benchmark (dump part) that spawned it. On fork the parent now records the new
child pid against the part currently being measured, and each part's header
lists the children spawned during it as desc: Spawned pid: lines (a desc
field so kcachegrind and callgrind_annotate ignore it). A fork child restarts
its part counter at 1 and drops the inherited edges so it only reports children
it spawns itself, and zeroes cost so work before the fork stays attributed to
the parent.

To carry the child pid to the fork hook, VG_(atfork)'s parent callback now
takes the pid of the just-created child; the pre/child callbacks are unchanged.
All fork/clone sites (Linux, FreeBSD, Solaris, generic) pass it through. The
child hook also re-stamps the in-flight fork syscall's start time from the
child's clocks, since the thread CPU clock restarts in the child and the exit
delta would otherwise underflow.

The spawn edge is recorded parent→child rather than child→parent: at fork time
the parent already knows both the child pid and its own live part, so no spawn
identity needs to cross the exec boundary — only the instrumentation state
does. This is a change from the earlier squashed design that emitted
desc: Spawned by: <pid>:<part> from the child; see the COD-2349 discussion for
the full rationale.

Stacked on top of #24 (COD-3196, per-thread dump support), which this builds
on for the retired-thread handling in the fork/exec paths.

Behavior verified with fork/exec integration tests in the valgrind-helpers repo
(subprocess measured and recorded across fork, across a traced exec, through a
deep fork tree, and one child recorded per part across a two-part run).

Closes COD-2349

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR tracks benchmark subprocesses across fork and exec. The main changes are:

  • Forwards Callgrind instrumentation state across traced execs.
  • Records spawned child PIDs against the active dump part.
  • Preserves and flushes retired-thread costs in per-thread dumps.
  • Passes child PIDs through parent fork callbacks on supported platforms.
  • Discards spawn records after their associated part is written.

Confidence Score: 5/5

This looks safe to merge.

  • The updated cleanup removes records only for the part that has finished dumping.
  • Empty-cost parts still write a section before their spawn records are discarded.
  • Output-open failures stop the path before cleanup, preserving records that were not emitted.

Important Files Changed

Filename Overview
callgrind/dump.c Writes subprocess metadata per dump part and removes emitted records to prevent unbounded retention.
callgrind/main.c Tracks forked children, resets child state, and forwards instrumentation state across execs.
callgrind/threads.c Retains exited-thread data until it can be included in per-thread dumps.
coregrind/m_libcproc.c Extends parent fork callbacks with the newly created child PID.

Reviews (5): Last reviewed commit: "feat(callgrind): inherit instrumentation..." | Re-trigger Greptile

Comment thread callgrind/dump.c
@codspeed-hq

codspeed-hq Bot commented Jul 22, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 60 skipped benchmarks1


Comparing cod-2349-support-subprocesses-in-valgrind (ce72315) with cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os (5cae7e2)

Open in CodSpeed

Footnotes

  1. 60 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from ce72315 to 69b355e Compare July 22, 2026 21:23
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from 5cae7e2 to bf37129 Compare July 22, 2026 21:23
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from 69b355e to 002990c Compare July 22, 2026 21:56
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from bf37129 to bb4fb76 Compare July 22, 2026 21:56
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from 002990c to ec038a4 Compare July 27, 2026 12:27
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from bb4fb76 to febc434 Compare July 27, 2026 12:27

@art049 art049 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed it would be simpler to have a new inherit mode that would resolve a file owned by the parent process declaring if instrumentation is enabled or not.
It will make the overall change in the logic of the codebase more isolated and easier to maintain.

GuillaumeLagrange and others added 3 commits July 27, 2026 16:16
Make per-thread dumps behave like OS threads. The valgrind core recycles
ThreadId slots and callgrind keyed all per-thread state on that slot, so
a new OS thread reusing a slot silently inherited the dead thread's
identity and costs. Mid-run client-request dumps also only flushed the
calling thread, deferring every other thread's history to the
termination dump.

- Key thread identity (BBCC lookup, "thread:" header, dump suffix) on a
  monotonic serial instead of the recycled ThreadId slot.
- Retire exiting threads via a pre_thread_ll_exit hook: unwind, snapshot
  their name, move them to a retired list so their costs stay attributed
  across the rest of the run. Gated on separate_threads.
- Flush every thread, live and retired, at each dump so per-thread
  deltas land under the part being dumped, in ascending serial order.
  Threads with a zero delta are skipped; if that skips a whole part
  (metadata dumps with instrumentation off, termination), force one
  empty section so the part and its trigger metadata survive. Key the
  combined-dump header on file state, not out_counter, so it is written
  even when leading parts are skipped.
- Zero every thread on CALLGRIND_ZERO_STATS under separate_threads.
- Emit the core's thread name as a "desc: Thread name:" line, picked up
  by the backend in COD-3197.

Refs COD-3196
Co-Authored-By: Claude <noreply@anthropic.com>
When a benchmark spawns a subprocess, the resulting profile had no way to
attribute that subprocess back to the exact benchmark (dump part) that
spawned it, so the spawn tree could not be rebuilt.

Track fork edges and emit them in the dump: on fork the parent records
the new child pid against the part currently being measured, and each
part's header lists the children spawned during it as "desc: Spawned
pid:" lines (a desc field so kcachegrind and callgrind_annotate ignore
it). A fork child restarts its part counter at 1 and drops the inherited
edges so it only reports children it spawns itself, and zeroes cost so
work before the fork stays attributed to the parent.

To carry the child pid to the fork hook, VG_(atfork)'s parent callback
now takes the pid of the just-created child; the pre/child callbacks are
unchanged. All fork/clone sites (Linux, FreeBSD, Solaris, generic) pass
it through. The child hook also re-stamps the in-flight fork syscall's
start time from the child's clocks, since the thread CPU clock restarts
in the child and the exit delta would otherwise underflow.

Closes COD-2349
Co-Authored-By: Claude <noreply@anthropic.com>
A process spawned via exec under --trace-children=yes gets a fresh
valgrind whose instrumentation state resets to --instr-atstart, so a
benchmark reached through an exec chain (e.g. `cargo run`) was measured
from the wrong point. A plain fork inherits the state with the address
space, but an exec does not.

Add --instr-atstart=inherit: like "no", except each process advertises
its current instrumentation state by keeping
<tmpdir>/callgrind-instr-<pid> in existence while enabled, and a fresh
valgrind adopts the state advertised for its own PID. An exec keeps the
PID, so the file maintained by the pre-exec image hands the state over;
a fork child inherits the state with the address space and republishes
it under its new PID. The file stores the process start time (stable
across exec), so a file left behind by a killed process is rejected
when its PID is reused.

Closes COD-2349
Generated with AI Agent (Claude Code)
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from ec038a4 to aece00f Compare July 27, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants