runtime: support unwinding logical cgo stack transitions using frame pointers #58378

qmuntal · 2023-02-07T13:18:17Z

Note: follow-up issue for #40044 (comment)

When using generic third-party debuggers (e.g. dbg or windbg) to debug a amd64 Go binary, it is currently not possible to know which piece of Go code started a cgo call once runtime.asmcgocall has switched from the Go routine stack to the system stack. This is because asmcgocall is marked as NOFRAME, so the frame pointer is not set on the function prologue, confusing the debugger unwinding process.

Notice that this is only true for amd64 binaries, as all other architectures supporting frame pointers do not mark asmcgocall as NOFRAME. Yet, at least windows/arm64 neither supports cgo stack unwinding because asmcgocall calls asmstdcall, which is marked as NOFRAME.

IMO we should at least make sure that asmcgocall and related-auxiliary functions can be unwound from the system stack up to the Go stack by placing the right frame pointer in their prologue. I'm leaving out of this discussion other functions that perform a system stack on purpose, as they are not so fundamental as asmcgocall and correctly setting the frame pointer of low-level assembly functions is not straight-forward.

I'm particularly interested on this because on Windows, WinDbg uses the frame pointer to unwind the stack (see #57302) to unwound the stack of a debugged binary or crash minidump (#57441, #49471, #20498).

It would also help fixing at least #40044 and #57698.

@golang/runtime @golang/compiler

The text was updated successfully, but these errors were encountered:

mknyszek · 2023-02-15T20:18:30Z

@qmuntal I've assigned this to you for now in triage; do you plan to work on it? (Feel free to unassign and leave a "help wanted" tag.)

qmuntal · 2023-02-16T08:30:02Z

@qmuntal I've assigned this to you for now in triage; do you plan to work on it?

Yep. In fact I've already submitted a bunch of CLs (not merged yet) that will make this issue easier to implement (although their are valuable on their own). Will update them referencing this issue.

gopherbot · 2023-02-16T08:32:47Z

Change https://go.dev/cl/466835 mentions this issue: runtime: remove unnecessary NOFRAME flags on windows

gopherbot · 2023-02-16T08:34:54Z

Change https://go.dev/cl/466396 mentions this issue: runtime: use explicit NOFRAME on netbsd/amd64

gopherbot · 2023-02-16T08:34:55Z

Change https://go.dev/cl/466457 mentions this issue: runtime: use explicit NOFRAME on plan9/amd64

gopherbot · 2023-02-16T08:34:56Z

Change https://go.dev/cl/466455 mentions this issue: runtime: use explicit NOFRAME on openbsd/amd64

gopherbot · 2023-02-16T08:34:57Z

Change https://go.dev/cl/466316 mentions this issue: runtime: use explicit NOFRAME on linux/amd64

gopherbot · 2023-02-16T08:34:58Z

Change https://go.dev/cl/466458 mentions this issue: runtime: remove implicit NOFRAME heuristic support

gopherbot · 2023-02-16T08:34:59Z

Change https://go.dev/cl/466395 mentions this issue: runtime: use explicit NOFRAME on freebsd/amd64

gopherbot · 2023-02-16T08:35:00Z

Change https://go.dev/cl/466355 mentions this issue: runtime: use explicit NOFRAME on dragonfly/amd64

gopherbot · 2023-02-16T08:35:01Z

Change https://go.dev/cl/466456 mentions this issue: runtime: use explicit NOFRAME on solaris/amd64

This CL marks some linux assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. Updates #58378 Change-Id: I7792cff4f6e539bfa56c02868f2965088ca1975a Reviewed-on: https://go-review.googlesource.com/c/go/+/466316 Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Run-TryBot: Quim Muntal <quimmuntal@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org>

This CL marks some dragonfly assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. Updates #58378 Change-Id: I832a1a78d68a49f11df3b03fa9d50d4796bcac03 Reviewed-on: https://go-review.googlesource.com/c/go/+/466355 Run-TryBot: Quim Muntal <quimmuntal@gmail.com> Reviewed-by: Than McIntosh <thanm@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com>

