From b52f6d3721da6164687350fff8bd929e934d7725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Tue, 14 Mar 2023 18:25:31 +0100 Subject: [PATCH] runtime: fix frame pointer loop on amd64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses a regression caused by commit 43f911b0b6c550e6c5b46219d8d0d1ca7ce3f97c (CL 472195) which led to frame pointer cycles, causing frame pointer unwinders (refer to CL 463835) to encounter repetitive stack frames. The issue occurs when mcall invokes fn on g0's stack. fn is expected not to return but to continue g's execution through gogo(&g.sched). To achieve this, g.sched must hold the sp, pc, and bp of mcall's caller. CL 472195 mistakenly altered g.sched.bp to store mcall's own bp, causing gogo to resume execution with a bp value that points downwards into the now non-existent mcall frame. This results in the next function call executed by mcall's callee pushing a bp that points to itself on the stack, creating a pointer loop. Fix this by dereferencing bp before storing it in g.sched.bp to reinstate the correct behavior. Although this problem could potentially be resolved by reverting the mcall-related changes from CL 472195, doing so would hide mcall's caller frame from async frame pointer unwinders like Linux perf when unwinding during fn's execution. Currently, there is no test coverage for frame pointers to validate these changes. However, runtime/trace.TestTraceSymbolize at CL 463835 will add basic test coverage and can be used to validate this change. Change-Id: Iad3c42908eeb1b0009fcb839d7fcfffe53d13326 Reviewed-on: https://go-review.googlesource.com/c/go/+/476235 TryBot-Result: Gopher Robot Reviewed-by: Michael Pratt Reviewed-by: Cherry Mui Run-TryBot: Felix Geisendörfer --- src/runtime/asm_amd64.s | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/runtime/asm_amd64.s b/src/runtime/asm_amd64.s index c8641cb2c26..690d6bacf01 100644 --- a/src/runtime/asm_amd64.s +++ b/src/runtime/asm_amd64.s @@ -428,15 +428,18 @@ TEXT gogo<>(SB), NOSPLIT, $0 TEXT runtime·mcall(SB), NOSPLIT, $0-8 MOVQ AX, DX // DX = fn - // Save state in g->sched. - // The original frame pointer is stored in BP, - // which is useful for stack unwinding. + // Save state in g->sched. The caller's SP and PC are restored by gogo to + // resume execution in the caller's frame (implicit return). The caller's BP + // is also restored to support frame pointer unwinding. MOVQ SP, BX // hide (SP) reads from vet MOVQ 8(BX), BX // caller's PC MOVQ BX, (g_sched+gobuf_pc)(R14) LEAQ fn+0(FP), BX // caller's SP MOVQ BX, (g_sched+gobuf_sp)(R14) - MOVQ BP, (g_sched+gobuf_bp)(R14) + // Get the caller's frame pointer by dereferencing BP. Storing BP as it is + // can cause a frame pointer cycle, see CL 476235. + MOVQ (BP), BX // caller's BP + MOVQ BX, (g_sched+gobuf_bp)(R14) // switch to m->g0 & its stack, call fn MOVQ g_m(R14), BX