From 1b3bd12b019237bd455153093f446a5e48e49c78 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Fri, 6 Nov 2026 18:53:04 +0100 Subject: [PATCH 1/3] fix(callgrind): end a call that returns with SP below its frame Callgrind only treated a ret as a return when SP moved back above the frame of the top call, or stayed equal and hit the recorded return address. A callee that returns with SP lower than it was called with, because it builds a stack frame for its caller, was turned into a 'RET w/o CALL' jump and then emulated as a call back into its caller. Everything the caller did afterwards was recorded under the callee. V8's Sparkplug prologue (Builtins_BaselineOutOfLinePrologue) works this way. With the node binary instrumented, every JS function's callees ended up under the shared prologue, which tied all JS functions into one cycle and showed non-recursive functions as recursive. A ret to the return address recorded for the top call now pops that call even when SP is still below its frame. Instruction totals are unchanged. Tests: ret_below_frame reproduces the pattern with a hand-written amd64 prologue, ret_below_frame_node checks that the Sparkplug prologue never calls into JIT code under node --always-sparkplug. --- .gitignore | 1 + CODSPEED-CHANGELOG.md | 13 ++++ callgrind/bbcc.c | 21 ++++-- callgrind/tests/Makefile.am | 9 ++- callgrind/tests/print_calls | 5 ++ callgrind/tests/ret_below_frame.c | 70 +++++++++++++++++++ callgrind/tests/ret_below_frame.post.exp | 6 ++ callgrind/tests/ret_below_frame.stderr.exp | 6 ++ callgrind/tests/ret_below_frame.stdout.exp | 1 + callgrind/tests/ret_below_frame.vgtest | 5 ++ callgrind/tests/ret_below_frame_node.js | 23 ++++++ callgrind/tests/ret_below_frame_node.post.exp | 2 + .../tests/ret_below_frame_node.stderr.exp | 6 ++ .../tests/ret_below_frame_node.stdout.exp | 1 + callgrind/tests/ret_below_frame_node.vgtest | 6 ++ 15 files changed, 169 insertions(+), 6 deletions(-) create mode 100755 callgrind/tests/print_calls create mode 100644 callgrind/tests/ret_below_frame.c create mode 100644 callgrind/tests/ret_below_frame.post.exp create mode 100644 callgrind/tests/ret_below_frame.stderr.exp create mode 100644 callgrind/tests/ret_below_frame.stdout.exp create mode 100644 callgrind/tests/ret_below_frame.vgtest create mode 100644 callgrind/tests/ret_below_frame_node.js create mode 100644 callgrind/tests/ret_below_frame_node.post.exp create mode 100644 callgrind/tests/ret_below_frame_node.stderr.exp create mode 100644 callgrind/tests/ret_below_frame_node.stdout.exp create mode 100644 callgrind/tests/ret_below_frame_node.vgtest diff --git a/.gitignore b/.gitignore index 97a35bea0b..7623c1289f 100644 --- a/.gitignore +++ b/.gitignore @@ -169,6 +169,7 @@ /callgrind/tests/runtime_obj_skip_underflow /callgrind/tests/prctl_get_auxv /callgrind/tests/deterministic_rng +/callgrind/tests/ret_below_frame /callgrind/tests/*.so /callgrind/tests/out_dbg /callgrind/tests/test-suite-overall.log diff --git a/CODSPEED-CHANGELOG.md b/CODSPEED-CHANGELOG.md index e5e366b444..6ed973281d 100644 --- a/CODSPEED-CHANGELOG.md +++ b/CODSPEED-CHANGELOG.md @@ -4,6 +4,19 @@ This file documents changes made to Valgrind for CodSpeed integration, beyond th ## Fixes +### Callgrind: end a call when it returns with SP below its frame + +Callgrind treated a `ret` as a return only when SP moved back above the +frame of the call. A function that returns with SP lower than it was called +with, because it builds a stack frame for its caller, was therefore recorded +as calling back into its caller, and everything the caller did afterwards +appeared under it. V8's Sparkplug prologue (`Builtins_BaselineOutOfLinePrologue`) +works this way, so with the node binary instrumented, every JS function's +callees were attributed to the shared prologue. + +A `ret` to the return address recorded for the innermost call now ends that +call even when SP is still below its frame. + ### Linux: glibc reports the main thread stack size correctly again `pthread_getattr_np` reports the main thread's stack size as `RLIMIT_STACK` diff --git a/callgrind/bbcc.c b/callgrind/bbcc.c index 910d494194..fe83a90df2 100644 --- a/callgrind/bbcc.c +++ b/callgrind/bbcc.c @@ -571,6 +571,7 @@ void CLG_(setup_bbcc)(BB* bb) Bool isConditionalJump; Int passed = 0, csp; Bool ret_without_call = False; + Bool ret_below_frame = False; Int popcount_on_return = 1; CLG_DEBUG(3,"+ setup_bbcc(BB %#lx)\n", bb_addr(bb)); @@ -664,11 +665,19 @@ void CLG_(setup_bbcc)(BB* bb) * - SP has the same value as when reaching the current function * and the address of this BB is the return address of last call * (we even allow to leave multiple frames if the SP stays the - * same and we find a matching return address) - * The latter condition is needed because on PPC, SP can stay - * the same over CALL=b(c)l / RET=b(c)lr boundaries + * same and we find a matching return address), or + * - SP is still below the current stack frame and the address of + * this BB is the return address of last call + * The second condition is needed because on PPC, SP can stay + * the same over CALL=b(c)l / RET=b(c)lr boundaries. + * The third one is needed for callees that leave data on the stack + * for their caller, e.g. V8's Sparkplug prologue, which builds its + * caller's frame and returns on top of it. */ - if (sp < top_ce->sp) popcount_on_return = 0; + if (sp < top_ce->sp) { + if (top_ce->ret_addr == bb_addr(bb)) ret_below_frame = True; + else popcount_on_return = 0; + } else if (top_ce->sp == sp) { while(1) { if (top_ce->ret_addr == bb_addr(bb)) break; @@ -788,6 +797,10 @@ void CLG_(setup_bbcc)(BB* bb) handleUnderflow(bb); CLG_(pop_call_stack)(); } + else if (ret_below_frame) { + /* SP-based unwinding never pops a frame SP is still below. */ + CLG_(pop_call_stack)(); + } else { CLG_ASSERT(popcount_on_return >0); CLG_(unwind_call_stack)(sp, popcount_on_return); diff --git a/callgrind/tests/Makefile.am b/callgrind/tests/Makefile.am index 0b7f9965db..de3beb896a 100644 --- a/callgrind/tests/Makefile.am +++ b/callgrind/tests/Makefile.am @@ -4,7 +4,7 @@ include $(top_srcdir)/Makefile.tool-tests.am SUBDIRS = . DIST_SUBDIRS = . -dist_noinst_SCRIPTS = filter_stderr filter_inline make_capped_true +dist_noinst_SCRIPTS = filter_stderr filter_inline make_capped_true print_calls EXTRA_DIST = \ ann1.post.exp ann1.stderr.exp ann1.vgtest \ @@ -28,6 +28,11 @@ EXTRA_DIST = \ runtime_obj_skip_c.c runtime_obj_skip_c_lib.c \ runtime_obj_skip_underflow.vgtest runtime_obj_skip_underflow.stderr.exp runtime_obj_skip_underflow.post.exp \ runtime_obj_skip_underflow.c runtime_obj_skip_underflow_lib.c \ + ret_below_frame.vgtest ret_below_frame.stderr.exp ret_below_frame.stdout.exp \ + ret_below_frame.post.exp \ + ret_below_frame_node.vgtest ret_below_frame_node.stderr.exp \ + ret_below_frame_node.stdout.exp ret_below_frame_node.post.exp \ + ret_below_frame_node.js \ bug497723.stderr.exp bug497723.post.exp bug497723.vgtest \ simwork1.vgtest simwork1.stdout.exp simwork1.stderr.exp \ simwork2.vgtest simwork2.stdout.exp simwork2.stderr.exp \ @@ -46,7 +51,7 @@ EXTRA_DIST = \ inline-crossfile.vgtest inline-crossfile.stderr.exp inline-crossfile.stdout.exp inline-crossfile.post.exp \ inline-crossfile-helper1.h inline-crossfile-helper2.h filter_inline -check_PROGRAMS = clreq register_desc register_desc_threads find_debuginfo simwork threads inline-samefile inline-crossfile runtime_obj_skip_c runtime_obj_skip_underflow deterministic_rng +check_PROGRAMS = clreq register_desc register_desc_threads find_debuginfo simwork threads inline-samefile inline-crossfile runtime_obj_skip_c runtime_obj_skip_underflow deterministic_rng ret_below_frame if HAVE_PR_GET_AUXV check_PROGRAMS += prctl_get_auxv endif diff --git a/callgrind/tests/print_calls b/callgrind/tests/print_calls new file mode 100755 index 0000000000..a6d39d8fac --- /dev/null +++ b/callgrind/tests/print_calls @@ -0,0 +1,5 @@ +#! /bin/sh + +# Print each "caller -> callee" pair of a callgrind output file written with +# --compress-strings=no, once, sorted. +awk '/^fn=/ { fn = substr($0, 4) } /^cfn=/ { print fn " -> " substr($0, 5) }' "$1" | sort -u diff --git a/callgrind/tests/ret_below_frame.c b/callgrind/tests/ret_below_frame.c new file mode 100644 index 0000000000..3ec58064a3 --- /dev/null +++ b/callgrind/tests/ret_below_frame.c @@ -0,0 +1,70 @@ +/* A helper that builds its caller's stack frame and returns with SP below the + * SP it was called with, like V8's Sparkplug prologue + * (Builtins_BaselineOutOfLinePrologue). The return still ends the helper's + * frame: the callees of with_prologue_* must stay under with_prologue_*. */ + +#include + +__attribute__((noinline)) int leaf_a(int x) +{ + __asm__ volatile("" : "+r"(x)); + return x + 1; +} + +__attribute__((noinline)) int leaf_b(int x) +{ + __asm__ volatile("" : "+r"(x)); + return x + 2; +} + +#if defined(__x86_64__) +int with_prologue_a(int x); +int with_prologue_b(int x); + +/* frame_prologue pops its return address, pushes the caller's frame (saved + * rbp and two slots), pushes the return address back and returns. The caller + * runs its body on that frame and tears it down itself. */ +__asm__( + ".text\n" + ".type frame_prologue, @function\n" + "frame_prologue:\n" + " pop %r11\n" + " push %rbp\n" + " mov %rsp, %rbp\n" + " push $1\n" + " push $2\n" + " push %r11\n" + " ret\n" + ".size frame_prologue, .-frame_prologue\n" + + ".globl with_prologue_a\n" + ".type with_prologue_a, @function\n" + "with_prologue_a:\n" + " call frame_prologue\n" + " call leaf_a\n" + " mov %rbp, %rsp\n" + " pop %rbp\n" + " ret\n" + ".size with_prologue_a, .-with_prologue_a\n" + + ".globl with_prologue_b\n" + ".type with_prologue_b, @function\n" + "with_prologue_b:\n" + " call frame_prologue\n" + " call leaf_b\n" + " mov %rbp, %rsp\n" + " pop %rbp\n" + " ret\n" + ".size with_prologue_b, .-with_prologue_b\n"); + +int main(void) +{ + printf("%d\n", with_prologue_a(1) + with_prologue_b(2)); + return 0; +} +#else +int main(void) +{ + return 0; +} +#endif diff --git a/callgrind/tests/ret_below_frame.post.exp b/callgrind/tests/ret_below_frame.post.exp new file mode 100644 index 0000000000..0df3efe3d5 --- /dev/null +++ b/callgrind/tests/ret_below_frame.post.exp @@ -0,0 +1,6 @@ +main -> with_prologue_a +main -> with_prologue_b +with_prologue_a -> frame_prologue +with_prologue_a -> leaf_a +with_prologue_b -> frame_prologue +with_prologue_b -> leaf_b diff --git a/callgrind/tests/ret_below_frame.stderr.exp b/callgrind/tests/ret_below_frame.stderr.exp new file mode 100644 index 0000000000..d0b7820ae3 --- /dev/null +++ b/callgrind/tests/ret_below_frame.stderr.exp @@ -0,0 +1,6 @@ + + +Events : Ir +Collected : + +I refs: diff --git a/callgrind/tests/ret_below_frame.stdout.exp b/callgrind/tests/ret_below_frame.stdout.exp new file mode 100644 index 0000000000..1e8b314962 --- /dev/null +++ b/callgrind/tests/ret_below_frame.stdout.exp @@ -0,0 +1 @@ +6 diff --git a/callgrind/tests/ret_below_frame.vgtest b/callgrind/tests/ret_below_frame.vgtest new file mode 100644 index 0000000000..039e9bff20 --- /dev/null +++ b/callgrind/tests/ret_below_frame.vgtest @@ -0,0 +1,5 @@ +prereq: ../../tests/arch_test amd64 && test -f ret_below_frame +prog: ret_below_frame +vgopts: --compress-strings=no --callgrind-out-file=callgrind.out.ret_below_frame +post: ./print_calls callgrind.out.ret_below_frame | grep -E 'with_prologue_|frame_prologue|leaf_' +cleanup: rm -f callgrind.out.ret_below_frame diff --git a/callgrind/tests/ret_below_frame_node.js b/callgrind/tests/ret_below_frame_node.js new file mode 100644 index 0000000000..2cfd70849a --- /dev/null +++ b/callgrind/tests/ret_below_frame_node.js @@ -0,0 +1,23 @@ +// Run under --always-sparkplug: every JS function starts by calling +// Builtins_BaselineOutOfLinePrologue, which returns with SP below the SP it +// was called with. + +function leaf(x) { + return (x * 31 + 7) % 1000003; +} + +function mid(x) { + let sum = 0; + for (let i = 0; i < 5; i++) sum += leaf(x + i); + return sum; +} + +function outer(n) { + let sum = 0; + for (let i = 0; i < n; i++) sum += mid(i); + return sum; +} + +let total = 0; +for (let k = 0; k < 200; k++) total += outer(20); +console.log(total); diff --git a/callgrind/tests/ret_below_frame_node.post.exp b/callgrind/tests/ret_below_frame_node.post.exp new file mode 100644 index 0000000000..6ace5705ef --- /dev/null +++ b/callgrind/tests/ret_below_frame_node.post.exp @@ -0,0 +1,2 @@ +prologue called +prologue calls into JIT code: 0 diff --git a/callgrind/tests/ret_below_frame_node.stderr.exp b/callgrind/tests/ret_below_frame_node.stderr.exp new file mode 100644 index 0000000000..d0b7820ae3 --- /dev/null +++ b/callgrind/tests/ret_below_frame_node.stderr.exp @@ -0,0 +1,6 @@ + + +Events : Ir +Collected : + +I refs: diff --git a/callgrind/tests/ret_below_frame_node.stdout.exp b/callgrind/tests/ret_below_frame_node.stdout.exp new file mode 100644 index 0000000000..17d6e3f6b0 --- /dev/null +++ b/callgrind/tests/ret_below_frame_node.stdout.exp @@ -0,0 +1 @@ +7270000 diff --git a/callgrind/tests/ret_below_frame_node.vgtest b/callgrind/tests/ret_below_frame_node.vgtest new file mode 100644 index 0000000000..0504df1489 --- /dev/null +++ b/callgrind/tests/ret_below_frame_node.vgtest @@ -0,0 +1,6 @@ +prereq: ../../tests/arch_test amd64 && command -v node >/dev/null +prog-asis: node +args: --always-sparkplug --no-opt --no-maglev ret_below_frame_node.js +vgopts: --compress-strings=no --callgrind-out-file=callgrind.out.ret_below_frame_node +post: sh -c 'calls=$(./print_calls callgrind.out.ret_below_frame_node); echo "$calls" | grep -q -E " -> Builtins_BaselineOutOfLinePrologue($|'"'"')" && echo "prologue called"; echo "prologue calls into JIT code: $(echo "$calls" | grep -c -E "^Builtins_BaselineOutOfLinePrologue[^ ]* -> 0x")"' +cleanup: rm -f callgrind.out.ret_below_frame_node From 3e646c902d152724e32d1e988d39a5d381950346 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Mon, 9 Nov 2026 14:47:45 +0100 Subject: [PATCH 2/3] feat(callgrind): add --separate-callers=all --separate-callers=all names every function context by its whole call stack. With the node binary instrumented, V8 builtins and trampolines sit between most pairs of JS functions; as single nodes they link every JS caller to every JS callee and form false cycles. Whole-stack contexts keep unrelated call chains apart. Full-stack contexts make the --pop-on-jump bookkeeping visible, so fix two bugs there: - After a pop on jump, the delayed push reused the SP of the frame below the replaced call. Keep the replaced call's SP, so tail-jump chains (V8 bytecode handlers, OpenSSL doall callbacks) no longer nest. - A jump from a stack entry without a call (skipped or seeded frames) hit an assert. It now nests like a normal call. Without --pop-on-jump, node -e 0 reaches depth 367 and a 704 MB dump; with it, depth 90 and 309 MB. The docs recommend combining both. Tests: separate_callers_all_node and pop_on_jump_node, which checks OPENSSL_LH_doall appears once per context. --- CODSPEED-CHANGELOG.md | 23 +++++++++++++++ callgrind/bbcc.c | 27 +++++++++--------- callgrind/clo.c | 5 +++- callgrind/context.c | 28 +++++++++++++++---- callgrind/docs/cl-manual.xml | 6 ++-- callgrind/global.h | 1 + callgrind/tests/Makefile.am | 5 ++++ callgrind/tests/pop_on_jump_node.post.exp | 1 + callgrind/tests/pop_on_jump_node.stderr.exp | 6 ++++ callgrind/tests/pop_on_jump_node.stdout.exp | 0 callgrind/tests/pop_on_jump_node.vgtest | 6 ++++ callgrind/tests/separate_callers_all_node.js | 27 ++++++++++++++++++ .../tests/separate_callers_all_node.post | 26 +++++++++++++++++ .../tests/separate_callers_all_node.post.exp | 1 + .../separate_callers_all_node.stderr.exp | 6 ++++ .../separate_callers_all_node.stdout.exp | 1 + .../tests/separate_callers_all_node.vgtest | 6 ++++ 17 files changed, 152 insertions(+), 23 deletions(-) create mode 100644 callgrind/tests/pop_on_jump_node.post.exp create mode 100644 callgrind/tests/pop_on_jump_node.stderr.exp create mode 100644 callgrind/tests/pop_on_jump_node.stdout.exp create mode 100644 callgrind/tests/pop_on_jump_node.vgtest create mode 100644 callgrind/tests/separate_callers_all_node.js create mode 100755 callgrind/tests/separate_callers_all_node.post create mode 100644 callgrind/tests/separate_callers_all_node.post.exp create mode 100644 callgrind/tests/separate_callers_all_node.stderr.exp create mode 100644 callgrind/tests/separate_callers_all_node.stdout.exp create mode 100644 callgrind/tests/separate_callers_all_node.vgtest diff --git a/CODSPEED-CHANGELOG.md b/CODSPEED-CHANGELOG.md index 6ed973281d..24a4b29a2e 100644 --- a/CODSPEED-CHANGELOG.md +++ b/CODSPEED-CHANGELOG.md @@ -17,6 +17,14 @@ callees were attributed to the shared prologue. A `ret` to the return address recorded for the innermost call now ends that call even when SP is still below its frame. +### Callgrind: `--pop-on-jump` keeps the replaced call's stack frame + +With `--pop-on-jump`, a jump to another function replaces the current call +with a call to the target. The new call took the stack pointer of the frame +below instead of the replaced one, so its return was often not recognized, +and the caller was recorded as called again from the jump target. In node, +OpenSSL's `OPENSSL_LH_doall` showed up 179 times nested in one call chain. + ### Linux: glibc reports the main thread stack size correctly again `pthread_getattr_np` reports the main thread's stack size as `RLIMIT_STACK` @@ -50,6 +58,21 @@ matching Linux on AArch64. ## Features +### Callgrind: `--separate-callers=all` + +Added `all` as a value for `--separate-callers`. Every function is then +separated by its whole call chain, which is appended to the name as with +`--separate-callers=`, e.g. `Builtins_CallFunction'Builtins_Call_Baseline'0x...'main`. + +With the node binary instrumented, V8 builtins and trampolines sit between +most pairs of JS functions. As single nodes, they connect every JS caller to +every JS callee, which merges unrelated call chains and forms false cycles. +With whole call chains as contexts, no edge joins two chains that did not run. +Combine it with `--pop-on-jump`: by default a tail jump to another function +is recorded as a nested call, so a chain of tail jumps (such as V8's bytecode +handlers dispatching to each other) keeps growing until the real frame +returns, and every step becomes a new, deeper context. + ### Fixed Linux startup stack layout Reduce simulated cache-miss variation from argument and environment sizes diff --git a/callgrind/bbcc.c b/callgrind/bbcc.c index fe83a90df2..a794366a2d 100644 --- a/callgrind/bbcc.c +++ b/callgrind/bbcc.c @@ -572,6 +572,7 @@ void CLG_(setup_bbcc)(BB* bb) Int passed = 0, csp; Bool ret_without_call = False; Bool ret_below_frame = False; + Bool popped_on_jump = False; Int popcount_on_return = 1; CLG_DEBUG(3,"+ setup_bbcc(BB %#lx)\n", bb_addr(bb)); @@ -726,23 +727,21 @@ void CLG_(setup_bbcc)(BB* bb) bb->fn->name, bb->obj->name, ret_without_call?" (RET w/o CALL)":""); - if (CLG_(get_fn_node)(last_bb)->pop_on_jump && (csp>0)) { + /* Entries without a jcc (skipped frames, frames seeded from the native + * stack at START) have no call to replace, so the jump nests instead. */ + if (CLG_(get_fn_node)(last_bb)->pop_on_jump && (csp>0) && + CLG_(current_call_stack).entry[csp-1].jcc) { call_entry* top_ce = &(CLG_(current_call_stack).entry[csp-1]); - - if (top_ce->jcc) { - CLG_DEBUG(1," Pop on Jump!\n"); + CLG_DEBUG(1," Pop on Jump!\n"); - /* change source for delayed push */ - CLG_(current_state).bbcc = top_ce->jcc->from; - sp = top_ce->sp; - passed = top_ce->jcc->jmp; - CLG_(pop_call_stack)(); - } - else { - CLG_ASSERT(CLG_(current_state).nonskipped != 0); - } + /* change source for delayed push */ + CLG_(current_state).bbcc = top_ce->jcc->from; + sp = top_ce->sp; + passed = top_ce->jcc->jmp; + CLG_(pop_call_stack)(); + popped_on_jump = True; } jmpkind = jk_Call; @@ -817,7 +816,7 @@ void CLG_(setup_bbcc)(BB* bb) delayed_push = True; csp = CLG_(current_call_stack).sp; - if (call_emulation && csp>0) + if (call_emulation && csp>0 && !popped_on_jump) sp = CLG_(current_call_stack).entry[csp-1].sp; } diff --git a/callgrind/clo.c b/callgrind/clo.c index 911051a27d..b06337f7ec 100644 --- a/callgrind/clo.c +++ b/callgrind/clo.c @@ -497,6 +497,8 @@ Bool CLG_(process_cmd_line_option)(const HChar* arg) else if VG_XACT_CLO(arg, "--separate-callers=auto", CLG_(clo).separate_callers, CONFIG_AUTO) {} + else if VG_XACT_CLO(arg, "--separate-callers=all", + CLG_(clo).separate_callers_all, True) {} else if VG_INT_CLO( arg, "--separate-callers", CLG_(clo).separate_callers) {} @@ -628,7 +630,7 @@ void CLG_(print_usage)(void) "\n cost entity separation options:\n" " --separate-threads=no|yes Separate data per thread [no]\n" -" --separate-callers= Separate functions by call chain length [0]\n" +" --separate-callers=|all Separate functions by call chain length [0]\n" " --separate-callers= Separate callers for function \n" " --separate-recs= Separate function recursions up to level [2]\n" " --separate-recs= Separate recursions for function \n" @@ -696,6 +698,7 @@ void CLG_(set_clo_defaults)(void) CLG_(clo).skip_plt = True; CLG_(clo).separate_callers = 0; + CLG_(clo).separate_callers_all = False; CLG_(clo).separate_recursions = 2; CLG_(clo).skip_direct_recursion = False; diff --git a/callgrind/context.c b/callgrind/context.c index 55be820284..2e323e6039 100644 --- a/callgrind/context.c +++ b/callgrind/context.c @@ -160,22 +160,39 @@ static Bool is_cxt(UWord hash, fn_node** fn, Context* cxt) return True; } +/* Number of functions in the context of the top of a function stack. + * + * With --separate-callers=all, a function is separated by its whole call + * chain. Runtime code like V8's builtins and trampolines sits between most + * pairs of JS functions; keeping one node per builtin joins every JS caller + * to every JS callee through it, which merges unrelated call chains and forms + * false cycles. */ +static UInt cxt_size(fn_node** fn) +{ + Int size = (*fn)->separate_callers + 1; + if (size <= 0) size = -size + 1; + + if (!CLG_(clo).separate_callers_all) return size; + + for (size = 0; *fn != 0; fn--) size++; + return size; +} + /** * Allocate new Context structure */ -static Context* new_cxt(fn_node** fn) +static Context* new_cxt(fn_node** fn, UInt size) { Context* cxt; UInt idx, offset; UWord hash; - int size, recs; + int recs; fn_node* top_fn; CLG_ASSERT(fn); top_fn = *fn; if (top_fn == 0) return 0; - size = top_fn->separate_callers +1; recs = top_fn->separate_recursions; if (recs<1) recs=1; @@ -231,8 +248,7 @@ Context* CLG_(get_cxt)(fn_node** fn) CLG_ASSERT(fn != 0); if (*fn == 0) return 0; - size = (*fn)->separate_callers+1; - if (size<=0) { size = -size+1; } + size = cxt_size(fn); CLG_DEBUG(5, "+ get_cxt(fn '%s'): size %u\n", (*fn)->name, size); @@ -255,7 +271,7 @@ Context* CLG_(get_cxt)(fn_node** fn) } if (!cxt) - cxt = new_cxt(fn); + cxt = new_cxt(fn, size); (*fn)->last_cxt = cxt; diff --git a/callgrind/docs/cl-manual.xml b/callgrind/docs/cl-manual.xml index 8b4e50d853..37412bc2ca 100644 --- a/callgrind/docs/cl-manual.xml +++ b/callgrind/docs/cl-manual.xml @@ -875,11 +875,13 @@ Also see . - + Separate contexts by at most <callers> functions in the - call chain. See . + call chain. With , every function is separated by + its whole call chain, so each distinct stack gets its own context. + See . diff --git a/callgrind/global.h b/callgrind/global.h index 97a22bf252..4345aed1c7 100644 --- a/callgrind/global.h +++ b/callgrind/global.h @@ -112,6 +112,7 @@ struct _CommandLineOptions { Bool separate_threads; /* Separate threads in dump? */ Int separate_callers; /* Separate dependent on how many callers? */ Int separate_recursions; /* Max level of recursions to separate */ + Bool separate_callers_all; /* Separate by the whole call chain? */ Bool skip_plt; /* Skip functions in PLT section? */ Bool skip_direct_recursion; /* Increment direct recursions the level? */ diff --git a/callgrind/tests/Makefile.am b/callgrind/tests/Makefile.am index de3beb896a..536f817615 100644 --- a/callgrind/tests/Makefile.am +++ b/callgrind/tests/Makefile.am @@ -33,6 +33,11 @@ EXTRA_DIST = \ ret_below_frame_node.vgtest ret_below_frame_node.stderr.exp \ ret_below_frame_node.stdout.exp ret_below_frame_node.post.exp \ ret_below_frame_node.js \ + separate_callers_all_node.vgtest separate_callers_all_node.stderr.exp \ + separate_callers_all_node.stdout.exp separate_callers_all_node.post.exp \ + separate_callers_all_node.js separate_callers_all_node.post \ + pop_on_jump_node.vgtest pop_on_jump_node.stderr.exp \ + pop_on_jump_node.stdout.exp pop_on_jump_node.post.exp \ bug497723.stderr.exp bug497723.post.exp bug497723.vgtest \ simwork1.vgtest simwork1.stdout.exp simwork1.stderr.exp \ simwork2.vgtest simwork2.stdout.exp simwork2.stderr.exp \ diff --git a/callgrind/tests/pop_on_jump_node.post.exp b/callgrind/tests/pop_on_jump_node.post.exp new file mode 100644 index 0000000000..ccf0ed3302 --- /dev/null +++ b/callgrind/tests/pop_on_jump_node.post.exp @@ -0,0 +1 @@ +most OPENSSL_LH_doall frames in one context: 1 diff --git a/callgrind/tests/pop_on_jump_node.stderr.exp b/callgrind/tests/pop_on_jump_node.stderr.exp new file mode 100644 index 0000000000..d0b7820ae3 --- /dev/null +++ b/callgrind/tests/pop_on_jump_node.stderr.exp @@ -0,0 +1,6 @@ + + +Events : Ir +Collected : + +I refs: diff --git a/callgrind/tests/pop_on_jump_node.stdout.exp b/callgrind/tests/pop_on_jump_node.stdout.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/callgrind/tests/pop_on_jump_node.vgtest b/callgrind/tests/pop_on_jump_node.vgtest new file mode 100644 index 0000000000..c2648a5f93 --- /dev/null +++ b/callgrind/tests/pop_on_jump_node.vgtest @@ -0,0 +1,6 @@ +prereq: ../../tests/arch_test amd64 && command -v node >/dev/null +prog-asis: node +args: -e 0 +vgopts: --compress-strings=no --separate-callers=all --pop-on-jump --callgrind-out-file=callgrind.out.pop_on_jump_node +post: awk '/^fn=/ { n = gsub(/OPENSSL_LH_doall/, "&"); if (n > max) max = n } END { print "most OPENSSL_LH_doall frames in one context: " max }' callgrind.out.pop_on_jump_node +cleanup: rm -f callgrind.out.pop_on_jump_node diff --git a/callgrind/tests/separate_callers_all_node.js b/callgrind/tests/separate_callers_all_node.js new file mode 100644 index 0000000000..70a55ab462 --- /dev/null +++ b/callgrind/tests/separate_callers_all_node.js @@ -0,0 +1,27 @@ +// callerA only calls calleeX and callerB only calls calleeY, both through the +// same V8 call builtins. With --separate-callers=all those builtins are +// separated per JS caller, so no builtin context reaches both callees. + +function calleeX(v) { + return v + 1; +} + +function calleeY(v) { + return v + 2; +} + +function callerA(n) { + let sum = 0; + for (let i = 0; i < n; i++) sum += calleeX(i); + return sum; +} + +function callerB(n) { + let sum = 0; + for (let i = 0; i < n; i++) sum += calleeY(i); + return sum; +} + +let total = 0; +for (let k = 0; k < 200; k++) total += callerA(20) + callerB(20); +console.log(total); diff --git a/callgrind/tests/separate_callers_all_node.post b/callgrind/tests/separate_callers_all_node.post new file mode 100755 index 0000000000..36616cd725 --- /dev/null +++ b/callgrind/tests/separate_callers_all_node.post @@ -0,0 +1,26 @@ +#! /bin/sh + +# Count native contexts that call both calleeX and calleeY. JIT functions are +# named by address in the callgrind output; node's perf map resolves them. + +out=callgrind.out.separate_callers_all_node +pid=$(awk '/^pid:/ { print $2; exit }' "$out") +map=/tmp/perf-$pid.map + +addrs() { + awk -v fn="JS:^$1" '$3 == fn { a = $1; while (length(a) < 16) a = "0" a; print "0x" a }' "$map" +} + +callers() { + ./print_calls "$out" | awk -v addrs="$(addrs "$1")" ' + BEGIN { n = split(addrs, a, "\n"); for (i = 1; i <= n; i++) want[a[i]] = 1 } + $1 !~ /^0x/ { split($3, c, "'"'"'"); if (c[1] in want) print $1 }' | sort -u +} + +for fn in calleeX calleeY; do + [ -n "$(addrs $fn)" ] || echo "$fn missing from $map" +done +callers calleeX > callers.x +callers calleeY > callers.y +echo "native contexts calling both callees: $(comm -12 callers.x callers.y | wc -l)" +rm -f callers.x callers.y "$map" diff --git a/callgrind/tests/separate_callers_all_node.post.exp b/callgrind/tests/separate_callers_all_node.post.exp new file mode 100644 index 0000000000..023563377c --- /dev/null +++ b/callgrind/tests/separate_callers_all_node.post.exp @@ -0,0 +1 @@ +native contexts calling both callees: 0 diff --git a/callgrind/tests/separate_callers_all_node.stderr.exp b/callgrind/tests/separate_callers_all_node.stderr.exp new file mode 100644 index 0000000000..d0b7820ae3 --- /dev/null +++ b/callgrind/tests/separate_callers_all_node.stderr.exp @@ -0,0 +1,6 @@ + + +Events : Ir +Collected : + +I refs: diff --git a/callgrind/tests/separate_callers_all_node.stdout.exp b/callgrind/tests/separate_callers_all_node.stdout.exp new file mode 100644 index 0000000000..7545f5cca8 --- /dev/null +++ b/callgrind/tests/separate_callers_all_node.stdout.exp @@ -0,0 +1 @@ +88000 diff --git a/callgrind/tests/separate_callers_all_node.vgtest b/callgrind/tests/separate_callers_all_node.vgtest new file mode 100644 index 0000000000..4ec24020ac --- /dev/null +++ b/callgrind/tests/separate_callers_all_node.vgtest @@ -0,0 +1,6 @@ +prereq: ../../tests/arch_test amd64 && command -v node >/dev/null +prog-asis: node +args: --perf-basic-prof --always-sparkplug --no-opt --no-maglev separate_callers_all_node.js +vgopts: --compress-strings=no --separate-callers=all --pop-on-jump --callgrind-out-file=callgrind.out.separate_callers_all_node +post: ./separate_callers_all_node.post +cleanup: rm -f callgrind.out.separate_callers_all_node isolate-*-v8.log From 1a437dc98fafdd2b6e9a3c79f4e6ab66a3cd44e6 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Mon, 9 Nov 2026 12:43:50 +0100 Subject: [PATCH 3/3] fix(callgrind): keep the caller when a frameless callee tail-jumps A callee that pops its return address and then tail-jumps loses its frame to the SP rise before the jump: callgrind unwinds it as a return. With --pop-on-jump, the jump then replaced the caller's frame instead, so the jump target was recorded under the caller's caller. V8 does this for every JS call from Ignition bytecode (Builtins_InterpreterPushUndefinedAndArgsThenCall), which hung JS callees under the wrong JS function. When an SP rise unwinds a frame while execution stays in that callee, remember the callee in the exec state. A jump from it nests under the current top frame at the current SP, so the target's return is matched. pop_ra_tail_jump reproduces the pattern in native code: without the fix it reports main -> target, with it caller -> target. --- .gitignore | 1 + CODSPEED-CHANGELOG.md | 8 +++ callgrind/bbcc.c | 30 ++++++++-- callgrind/global.h | 3 + callgrind/tests/Makefile.am | 4 +- callgrind/tests/pop_ra_tail_jump.c | 62 +++++++++++++++++++++ callgrind/tests/pop_ra_tail_jump.post.exp | 4 ++ callgrind/tests/pop_ra_tail_jump.stderr.exp | 6 ++ callgrind/tests/pop_ra_tail_jump.stdout.exp | 1 + callgrind/tests/pop_ra_tail_jump.vgtest | 5 ++ callgrind/threads.c | 3 + 11 files changed, 120 insertions(+), 7 deletions(-) create mode 100644 callgrind/tests/pop_ra_tail_jump.c create mode 100644 callgrind/tests/pop_ra_tail_jump.post.exp create mode 100644 callgrind/tests/pop_ra_tail_jump.stderr.exp create mode 100644 callgrind/tests/pop_ra_tail_jump.stdout.exp create mode 100644 callgrind/tests/pop_ra_tail_jump.vgtest diff --git a/.gitignore b/.gitignore index 7623c1289f..b582855a6c 100644 --- a/.gitignore +++ b/.gitignore @@ -170,6 +170,7 @@ /callgrind/tests/prctl_get_auxv /callgrind/tests/deterministic_rng /callgrind/tests/ret_below_frame +/callgrind/tests/pop_ra_tail_jump /callgrind/tests/*.so /callgrind/tests/out_dbg /callgrind/tests/test-suite-overall.log diff --git a/CODSPEED-CHANGELOG.md b/CODSPEED-CHANGELOG.md index 24a4b29a2e..5581f812f4 100644 --- a/CODSPEED-CHANGELOG.md +++ b/CODSPEED-CHANGELOG.md @@ -25,6 +25,14 @@ below instead of the replaced one, so its return was often not recognized, and the caller was recorded as called again from the jump target. In node, OpenSSL's `OPENSSL_LH_doall` showed up 179 times nested in one call chain. +### Callgrind: `--pop-on-jump` keeps the caller of a callee that popped its return address + +A callee that pops its return address and then tail-jumps (V8's +`Builtins_InterpreterPushUndefinedAndArgsThenCall` does this for every JS +call from Ignition bytecode) has its frame ended by the SP rise. Its jump +then replaced the caller's frame instead, so the jump target was recorded +under the caller's caller. The jump target now nests under the caller. + ### Linux: glibc reports the main thread stack size correctly again `pthread_getattr_np` reports the main thread's stack size as `RLIMIT_STACK` diff --git a/callgrind/bbcc.c b/callgrind/bbcc.c index a794366a2d..a90497bfb0 100644 --- a/callgrind/bbcc.c +++ b/callgrind/bbcc.c @@ -572,7 +572,7 @@ void CLG_(setup_bbcc)(BB* bb) Int passed = 0, csp; Bool ret_without_call = False; Bool ret_below_frame = False; - Bool popped_on_jump = False; + Bool jump_keeps_sp = False; Int popcount_on_return = 1; CLG_DEBUG(3,"+ setup_bbcc(BB %#lx)\n", bb_addr(bb)); @@ -727,10 +727,19 @@ void CLG_(setup_bbcc)(BB* bb) bb->fn->name, bb->obj->name, ret_without_call?" (RET w/o CALL)":""); + fn_node* jumping_fn = CLG_(get_fn_node)(last_bb); + + if (jumping_fn->pop_on_jump && + jumping_fn == CLG_(current_state).frameless_fn) { + /* The jumping function has no frame left to replace: the top frame + * is its caller's, so the jump nests there at the current SP. */ + CLG_DEBUG(1," Nest frameless jump!\n"); + jump_keeps_sp = True; + } /* Entries without a jcc (skipped frames, frames seeded from the native * stack at START) have no call to replace, so the jump nests instead. */ - if (CLG_(get_fn_node)(last_bb)->pop_on_jump && (csp>0) && - CLG_(current_call_stack).entry[csp-1].jcc) { + else if (jumping_fn->pop_on_jump && (csp>0) && + CLG_(current_call_stack).entry[csp-1].jcc) { call_entry* top_ce = &(CLG_(current_call_stack).entry[csp-1]); @@ -741,7 +750,7 @@ void CLG_(setup_bbcc)(BB* bb) sp = top_ce->sp; passed = top_ce->jcc->jmp; CLG_(pop_call_stack)(); - popped_on_jump = True; + jump_keeps_sp = True; } jmpkind = jk_Call; @@ -785,7 +794,8 @@ void CLG_(setup_bbcc)(BB* bb) /* Handle CALL/RET and update context to get correct BBCC */ if (jmpkind == jk_Return) { - + CLG_(current_state).frameless_fn = 0; + if ((csp == 0) || ((CLG_(current_fn_stack).top > CLG_(current_fn_stack).bottom) && ( *(CLG_(current_fn_stack).top-1)==0)) ) { @@ -806,17 +816,25 @@ void CLG_(setup_bbcc)(BB* bb) } } else { + csp = CLG_(current_call_stack).sp; + fn_node* top_fn = (csp>0 && CLG_(current_call_stack).entry[csp-1].jcc) ? + CLG_(current_call_stack).entry[csp-1].jcc->to->cxt->fn[0] : 0; Int unwind_count = CLG_(unwind_call_stack)(sp, 0); if (unwind_count > 0) { /* if unwinding was done, this actually is a return */ jmpkind = jk_Return; + /* Unless the code still runs in the unwound callee: it popped its + * return address and keeps running on its caller's frame. */ + CLG_(current_state).frameless_fn = + (top_fn == CLG_(get_fn_node)(bb)) ? top_fn : 0; } if (jmpkind == jk_Call) { delayed_push = True; + CLG_(current_state).frameless_fn = 0; csp = CLG_(current_call_stack).sp; - if (call_emulation && csp>0 && !popped_on_jump) + if (call_emulation && csp>0 && !jump_keeps_sp) sp = CLG_(current_call_stack).entry[csp-1].sp; } diff --git a/callgrind/global.h b/callgrind/global.h index 4345aed1c7..b75af5a8ab 100644 --- a/callgrind/global.h +++ b/callgrind/global.h @@ -527,6 +527,9 @@ struct _exec_state { Int jmps_passed; BBCC* bbcc; /* last BB executed */ BBCC* nonskipped; + /* Function whose frame an SP rise unwound while it kept running (e.g. a + * callee popping its return address before a tail jump), 0 otherwise. */ + fn_node* frameless_fn; Int call_stack_bottom; /* Index into fn_stack */ }; diff --git a/callgrind/tests/Makefile.am b/callgrind/tests/Makefile.am index 536f817615..37b2bfca8a 100644 --- a/callgrind/tests/Makefile.am +++ b/callgrind/tests/Makefile.am @@ -38,6 +38,8 @@ EXTRA_DIST = \ separate_callers_all_node.js separate_callers_all_node.post \ pop_on_jump_node.vgtest pop_on_jump_node.stderr.exp \ pop_on_jump_node.stdout.exp pop_on_jump_node.post.exp \ + pop_ra_tail_jump.vgtest pop_ra_tail_jump.stderr.exp \ + pop_ra_tail_jump.stdout.exp pop_ra_tail_jump.post.exp \ bug497723.stderr.exp bug497723.post.exp bug497723.vgtest \ simwork1.vgtest simwork1.stdout.exp simwork1.stderr.exp \ simwork2.vgtest simwork2.stdout.exp simwork2.stderr.exp \ @@ -56,7 +58,7 @@ EXTRA_DIST = \ inline-crossfile.vgtest inline-crossfile.stderr.exp inline-crossfile.stdout.exp inline-crossfile.post.exp \ inline-crossfile-helper1.h inline-crossfile-helper2.h filter_inline -check_PROGRAMS = clreq register_desc register_desc_threads find_debuginfo simwork threads inline-samefile inline-crossfile runtime_obj_skip_c runtime_obj_skip_underflow deterministic_rng ret_below_frame +check_PROGRAMS = clreq register_desc register_desc_threads find_debuginfo simwork threads inline-samefile inline-crossfile runtime_obj_skip_c runtime_obj_skip_underflow deterministic_rng ret_below_frame pop_ra_tail_jump if HAVE_PR_GET_AUXV check_PROGRAMS += prctl_get_auxv endif diff --git a/callgrind/tests/pop_ra_tail_jump.c b/callgrind/tests/pop_ra_tail_jump.c new file mode 100644 index 0000000000..d2c858b124 --- /dev/null +++ b/callgrind/tests/pop_ra_tail_jump.c @@ -0,0 +1,62 @@ +/* A callee that pops its return address, pushes an argument and the return + * address back, then tail-jumps to the real target, like V8's + * Builtins_InterpreterPushUndefinedAndArgsThenCall. With --pop-on-jump the + * target must stay under the caller: the SP rise already ended the callee's + * frame, so the jump has no frame of its own to replace. */ + +#include + +__attribute__((noinline)) int leaf(int x) +{ + __asm__ volatile("" : "+r"(x)); + return x + 1; +} + +#if defined(__x86_64__) +int caller(int x); + +/* The taken branch after the pop starts a new block, so callgrind sees SP + * above the callee's frame before the jump, as with V8's argument loop. + * target drops the pushed argument on return, as V8 callees do. */ +__asm__( + ".text\n" + ".type push_arg_then_jump, @function\n" + "push_arg_then_jump:\n" + " pop %r11\n" + " test %rsp, %rsp\n" + " jnz 1f\n" + " ud2\n" + "1:\n" + " push $0\n" + " push %r11\n" + " jmp target\n" + ".size push_arg_then_jump, .-push_arg_then_jump\n" + + ".type target, @function\n" + "target:\n" + " sub $8, %rsp\n" + " call leaf\n" + " add $8, %rsp\n" + " ret $8\n" + ".size target, .-target\n" + + ".globl caller\n" + ".type caller, @function\n" + "caller:\n" + " sub $8, %rsp\n" + " call push_arg_then_jump\n" + " add $8, %rsp\n" + " ret\n" + ".size caller, .-caller\n"); + +int main(void) +{ + printf("%d\n", caller(1)); + return 0; +} +#else +int main(void) +{ + return 0; +} +#endif diff --git a/callgrind/tests/pop_ra_tail_jump.post.exp b/callgrind/tests/pop_ra_tail_jump.post.exp new file mode 100644 index 0000000000..6d9428d761 --- /dev/null +++ b/callgrind/tests/pop_ra_tail_jump.post.exp @@ -0,0 +1,4 @@ +caller -> push_arg_then_jump +caller -> target +main -> caller +target -> leaf diff --git a/callgrind/tests/pop_ra_tail_jump.stderr.exp b/callgrind/tests/pop_ra_tail_jump.stderr.exp new file mode 100644 index 0000000000..d0b7820ae3 --- /dev/null +++ b/callgrind/tests/pop_ra_tail_jump.stderr.exp @@ -0,0 +1,6 @@ + + +Events : Ir +Collected : + +I refs: diff --git a/callgrind/tests/pop_ra_tail_jump.stdout.exp b/callgrind/tests/pop_ra_tail_jump.stdout.exp new file mode 100644 index 0000000000..0cfbf08886 --- /dev/null +++ b/callgrind/tests/pop_ra_tail_jump.stdout.exp @@ -0,0 +1 @@ +2 diff --git a/callgrind/tests/pop_ra_tail_jump.vgtest b/callgrind/tests/pop_ra_tail_jump.vgtest new file mode 100644 index 0000000000..f49d376f04 --- /dev/null +++ b/callgrind/tests/pop_ra_tail_jump.vgtest @@ -0,0 +1,5 @@ +prereq: ../../tests/arch_test amd64 && test -f pop_ra_tail_jump +prog: pop_ra_tail_jump +vgopts: --compress-strings=no --pop-on-jump --callgrind-out-file=callgrind.out.pop_ra_tail_jump +post: ./print_calls callgrind.out.pop_ra_tail_jump | grep -E 'caller|push_arg_then_jump|target|leaf' +cleanup: rm -f callgrind.out.pop_ra_tail_jump diff --git a/callgrind/threads.c b/callgrind/threads.c index 642813e4cb..ad536b5f74 100644 --- a/callgrind/threads.c +++ b/callgrind/threads.c @@ -441,6 +441,7 @@ void CLG_(init_exec_state)(exec_state* es) es->jmps_passed = 0; es->bbcc = 0; es->nonskipped = 0; + es->frameless_fn = 0; } @@ -540,6 +541,7 @@ exec_state* exec_state_save(void) es->jmps_passed = CLG_(current_state).jmps_passed; es->bbcc = CLG_(current_state).bbcc; es->nonskipped = CLG_(current_state).nonskipped; + es->frameless_fn = CLG_(current_state).frameless_fn; CLG_ASSERT(es->cost == CLG_(current_state).cost); CLG_DEBUGIF(1) { @@ -565,6 +567,7 @@ exec_state* exec_state_restore(void) CLG_(current_state).jmps_passed = es->jmps_passed; CLG_(current_state).bbcc = es->bbcc; CLG_(current_state).nonskipped = es->nonskipped; + CLG_(current_state).frameless_fn = es->frameless_fn; CLG_(current_state).cost = es->cost; CLG_(current_state).sig = es->sig;