fix(callgrind): end a call that returns with SP below its frame

.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

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`

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);

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

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

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 <stdio.h>
+
+__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

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

callgrind/tests/ret_below_frame.stderr.exp

@@ -0,0 +1,6 @@
+
+
+Events    : Ir
+Collected :
+
+I   refs:

callgrind/tests/ret_below_frame.stdout.exp

@@ -0,0 +1 @@
+6

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

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);

callgrind/tests/ret_below_frame_node.post.exp

@@ -0,0 +1,2 @@
+prologue called
+prologue calls into JIT code: 0

callgrind/tests/ret_below_frame_node.stderr.exp

@@ -0,0 +1,6 @@
+
+
+Events    : Ir
+Collected :
+
+I   refs:

callgrind/tests/ret_below_frame_node.stdout.exp

@@ -0,0 +1 @@
+7270000

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