fix(callgrind): stop dump output depending on heap layout by lvaroqui · Pull Request #44 · CodSpeedHQ/valgrind-codspeed

lvaroqui · 2026-10-01T15:54:35Z

Fix callgrind output that depended on where the tool's nodes were allocated in memory. This made callgrind/tests/inline-crossfile fail on the ubuntu-22.04 job:

 fe=inline-crossfile.c
-cfni=???

Root cause: an inconsistent sort comparator

Before dumping, my_cmp sorts BBCCs by subtracting pointers to obj_node, file_node and fn_node (return cxt1->fn[0]->file - cxt2->fn[0]->file;). These nodes are allocated separately, so the subtraction is undefined. With node sizes that are not a power of two (file_node is 728 bytes), GCC compiles it as an exact division: a shift, then a multiply by the modular inverse. That gives arbitrary values whenever the byte distance is not a multiple of the size:

// a < b < c in memory, each 16-byte aligned, sizeof(file_node) == 728
cmp(a, b) = -1510318172
cmp(b, c) =  2076687464   // b > c ?
cmp(a, c) =   566369292   // a > c ?

The order is not even transitive, so qsort can split one function's BBCCs into several blocks, each with its own fn= header. In the failing runs, main was split right after the inlined compute_product code. The flush at the end of a block writes fe= back to the function's file without checking inline state, so the cfni=??? that closes the inlined range was never written. filter_inline only reads the first fn=main block, which is exactly the diff above.

Which functions get split depends on heap addresses, so it varied with the tool binary, the checkout path and the debug info Valgrind loads. On the ubuntu-22.04 runner, the CI's apt-get install gdb also pulls in libc6-dbg, so Valgrind reads glibc's full DWARF there, which changes the allocations. The test binary was byte-identical between passing and failing runs.

Changes

Sort by node number (68fea9cdb): compare the nodes' creation numbers, which are unique per obj/file/fn, and return -1/0/1 through a CMP3 macro instead of a difference that can overflow an int. This also covers rec_index and bb->offset.
- Every function now appears in exactly one block.
- Blocks are written in creation order instead of address order. The order changes for every profile, but the content does not. Consumers that merged repeated fn= blocks get the same totals.
Debug cache fix (cc0000d96): this is a real bug, but not the cause of the flake. On a cache miss, get_inline_info() stored the inline name in an entry still keyed by another address, so a later lookup of that address got the wrong inline function. On a miss it now queries without caching; entries are only filled by get_debug_pos(), which writes the key, file, line and inline name together. Inline function names are also compared with VG_(strcmp), since the same name is not always the same pointer.

The issue also suspected that last_inline_fn is never reset between functions. It is reset: print_bbccs_of_thread resets it whenever print_fn_pos reports a new function. So that part is left as is.

Verification

I reproduced the failure in an ubuntu:22.04 container, with libc6-dbg installed and the repo at the runner's checkout path. Each run below profiles 4 test programs (inline-crossfile, inline-samefile, simwork, threads) under 10 GLIBC_TUNABLES CPU-feature settings, and counts functions that appear in more than one fn= block:

	Split functions (40 runs)
Before	23
After	0

With the fix, the full callgrind suite passes in that container, and the ubuntu-22.04 test-callgrind job is green here.

Closes COD-3536

get_inline_info() missed the debug cache for an address whose entry belonged to another address, then stored its inline name in that entry anyway, under the other address's key. A later lookup of the other address returned the wrong inline function. The cache is shared by all functions of a dump, and they are dumped in an order derived from pointer values, so which cfni= markers a function got depended on the tool binary's layout rather than on the profiled program. On a miss, query the inline name without caching it; the entry is only filled by get_debug_pos(), which writes the whole entry. Also compare inline function names by content rather than by pointer, since the same name is not always the same pointer. Closes COD-3536 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

codspeed · 2026-10-01T16:16:11Z

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 60 skipped benchmarks¹

_{Comparing cod-3536-callgrind-inline-markers-cfni-depend-on-the-tool-binary (68fea9c) with master (4adb224)}

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. ↩

The dump comparator ordered BBCCs by subtracting pointers to separately allocated obj, file and fn nodes. That is undefined, and since the node sizes are not powers of two the compiler's exact division returns arbitrary values, so the comparison was not a consistent order. qsort could then split one function's BBCCs into several blocks, each with its own fn= header. A block boundary falling inside a function changed the markers written around it, for instance dropping the cfni= marker that closes an inlined region. Which functions got split depended on heap addresses, so it varied with the tool binary and the loaded debug info. Compare the nodes' creation numbers instead, and return -1/0/1 rather than a difference that can overflow an int. The dump order is now also independent of where the nodes are allocated. Closes COD-3536 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

not-matthias

Very nice!

lvaroqui · 2026-10-02T09:01:39Z

Ran the workflow dispatch 20 times and all passed!

greptile-apps · 2026-10-02T09:05:15Z

