diff --git a/.gitignore b/.gitignore index 97a35bea0..7623c1289 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 e5e366b44..6ed973281 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 910d49419..fe83a90df 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 0b7f9965d..de3beb896 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 000000000..a6d39d8fa --- /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 000000000..3ec58064a --- /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 000000000..0df3efe3d --- /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 000000000..d0b7820ae --- /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 000000000..1e8b31496 --- /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 000000000..039e9bff2 --- /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 000000000..2cfd70849 --- /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 000000000..6ace5705e --- /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 000000000..d0b7820ae --- /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 000000000..17d6e3f6b --- /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 000000000..0504df148 --- /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