Skip to content

Commit 1d23d5f

Browse files
committed
fix(coregrind): arm64 FP-chain unwind fallback when CFI is missing
VG_(get_StackTrace) on arm64 was CFI-only, so Callgrind's OFF->ON shadow-stack seeding stopped at the first frame without unwind info -- notably CPython's -X perf JIT trampolines, which have no FDEs but keep the AAPCS64 frame chain alive (that is their design: perf/samply walk them by FP). The truncated seed left the seeded context stack one entry deep after the innermost frame popped; bbcc.c's underflow check then misread the fn-stack base sentinel as a signal marker on every return, and handleUnderflow fabricated named nodes for obj-skipped interpreter functions with inverted, full-cost edges (_ctypes_callproc -> PyCFuncPtr_call -> _TAIL_CALL_* -> ... as the graph root). x86_64 never hit this because its unwinder already has the %rbp fallback. Follow the frame records {saved X29, saved X30} when CFI fails, with guards: record in-stack and 8-aligned, SP must progress, next record strictly higher (saved X29 == 0 accepted as chain terminator), pc 0/1 stops the walk. Caller IPs keep the CFI path's -1 bias. Callgrind's seeder correspondingly records ret_addr = ips[frame+1] + 1, undoing that bias so the arm64 return matcher (exact-X30 matching, no SP movement on bl/ret) can pop seeded frames. An A/B run confirmed raising CLG_RECON_MAX_FRAMES alone does not help: the seed was CFI-truncated at ~8 frames, nowhere near the 256 cap. Regression coverage: callgrind-utils/tests/objskip_seed_underflow.rs, a minimal fixture whose asm trampoline maintains FP but has no CFI and which starts instrumentation two obj-skipped frames deep; it asserts no skipped frame leaks into the folded graph and that the workload parents under the trampoline.
1 parent a099b30 commit 1d23d5f

2 files changed

Lines changed: 59 additions & 2 deletions

File tree

callgrind/callstack.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,15 @@ void CLG_(reconstruct_call_stack_from_native)(ThreadId tid)
568568
* SP, sps[frame+1]; the outermost frame keeps its own SP as nothing
569569
* returns past it during measurement. */
570570
ce->sp = (frame + 1 < (Int)n) ? sps[frame + 1] : sps[frame];
571-
ce->ret_addr = (frame + 1 < (Int)n) ? ips[frame + 1] : 0;
571+
/* ret_addr must be the exact architectural return target: the arm64
572+
* return detector (setup_bbcc) matches it against bb_addr() of the
573+
* returned-into block, and push_call_stack records the exact X30 there.
574+
* But VG_(get_StackTrace) reports each caller IP as return_addr - 1 (so
575+
* symbolization lands in the calling instruction, not the one after);
576+
* undo that bias here, else every seeded frame's return is off by one,
577+
* is misread as a jump, and the seeded skipped interpreter/ctypes frames
578+
* never pop -- leaking them as the graph root. */
579+
ce->ret_addr = (frame + 1 < (Int)n) ? ips[frame + 1] + 1 : 0;
572580
cs->sp++;
573581
ensure_stack_size(cs->sp + 1);
574582
cs->entry[cs->sp].cxt = 0;

coregrind/m_stacktrace.c

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1249,8 +1249,11 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
12491249
ips[0] = uregs.pc;
12501250
i = 1;
12511251

1252-
/* Loop unwinding the stack, using CFI. */
1252+
/* Loop unwinding the stack: CFI first, AAPCS64 frame-pointer chain as
1253+
the fallback. */
12531254
while (True) {
1255+
Addr old_sp;
1256+
12541257
if (debug) {
12551258
VG_(printf)("i: %d, pc: 0x%lx, sp: 0x%lx\n",
12561259
i, uregs.pc, uregs.sp);
@@ -1259,6 +1262,8 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
12591262
if (i >= max_n_ips)
12601263
break;
12611264

1265+
old_sp = uregs.sp;
1266+
12621267
if (VG_(use_CF_info)( &uregs, fp_min, fp_max )) {
12631268
if (sps) sps[i] = uregs.sp;
12641269
if (fps) fps[i] = uregs.x29;
@@ -1271,6 +1276,50 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
12711276
continue;
12721277
}
12731278

1279+
/* If VG_(use_CF_info) fails, the location has no unwind info: JIT
1280+
pages (e.g. CPython's -X perf trampolines) or hand-written
1281+
assembly. Fall back to following the AAPCS64 frame-pointer chain:
1282+
X29 points at a frame record { saved X29, saved X30 }. Code built
1283+
for fp-based profilers (perf, samply) maintains this chain exactly
1284+
where CFI is missing. Mirrors the amd64 %rbp fallback, with the
1285+
same guards: the record must lie inside the stack, the recovered
1286+
SP must make progress towards the stack base, and the next record
1287+
must be strictly further up (a saved X29 of 0 is the conventional
1288+
chain terminator: emit this frame, the bounds check ends the walk
1289+
on the next iteration). Stop rather than emit a bogus trail. */
1290+
if (VG_IS_8_ALIGNED(uregs.x29)
1291+
&& fp_min <= uregs.x29
1292+
&& uregs.x29 <= fp_max - 2 * sizeof(Addr)) {
1293+
Addr next_x29 = ((Addr*)uregs.x29)[0];
1294+
Addr next_pc = ((Addr*)uregs.x29)[1];
1295+
/* End-of-chain sentinel, same test the other unwinders in this
1296+
file use. Recorded pcs are decremented by 1 (see below) so the
1297+
symbol lookup lands on the call insn rather than the return
1298+
address; both 0 and 1 collapse to the null address 0 after that
1299+
-1, so treat either as "no real caller" and stop. */
1300+
if (0 == next_pc || 1 == next_pc) break;
1301+
uregs.sp = uregs.x29 + 2 * sizeof(Addr);
1302+
if (old_sp >= uregs.sp
1303+
|| (next_x29 != 0 && next_x29 <= uregs.x29)) {
1304+
if (debug)
1305+
VG_(printf)(" FF end of stack sp %#lx next x29 %#lx\n",
1306+
uregs.sp, next_x29);
1307+
break;
1308+
}
1309+
uregs.x29 = next_x29;
1310+
uregs.x30 = next_pc;
1311+
uregs.pc = next_pc;
1312+
if (sps) sps[i] = uregs.sp;
1313+
if (fps) fps[i] = uregs.x29;
1314+
ips[i++] = uregs.pc - 1; /* -1: refer to calling insn, not the RA */
1315+
if (debug)
1316+
VG_(printf)("USING FP: pc: 0x%lx, sp: 0x%lx\n",
1317+
uregs.pc, uregs.sp);
1318+
uregs.pc = uregs.pc - 1;
1319+
RECURSIVE_MERGE(cmrf,ips,i);
1320+
continue;
1321+
}
1322+
12741323
/* No luck. We have to give up. */
12751324
break;
12761325
}

0 commit comments

Comments
 (0)