fix(callgrind): correct V8 call graphs (Sparkplug returns, whole-stack contexts, pop-on-jump)

.gitignore

@@ -169,6 +169,8 @@
 /callgrind/tests/runtime_obj_skip_underflow
 /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

CODSPEED-CHANGELOG.md

@@ -4,6 +4,35 @@ 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.
+
+### 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.
+
+### 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`
@@ -37,6 +66,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=<n>`, 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

callgrind/bbcc.c

@@ -571,6 +571,8 @@ void CLG_(setup_bbcc)(BB* bb)
   Bool isConditionalJump;
   Int passed = 0, csp;
   Bool ret_without_call = False;
+  Bool ret_below_frame = False;
+  Bool jump_keeps_sp = False;
   Int popcount_on_return = 1;
 
   CLG_DEBUG(3,"+ setup_bbcc(BB %#lx)\n", bb_addr(bb));
@@ -664,11 +666,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;
@@ -717,23 +727,30 @@ 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)) {
+	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. */
+	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]);
-
-	    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)();
+	    jump_keeps_sp = True;
 	}
 
 	jmpkind = jk_Call;
@@ -777,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)) ) {
@@ -788,23 +806,35 @@ 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);
     }
   }
   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)
+      if (call_emulation && csp>0 && !jump_keeps_sp)
 	sp = CLG_(current_call_stack).entry[csp-1].sp;	
 
     }

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=<n>    Separate functions by call chain length [0]\n"
+"    --separate-callers=<n>|all  Separate functions by call chain length [0]\n"
 "    --separate-callers<n>=<f> Separate <n> callers for function <f>\n"
 "    --separate-recs=<n>       Separate function recursions up to level [2]\n"
 "    --separate-recs<n>=<f>    Separate <n> recursions for function <f>\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;
 

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;
 

callgrind/docs/cl-manual.xml

@@ -875,11 +875,13 @@ Also see <xref linkend="cl-manual.cycles"/>.</para>
 
   <varlistentry id="opt.separate-callers" xreflabel="--separate-callers">
     <term>
-      <option><![CDATA[--separate-callers=<callers> [default: 0] ]]></option>
+      <option><![CDATA[--separate-callers=<callers>|all [default: 0] ]]></option>
     </term>
     <listitem>
       <para>Separate contexts by at most &lt;callers&gt; functions in the
-      call chain. See <xref linkend="cl-manual.cycles"/>.</para>
+      call chain. With <option>all</option>, every function is separated by
+      its whole call chain, so each distinct stack gets its own context.
+      See <xref linkend="cl-manual.cycles"/>.</para>
     </listitem>
   </varlistentry>
 

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? */
 
@@ -526,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 */
 };

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,18 @@ 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 \
+	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 \
+	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 \
@@ -46,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
+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

callgrind/tests/pop_on_jump_node.post.exp

@@ -0,0 +1 @@
+most OPENSSL_LH_doall frames in one context: 1

callgrind/tests/pop_on_jump_node.stderr.exp

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

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