Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: support SEH stack unwinding on Windows #57302

Open
qmuntal opened this issue Dec 14, 2022 · 25 comments
Open

runtime: support SEH stack unwinding on Windows #57302

qmuntal opened this issue Dec 14, 2022 · 25 comments
Assignees
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. Debugging OS-Windows
Milestone

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Dec 14, 2022

Status update: SEH stack unwinding has been implemented in go1.20 for windows/amd64. Still missing windows/arm64.

Background

Go binaries can't be reliably debugged or profiled with native Windows tools, such as WinDbg or the Windows Performance Analyzer, because Go does not generate PE files which contains the necessary static data that Win32 functions like RtlVirtualUnwind and StackWalk use to unwind the stack.

Delve and go tool pprof are great tools for developing on Windows, but production environments that run on Windows tend to rely on language-agnostic tools provided by Microsoft for profiling and troubleshooting. Stack unwinding is such a fundamental thing for all these tools, and Go not supporting it is a major pain point in the Windows ecosystem, at least when running production workloads.

Proposal

The Go compiler and linker should emit the necessary SEH static data for each Go function to reliably unwind and walk the stack using the Windows stack unwind conventions for each architecture:

This new information will slightly increase the size of the final binary, around 12 bytes per non-leaf functions.

Stack unwinding overview

Note: each architecture has slightly different design, the following explanation is based on x64.

Stack unwinding normally take place in these three cases:

RtlVirtualUnwind unwinds exactly one frame from the stack and has two important parameters: ControlPC and FunctionEntry. The former is the PC from where to start the unwinding, and the later is the frame information of the current function. This frame information is what comes from the static data in the PE files, more specifically from the .pdata and .xdata sections. It contains the following bits: function length, prolog length, frame pointer location (if used), where does the stack grow to accommodate local variable, how to restore non-volatile registers, and the exception handler address (if any). RtlVirtualUnwind will use this information to restore the context of the calling function without physically walking the stack. If this information is not present (current situation in Go binaries), it will naively take the return address from the last 4/8 bytes of the stack, which really only works for leaf functions, and for non-leaf functions it means that the return address points to whatever value the last local variable happens to contain.

There is one important outcome of this explanation: tools using RtlVirtualUnwind will unwind Go binaries even if no unwind information is present in the PE (current situation), this process will never work correctly unless unwinding a leaf function. So, whatever we do, even if not perfect, will be an improvement over the current situation.

Implementation

I would rather keep the implementation details out of this discussion, it is doable and there any many ways to implement it, from naively generating the info in the linker (see prototype CL 457455) to a detailed stack frame representation generated by the compiler and the linker.

If this proposal is accepted, I would suggest implementing it incrementally, starting by just enabling stack walking and finishing with an accurate representation of the non-volatile registries at every frame of the call stack.

@golang/windows @golang/compiler

@ianlancetaylor
Copy link
Contributor

I don't see any API changes here, so I don't think this needs to go through the proposal process. I think the runtime team and the Windows team can make a decision here. Thanks.

@ianlancetaylor ianlancetaylor changed the title proposal: support SEH stack unwinding on Windows runtime: support SEH stack unwinding on Windows Dec 14, 2022
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Dec 14, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 14, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Dec 14, 2022
@ianlancetaylor ianlancetaylor added compiler/runtime Issues related to the Go compiler and/or runtime. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Dec 14, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/runtime @golang/windows

@qmuntal
Copy link
Contributor Author

qmuntal commented Dec 14, 2022

I don't see any API changes here, so I don't think this needs to go through the proposal process. I think the runtime team and the Windows team can make a decision here. Thanks.

Makes sense.

Side note: I open to lead this implementation as part of my job at Microsoft, I don't expect nor ask the Go compiler/runtime team to jump into this by their own.

@alexbrainman
Copy link
Member

I think we should accept these changes.

@qmuntal it would help if you create new issues demonstrating problems you intend to fix. You mention #49471 , but I suspect there are others. Broken WinDbg or Windows Performance Analyzer or other tools are OK for these issues. It would help Go team to make this decision once they see what they are getting for adding new code.