This CL marks some freebsd assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. Updates #58378 Change-Id: Ibd00748946f1137e165293df7da73278cb673bbd Reviewed-on: https://go-review.googlesource.com/c/go/+/466395 Reviewed-by: Than McIntosh <thanm@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Run-TryBot: Quim Muntal <quimmuntal@gmail.com>

This CL marks some netbsd assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. While here, and thanks to CL 466355, `asm_netbsd_amd64.s` can be deleted in favor of `asm9_unix2_amd64.s`, which makes better use of the frame pointer. Updates #58378 Change-Id: Iff554b664ec25f2bb6ec198c0f684590b359c383 Reviewed-on: https://go-review.googlesource.com/c/go/+/466396 Reviewed-by: Than McIntosh <thanm@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Run-TryBot: Quim Muntal <quimmuntal@gmail.com>

This CL marks some openbsd assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. Updates #58378 Change-Id: I993549df41a93255fb714357443f8b24c3dfb0a4 Reviewed-on: https://go-review.googlesource.com/c/go/+/466455 Reviewed-by: Keith Randall <khr@google.com> Run-TryBot: Quim Muntal <quimmuntal@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Joel Sing <joel@sing.id.au> Reviewed-by: Cherry Mui <cherryyz@google.com>

This CL marks some solaris assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. While here, I've reduced the stack usage of runtime·sigtramp by 16 bytes to compensate the additional 8 bytes from the stack-allocated frame pointer. There were two unused 8-byte slots on the stack, one at 24(SP) and the other at 80(SP). Updates #58378 Change-Id: If9230e71a8b3c72681ffc82030ade6ceccf824db Reviewed-on: https://go-review.googlesource.com/c/go/+/466456 Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Quim Muntal <quimmuntal@gmail.com>

gopherbot · 2023-02-28T18:39:25Z

Change https://go.dev/cl/472195 mentions this issue: runtime: remove NOFRAME from asmcgocall and systemstack

felixge · 2023-02-28T20:32:01Z

👋 It's great to see your CL above. I'm currently working on adding frame pointer unwind support to the execution tracer (much faster than gentraceback), see #16638 . systemstack() not pushing frame pointers is an issue I also encountered, causing the caller frame of systemstack to be missing.

qmuntal · 2023-02-28T20:55:13Z

It was been quite a journey getting to that CL 🥲, glad that it also helps other effords other than my goal of enabling SEH unwinding on Windows.

Now that I almost managed to get rid of the implicit NOFRAME rule, it would be easier to have frame pointers in the runtime functions.

This CL removes some NOFRAME flags on Windows assembly files for several reasons: - windows/386 does not use a frame pointer - Leaf frameless functions already skip the frame pointer - Some non-leaf functions do not contain enough dragons to justify not using the frame pointer Updates #58378 Change-Id: I31e71bf7f769e1957a4adba91778da5af66ce1e4 Reviewed-on: https://go-review.googlesource.com/c/go/+/466835 Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Keith Randall <khr@google.com> Run-TryBot: Quim Muntal <quimmuntal@gmail.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

This CL marks some plan9 assembly functions as NOFRAME to avoid relying on the implicit amd64 NOFRAME heuristic, where NOSPLIT functions without stack were also marked as NOFRAME. Updates #58378 Change-Id: Ic8c9ab5c1a0897bebc6c1419ddc903a7492a1b0a Reviewed-on: https://go-review.googlesource.com/c/go/+/466457 TryBot-Bypass: Quim Muntal <quimmuntal@gmail.com> Reviewed-by: Keith Randall <khr@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com>

All amd64 OSes already make use of the NOFRAME flag wherever is required, so we can remove the frameless nosplit functions heuristic code path. Updates #58378 Change-Id: I966970693ba07f8c66da0aca83c23caad7cbbfe5 Reviewed-on: https://go-review.googlesource.com/c/go/+/466458 Reviewed-by: Cherry Mui <cherryyz@google.com> Run-TryBot: Quim Muntal <quimmuntal@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>

nightlyone · 2023-03-02T04:12:44Z

Please consider a release note about that semantic change that may surprise assembler code outside of the tree. @qmuntal

qmuntal · 2023-03-02T17:35:16Z

Please consider a release note about that semantic change that may surprise assembler code outside of the tree. @qmuntal

Sure

