Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lvaroqui merged 2 commits into from
Oct 2, 2026
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc0000d
fix(callgrind): stop inline markers depending on dump order
lvaroqui Oct 1, 2026
68fea9c
fix(callgrind): sort dumped BBCCs by node number, not pointer difference
lvaroqui Oct 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 29 additions & 21 deletions callgrind/dump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,17 +425,21 @@ Bool get_debug_pos(BBCC* bbcc, Addr addr, AddrPos* p)
return found_file_line;
}

/* Get inline function name for an address, with caching.
/* Get inline function name for an address.
* Returns True if address is in an inlined function, False otherwise.
* If True, *inl_fn will be set to the inline function name.
*
* Served from the debug cache when get_debug_pos() filled the entry for this
* address. Otherwise the cache is left alone: the entry belongs to another
* address, and storing only the inline name there would hand it out for that
* other address.
*/
static Bool get_inline_info(Addr addr, const HChar** inl_fn)
{
int cachepos = addr % DEBUG_CACHE_SIZE;
Bool has_inline;

/* Check cache first - but only if inline info was already queried for this address */
if (debug_cache_addr[cachepos] == addr && debug_cache_inlfn[cachepos] != 0) {
/* We have cached inline info for this address */
if (debug_cache_inlfn[cachepos] == (const HChar*)(-1)) {
/* Special marker: no inline function at this address */
*inl_fn = 0;
Expand All @@ -446,16 +450,9 @@ static Bool get_inline_info(Addr addr, const HChar** inl_fn)
}

DiEpoch ep = VG_(current_DiEpoch)();
Bool has_inline = VG_(get_inline_fnname)(ep, addr, inl_fn);

if (has_inline) {
/* Cache the inline function name */
debug_cache_inlfn[cachepos] = *inl_fn;
} else {
has_inline = VG_(get_inline_fnname)(ep, addr, inl_fn);
if (!has_inline)
*inl_fn = 0;
/* Use special marker -1 to indicate "no inline function" */
debug_cache_inlfn[cachepos] = (const HChar*)(-1);
}

CLG_DEBUG(3, " get_inline_info(%#lx): %s\n",
addr, has_inline ? *inl_fn : "(not inlined)");
Expand Down Expand Up @@ -520,8 +517,10 @@ static void fprint_apos(VgFile *fp, AddrPos* curr, AddrPos* last,
const HChar* inline_fn = 0;
Bool is_inline = get_inline_info(curr_addr, &inline_fn);

/* Output cfni= if inline function changed */
if (is_inline && inline_fn && inline_fn != last_inline_fn) {
/* Output cfni= if inline function changed. Names are compared by
* content: the same name is not always the same pointer. */
if (is_inline && inline_fn &&
(!last_inline_fn || VG_(strcmp)(inline_fn, last_inline_fn) != 0)) {
VG_(fprintf)(fp, "cfni=%s\n", inline_fn);
last_inline_fn = inline_fn;
}
Expand Down Expand Up @@ -971,11 +970,19 @@ static Bool fprint_bbcc(VgFile *fp, BBCC* bbcc, AddrPos* last)
return something_written;
}

/* Three-way comparison, as -1, 0 or 1: a difference can overflow an int. */
#define CMP3(a, b) (((a) > (b)) - ((a) < (b)))

/* order by
* recursion,
* from->bb->obj, from->bb->fn
* obj, fn[0]->file, fn
* address
*
* Nodes are ordered by their creation number, not their address: subtracting
* pointers to separately allocated nodes is undefined, and with node sizes
* that are not a power of two the result is not even a consistent order, so
* the sort could split the BBCCs of one function into several blocks.
*/
static int my_cmp(BBCC** pbbcc1, BBCC** pbbcc2)
{
Expand All @@ -989,30 +996,31 @@ static int my_cmp(BBCC** pbbcc1, BBCC** pbbcc2)
int off = 1;

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);
Comment on lines 998 to +1006

@greptile-apps greptile-apps Bot Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 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!

Fix in Claude Code Fix in Codex


if (bbcc1->rec_index != bbcc2->rec_index)
return bbcc1->rec_index - bbcc2->rec_index;
return CMP3(bbcc1->rec_index, bbcc2->rec_index);

while((off < cxt1->size) && (off < cxt2->size)) {
fn_node* ffn1 = cxt1->fn[off];
fn_node* ffn2 = cxt2->fn[off];
if (ffn1->file->obj != ffn2->file->obj)
return ffn1->file->obj - ffn2->file->obj;
return CMP3(ffn1->file->obj->number, ffn2->file->obj->number);
if (ffn1 != ffn2)
return ffn1 - ffn2;
return CMP3(ffn1->number, ffn2->number);
off++;
}
if (cxt1->size > cxt2->size) return 1;
else if (cxt1->size < cxt2->size) return -1;

return bbcc1->bb->offset - bbcc2->bb->offset;
return CMP3(bbcc1->bb->offset, bbcc2->bb->offset);
#endif
}

Expand Down
Loading