Thank you.

Alex

@qmuntal
Copy link
Contributor Author

qmuntal commented Dec 20, 2022

@qmuntal it would help if you create new issues demonstrating problems you intend to fix. You mention #49471 , but I suspect there are others. Broken WinDbg or Windows Performance Analyzer or other tools are OK for these issues. It would help Go team to make this decision once they see what they are getting for adding new code.

That's a very good advice. Will do that!

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/461736 mentions this issue: cmd/internal/objabi,runtime: record NoFrame func flag

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/461737 mentions this issue: cmd/link,cmd/internal/objabi: support ADDR32NB relocations on windows

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/461738 mentions this issue: cmd/link: generate .pdata PE section

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/461749 mentions this issue: cmd/internal/obj: generate SEH aux symbols for windows/amd64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/457455 mentions this issue: cmd/link: generate .xdata PE section

gopherbot pushed a commit that referenced this issue Mar 29, 2023
This CL updates the linker to support
IMAGE_REL_[I386|AMD64|ARM|ARM64]_ADDR32NB relocations via the new
R_PEIMAGEOFF relocation type. This relocation type references symbols
using RVAs instead of VA, so it can use 4-byte offsets to reference
symbols that would normally require 8-byte offsets.

This new relocation is still not used, but will be useful when
generating Structured Exception Handling (SEH) metadata, which
needs to reference functions only using 4-byte addresses, thus
using RVAs instead of VA is of great help.

Updates #57302

Change-Id: I28d73e97d5cb78a3bc7194dc7d2fcb4a03f9f4d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/461737
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Davis Goodin <dagood@microsoft.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 5, 2023
This CL updates the Go compiler so it generate SEH unwind info [1] as a
function auxiliary symbol when building for windows/amd64.

A follow up CL will teach the Go linker how to assemble these codes
into the PE .xdata section.

Updates #57302

[1] https://learn.microsoft.com/en-us/cpp/build/exception-handling-x64#struct-unwind_info

Change-Id: I40ae0437bfee326c1a67c2b5e1496f0bf3ecea17
Reviewed-on: https://go-review.googlesource.com/c/go/+/461749
Reviewed-by: Davis Goodin <dagood@microsoft.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
@qmuntal
Copy link
Contributor Author

qmuntal commented Apr 20, 2023

1 month till freeze, but just 2 CLs for review remaining. Could you take a look to CL 461738 and CL 457455 @cherrymui and @thanm? Thanks!

gopherbot pushed a commit that referenced this issue May 2, 2023
This CL adds a .pdata section to the PE file generated by the Go linker.

The .pdata section is a standard section [1] that contains an array of
function table entries that are used for stack unwinding.
The table entries layout is taken from [2].

This CL just generates the table entries without any unwinding
information, which is enough to start doing some E2E tests
between the Go linker and the Win32 APIs.

The goal of the .pdata table is to allow Windows retrieve
unwind information for a function at a given PC. It does so by doing
a binary search on the table, looking for an entry that meets
BeginAddress >= PC < EndAddress.

Each table entry takes 12 bytes and only non-leaf functions with
frame pointer needs an entry on the .pdata table.
The result is that PE binaries will be ~0.7% bigger due to the unwind
information, a reasonable amount considering the benefits in
debuggability.

Updates #57302

[1] https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-pdata-section
[2] https://learn.microsoft.com/en-us/cpp/build/exception-handling-x64#struct-runtime_function

Change-Id: If675d10c64452946dbab76709da20569651e3e9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/461738
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue May 2, 2023
This CL adds a .xdata section to the PE file generated by the Go linker.
It is also the first CL of the SEH chain that adds effective support
for unwinding the Go stack, as demonstrated by the newly added tests.

The .xdata section is a standard PE section that contains
an array of unwind data info structures. This structures are used to
record the effects a function has on the stack pointer,
and where the nonvolatile registers are saved on the stack [1].

Note that this CL still does not support unwinding the cgo stack.

Updates #57302