Confidence Score: 4/5

[Medium risk] Changes caching logic in profiler debug output.

The PR appears safe to merge, with a non-blocking gap in regression coverage for function-block contiguity.

Findings

Function grouping lacks direct coverage ▶

Fix with agent prompt

### Issue 1
callgrind/dump.c:998-1006
**Function grouping lacks direct coverage.** This comparator fixes a heap-layout-dependent failure by keeping a function’s BBCCs together, but no regression test checks that grouping directly. The existing inline test reads only the first `fn=main` block, so a future split could pass unnoticed. A test that checks block contiguity would protect the behavior this change repairs.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR replaces heap-address-based BBCC sorting with creation-number comparisons and prevents inline-name cache misses from writing results under another address’s key. It also compares inline names by content.

The comparator addresses the reported function-block splitting.
Direct regression coverage for block contiguity is still missing.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Collected BBCCs] --> B[Sort by node creation number]
  B --> C[Contiguous function blocks]
  C --> D[Write positions and inline markers]
  D --> E[Callgrind profile]

_{Reviews (1) · Last reviewed commit: "fix(callgrind): sort dumped BBCCs by nod..."}

greptile-apps · 2026-10-02T09:05:19Z

    if (cxt1->fn[0]->file->obj != cxt2->fn[0]->file->obj)
-	return cxt1->fn[0]->file->obj - cxt2->fn[0]->file->obj;
+	return CMP3(cxt1->fn[0]->file->obj->number,
+		    cxt2->fn[0]->file->obj->number);

    if (cxt1->fn[0]->file != cxt2->fn[0]->file)
-	return cxt1->fn[0]->file - cxt2->fn[0]->file;
+	return CMP3(cxt1->fn[0]->file->number, cxt2->fn[0]->file->number);

    if (cxt1->fn[0] != cxt2->fn[0])
-	return cxt1->fn[0] - cxt2->fn[0];
+	return CMP3(cxt1->fn[0]->number, cxt2->fn[0]->number);


Function grouping lacks direct coverage. This comparator fixes a heap-layout-dependent failure by keeping a function’s BBCCs together, but no regression test checks that grouping directly. The existing inline test reads only the first fn=main block, so a future split could pass unnoticed. A test that checks block contiguity would protect the behavior this change repairs.

Knowledge Base Used: Execution profiling tools

Prompt To Fix With AI

This is a comment left during a code review. Path: callgrind/dump.c Line: 998-1006 Comment: **Function grouping lacks direct coverage.** This comparator fixes a heap-layout-dependent failure by keeping a function’s BBCCs together, but no regression test checks that grouping directly. The existing inline test reads only the first `fn=main` block, so a future split could pass unnoticed. A test that checks block contiguity would protect the behavior this change repairs. **Knowledge Base Used:** [Execution profiling tools](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/valgrind-codspeed/-/docs/execution-profiling-tools.md) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

lvaroqui mentioned this pull request Oct 1, 2026

feat(callgrind): add CALLGRIND_ADD_DESC client request #43

Merged

lvaroqui added this pull request to stack #45 October 1, 2026 15:56

lvaroqui changed the title ~~fix(callgrind): stop inline markers depending on dump order~~ fix(callgrind): stop dump output depending on heap layout Oct 2, 2026

lvaroqui requested review from GuillaumeLagrange and not-matthias October 2, 2026 08:39

not-matthias approved these changes Oct 2, 2026

View reviewed changes

lvaroqui marked this pull request as ready for review October 2, 2026 09:01

lvaroqui merged commit 18b901a into master Oct 2, 2026
70 checks passed

lvaroqui deleted the cod-3536-callgrind-inline-markers-cfni-depend-on-the-tool-binary branch October 2, 2026 09:03

greptile-apps Bot reviewed Oct 2, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix(callgrind): stop dump output depending on heap layout - #44

fix(callgrind): stop dump output depending on heap layout#44
lvaroqui merged 2 commits into
masterfrom
cod-3536-callgrind-inline-markers-cfni-depend-on-the-tool-binary

lvaroqui commented Oct 1, 2026 •

edited

Loading

Uh oh!

codspeed Bot commented Oct 1, 2026 •

edited

Loading

Uh oh!

not-matthias left a comment

Uh oh!

lvaroqui commented Oct 2, 2026

Uh oh!

Uh oh!

greptile-apps Bot commented Oct 2, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lvaroqui commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause: an inconsistent sort comparator

Changes

Verification

Uh oh!

codspeed Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

not-matthias left a comment

Choose a reason for hiding this comment

Uh oh!

lvaroqui commented Oct 2, 2026

Uh oh!

Uh oh!

greptile-apps Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

greptile-apps Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lvaroqui commented Oct 1, 2026 •

edited

Loading

codspeed Bot commented Oct 1, 2026 •

edited

Loading

greptile-apps Bot commented Oct 2, 2026 •

edited

Loading