@aclements I'm having problems on CL 472195 trying to get TestTracebackSystemstack happy. It seems that gentraceback doesn't like the fact that systemstack now has a frame pointer. Manage to fix it, but without knowing why and probably by chance. As you are working on refactoring that gentraceback, could you provide some guidance on how to properly fix that?

cherrymui · 2023-03-02T17:47:04Z

FWIW, I'm still not convinced that saving frame pointers is a good idea for stack transition functions like systemstack, at least not implicitly. If we want to use the frame pointer to express stack transition, I think we may want to be very explicit.

qmuntal · 2023-03-02T19:38:28Z

FWIW, I'm still not convinced that saving frame pointers is a good idea for stack transition functions like systemstack, at least not implicitly. If we want to use the frame pointer to express stack transition, I think we may want to be very explicit.

Your opinion worth alot 😸. What I don't understand is what could go wrong when saving the frame pointer on functions that change the stack. And, if there are caveats, what could be worse than not having any stack unwinding at all?

cherrymui · 2023-03-02T22:04:29Z

I'm not sure if it could go wrong, just that as the stack transition is special enough I think it is clearer that we do it more explicitly, with comments. It may also be simpler if we save the FP only when the stack transition actually happens, and not on the tail call case.

For the traceback failure (I didn't read the code or the failure message, just guessing), could it be that the frame layout of systemstack_switch need to match? Also the comment at gosave_systemstack_switch may be helpful.

aarzilli · 2023-03-03T08:03:07Z

I remembered that one of the problems we had with delve's stack unwinder was the morestack/newstack path. Maybe you have already accounted for it, I thought it mention it just in case.

felixge · 2023-03-03T08:12:39Z

the stack transition is special enough I think it is clearer that we do it more explicitly, with comments

Not sure I follow. Are you talking about some sort of compiler directive like this?

//go:emitfp
systemstack(func() { ... })

Sorry if this makes no sense, just trying to make sure I understand your comments 😅 .

If we want to use the frame pointer to express stack transition, I think we may want to be very explicit.

FWIW, while working on CL 463835 I found that frame pointer unwinding through systemstack seems to be working already because systemstack doesn't clobber the BP register, so it gets saved by the callee of systemstack. This causes an unwinder to miss the caller of systemstack, but the stack transition itself seems to be working.

qmuntal · 2023-03-03T10:27:16Z

For the traceback failure (I didn't read the code or the failure message, just guessing), could it be that the frame layout of systemstack_switch need to match? Also the comment at gosave_systemstack_switch may be helpful.

Thanks for the advice, the frame layout of systemstack_switch did have to match. Tests are green now without having to modify gentraceback.

I'm not sure if it could go wrong, just that as the stack transition is special enough I think it is clearer that we do it more explicitly, with comments.

Have the same questions as @felixge, plus one data point more: arm64 versions of asmcgocall and systemstack already place the frame pointer on the stack.

It may also be simpler if we save the FP only when the stack transition actually happens, and not on the tail call case.

I tried this at some point, but it would create another (and probably bigger) issue: the FP would be stored outside the function prologue, which is non-standard. Doing so will make it difficult to programmatically generate DWARF unwinding info using the FP instead of stack deltas, and AFAIK will not work on Windows with SEH unwinding info, which requires the FP to be on the prologue, and the prologue is limited to 255 bytes.

cherrymui · 2023-03-03T17:16:27Z

//go:emitfp
systemstack(func() { ... })

No. I was just thinking explicitly saving FP in the assembly code. If you all prefer the implicit saving, that is fine. But I think somewhere in the code we should have a comment mentioning that the frame pointer is chained through the stack transition.

arm64 versions of asmcgocall

Not sure if that is actually correct (https://golang.org/cl/241080). It does do the implicit saving, though.

felixge · 2023-03-04T13:55:18Z

No. I was just thinking explicitly saving FP in the assembly code. If you all prefer the implicit saving, that is fine.

Ah, got it! I don't have a preference on that. If it's better to do this "by hand" in the assembly rather than letting the compiler generate it, that works for me. As long as the frame pointer gets pushed I'm happy 😄.

This CL removes the NOFRAME flag from runtime.asmcgocall, runtime.systemstack and runtime.mcall so the compiler can place the frame pointer on the stack. This will help unwinding cgo stack frames, and might be all what's needed for tools that only use the frame pointer to unwind the stack. That's not the case for gdb, which uses DWARF CFI, and windbg, which uses SEH. Yet, having the frame pointer correctly set lays the foundation for supporting cgo unwinding with DWARF CFI and SEH. Updates #58378 Change-Id: I7655363b3fb619acccd9d5a7f0e3d3dec953cd52 Reviewed-on: https://go-review.googlesource.com/c/go/+/472195 Run-TryBot: Quim Muntal <quimmuntal@gmail.com> Auto-Submit: Michael Pratt <mpratt@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com>

qmuntal · 2023-04-21T07:28:25Z

Closing this issue as done. There has been many CLs during go1.21 dev cycle to improve the frame pointer unwinding through the cgo transition, and there are even some tests verifying that there are no regressions in this area, i.e. @felixge's CL 474916.

Thanks everyone for the discussion and the reviews!

gopherbot · 2023-06-08T10:43:00Z

Change https://go.dev/cl/501516 mentions this issue: doc/go1.21: NOFRAME heuristic changes

For #58378 Change-Id: I960b97f33a8bf29d3a9622b58d278544d0970a38 Reviewed-on: https://go-review.googlesource.com/c/go/+/501516 Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Quim Muntal <quimmuntal@gmail.com> Reviewed-by: David Chase <drchase@google.com>

qmuntal added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Debugging compiler/runtime Issues related to the Go compiler and/or runtime. labels Feb 7, 2023

mknyszek assigned qmuntal Feb 15, 2023

mknyszek added this to the Backlog milestone Feb 15, 2023

This was referenced Feb 23, 2023

x/build: Add gdb to Windows Builders #22021

Open

runtime: in core dump generated for a signal while executing C code, gdb reports a corrupt stack #57698

Open

qmuntal closed this as completed Apr 21, 2023

qmuntal modified the milestones: Backlog, Go1.21 Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime: support unwinding logical cgo stack transitions using frame pointers #58378

runtime: support unwinding logical cgo stack transitions using frame pointers #58378

qmuntal commented Feb 7, 2023

mknyszek commented Feb 15, 2023

qmuntal commented Feb 16, 2023 •

edited

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 28, 2023

felixge commented Feb 28, 2023

qmuntal commented Feb 28, 2023

nightlyone commented Mar 2, 2023

qmuntal commented Mar 2, 2023

cherrymui commented Mar 2, 2023

qmuntal commented Mar 2, 2023

cherrymui commented Mar 2, 2023 •

edited

aarzilli commented Mar 3, 2023

felixge commented Mar 3, 2023 •

edited

qmuntal commented Mar 3, 2023 •

edited

cherrymui commented Mar 3, 2023

felixge commented Mar 4, 2023

qmuntal commented Apr 21, 2023 •

edited

gopherbot commented Jun 8, 2023

runtime: support unwinding logical cgo stack transitions using frame pointers #58378

runtime: support unwinding logical cgo stack transitions using frame pointers #58378

Comments

qmuntal commented Feb 7, 2023

mknyszek commented Feb 15, 2023

qmuntal commented Feb 16, 2023 • edited

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 16, 2023

gopherbot commented Feb 28, 2023

felixge commented Feb 28, 2023

qmuntal commented Feb 28, 2023

nightlyone commented Mar 2, 2023

qmuntal commented Mar 2, 2023

cherrymui commented Mar 2, 2023

qmuntal commented Mar 2, 2023

cherrymui commented Mar 2, 2023 • edited

aarzilli commented Mar 3, 2023

felixge commented Mar 3, 2023 • edited

qmuntal commented Mar 3, 2023 • edited

cherrymui commented Mar 3, 2023

felixge commented Mar 4, 2023

qmuntal commented Apr 21, 2023 • edited

gopherbot commented Jun 8, 2023

qmuntal commented Feb 16, 2023 •

edited

cherrymui commented Mar 2, 2023 •

edited

felixge commented Mar 3, 2023 •

edited

qmuntal commented Mar 3, 2023 •

edited

qmuntal commented Apr 21, 2023 •

edited