[1] https://learn.microsoft.com/en-us/cpp/build/exception-handling-x64#struct-unwind_info

Change-Id: I6f305a51ed130b758ff9ca7b90c091e50a109a6f
Reviewed-on: https://go-review.googlesource.com/c/go/+/457455
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Davis Goodin <dagood@microsoft.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/499515 mentions this issue: doc: document WER and SEH changes in Go 1.21

gopherbot pushed a commit that referenced this issue May 31, 2023
While here, I've removed the CL 472195 TODO, which I marked as
RELNOTE=yes by mistake.

For #57441
For #57302

Change-Id: I7563140bf307f8d732f0154d02b8fa0735527323
Reviewed-on: https://go-review.googlesource.com/c/go/+/499515
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
@aclements
Copy link
Member

Thanks for all of this work @qmuntal ! Is there more to be done here, or can we close this issue at this point?

@qmuntal
Copy link
Contributor Author

qmuntal commented Jun 6, 2023

Thanks for all of this work @qmuntal ! Is there more to be done here, or can we close this issue at this point?

I haven't close it yet because SEH is still not implemented on windows/arm64, but I'm fine closing this bigger issue and open a more specific follow up issue. What do you recommend?

@mknyszek
Copy link
Contributor

mknyszek commented Jun 6, 2023

@qmuntal I think we just kick it to the next milestone and update the original post; possibly also the title.

@qmuntal
Copy link
Contributor Author

qmuntal commented Oct 11, 2023

While investigating #62887 I found out that, on windows/amd64 using internal linking, the Go linker doesn't merge .pdata/.xdata sections from host object files generated by the C compiler. This means that the stack can't be unwind in C -> Go transitions. I'm preparing a fix for that.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/534555 mentions this issue: cmd/internal/link: merge .pdata and .xdata sections from host object files

gopherbot pushed a commit that referenced this issue Nov 9, 2023
…files

The Go linker doesn't currently merge .pdata/.xdata sections from the
host object files generated by the C compiler when using internal
linking. This means that the stack can't be unwind in C -> Go.

This CL fixes that and adds a test to ensure that the stack can be
unwind in C -> Go and Go -> C transitions, which was not well tested.

Updates #57302

Change-Id: Ie86a5e6e30b80978277e66ccc2c48550e51263c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/534555
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
@cherrymui
Copy link
Member

Hi @qmuntal , what is the status for this now? Is there more to do? Should we bump to Go 1.23? Thanks.

@cherrymui
Copy link
Member

@qmuntal Also, is there anything that worth mentioning in the Go 1.22 release notes? Thanks.

@qmuntal
Copy link
Contributor Author

qmuntal commented Nov 28, 2023

Hi @qmuntal , what is the status for this now? Is there more to do? Should we bump to Go 1.23? Thanks.

windows/arm64 work still missing. I'll probably won't have bandwidth to implement it for Go 1.23, so we can move it to the backlog.

Also, is there anything that worth mentioning in the Go 1.22 release notes?

Yep, good point. CL 534555 and CL 525475, which should have linked this issue, deserve to be part of the release notes, as they can sublety alter program behavior (in a good way). I'll submit the corresponding release notes CL.

@cherrymui cherrymui modified the milestones: Go1.22, Backlog Nov 28, 2023
@thanm
Copy link
Contributor

thanm commented Dec 7, 2023

Friendly ping, just checking to see if you have had a chance to look into writing a release notes CL. Thanks.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/548575 mentions this issue: doc: document SEH changes

gopherbot pushed a commit that referenced this issue Dec 11, 2023
For #57302.
For #61422.

Change-Id: Iee4e6600bf473eb982d0cb7330f7b2c1b48b9e13
Reviewed-on: https://go-review.googlesource.com/c/go/+/548575
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
For golang#57302.
For golang#61422.

Change-Id: Iee4e6600bf473eb982d0cb7330f7b2c1b48b9e13
Reviewed-on: https://go-review.googlesource.com/c/go/+/548575
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. Debugging OS-Windows
Projects
Status: In Progress
Development

No branches or pull requests

8 participants