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.

Sign up for GitHub

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

Jump to bottom

cmd/cgo/internal/test: TestCallbackCallersSEH failures #65116

Open
qmuntal opened this issue Jan 16, 2024 · 6 comments
Open

cmd/cgo/internal/test: TestCallbackCallersSEH failures #65116

qmuntal opened this issue Jan 16, 2024 · 6 comments
Assignees
@qmuntal
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Go1.23

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Jan 16, 2024 •
edited

Go version

go version devel go1.22-e9b3ff15f4 Wed Jan 10 17:35:49 2024 +0000 windows/amd64

Output of go env in your module/workspace:

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\aaa\AppData\Local\go-build
set GOENV=C:\Users\aaa\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\aaa\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\aaa\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\aaa\code\golang-go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Users\aaa\code\golang-go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=devel go1.22-e9b3ff15f4 Wed Jan 10 17:35:49 2024 +0000
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\aaa\code\golang-go\src\cmd\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\aaa\AppData\Local\Temp\go-build998051253=/tmp/go-build -gno-record-gcc-switches

What did you do?

go test --ldflags=-linkmode=internal -run=TestCallbackCallersSEH ./cmd/cgo/internal/test

What did you see happen?

--- FAIL: TestCallbackCallersSEH (0.00s)
    callback_windows.go:107: incorrect backtrace:
        want:   [test._Cfunc_backtrace test.testCallbackCallersSEH.func1.1 test.testCallbackCallersSEH.func1 test.goCallback test._Cfunc_callback test.nestedCall.func1 test.nestedCall test.testCallbackCallersSEH test.TestCallbackCallersSEH]
        got:    []
FAIL
FAIL    cmd/cgo/internal/test   2.527s
FAIL

What did you expect to see?

=== RUN   TestCallbackCallersSEH
--- PASS: TestCallbackCallersSEH (0.00s)
PASS
ok      cmd/cgo/internal/test   2.286s

Some more context:

  • Only reproduces when using the Go internal linker.
  • The test fails only with certain gcc versions. I've reproduced the failure with:
>gcc --version
gcc (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
@qmuntal qmuntal added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 16, 2024
@qmuntal qmuntal added this to the Go1.22 milestone Jan 16, 2024
@qmuntal qmuntal self-assigned this Jan 16, 2024
@qmuntal
Copy link
Contributor Author

qmuntal commented Jan 16, 2024

I've tracked down the failure to an issue with how the .pdata entries are laid on disk. The docs says that these entries should be sorted using the function start address, but the Go linker not always honor this when copying the .pdata entries from the host objects, which makes the Windows runtime to find the wrong entry, therefore producing an incorrect unwinding.

Details of how the unsorted case happens: the Go linker concatenates .pdata sections from host objects without modifying them, relying on the host compiler to produce sorted entries (which they do). The host objects can contain unreachable function with their corresponding .pdata entries, which will eventually be replaced by the Go linker for runtime.unreachableMethod. As the unreachable function no longer exists, the Go linker also naively updates start address of the corresponding .pdata entry to runtime.unreachableMethod, when what it should do to keep the .pdata entries sorted is remove that entry. Unfortunately, this is not straightforward because .pdata sections from host objects are copied as a whole, not as individual entries.

It is already too late to fix this issue for go1.22, considering that the release is in ~3 weeks. We could disable cgo stack unwinding when using internal linking and reenable it for go1.23, but I would suggest to just skip TestCallbackCallersSEH as flaky until the issue is fixed. This feature has been implemented in go1.22, before it was not working at all, so having something that mostly works is better than nothing. Also, some unwinders, like WinDbg, can handle unsorted .pdata entries and show the correct call stack.

@golang/release @golang/windows

@qmuntal qmuntal mentioned this issue Jan 16, 2024
Open
@cherrymui
Copy link
Member

cherrymui commented Jan 17, 2024 •
edited

Thanks for the analysis.

The host objects can contain unreachable function with their corresponding .pdata entries, which will eventually be replaced by the Go linker for runtime.unreachableMethod.

Interesting. The intention of runtime.unreachableMethod was for Go functions. I didn't realize this could happen. One possibility is not rewriting the relocations in host objects, leaving it to point to, say, 0.

I agree that for the moment skipping the test is good. We could skip just in internal linking mode. I think cmd/dist (all.bat) passes internal build tag when running them in internal linking mode. Users generally don't explicitly use internal linking mode, so unlikely to be affected.

@gopherbot
Copy link

Change https://go.dev/cl/556635 mentions this issue: cmd/cgo/internal/test: skip TestCallbackCallersSEH when internal linking

gopherbot pushed a commit that referenced this issue Jan 23, 2024
@qmuntal
cmd/cgo/internal/test: skip TestCallbackCallersSEH when internal linking
adead1a 
TestCallbackCallersSEH is flaky when using the internal linker. Skip
it for now until the flakiness is resolved.

Updates #65116

Change-Id: I7628b07eaff8be00757d5604722f30aede25fce5
Reviewed-on: https://go-review.googlesource.com/c/go/+/556635
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/557815 mentions this issue: [release-branch.go1.22] cmd/cgo/internal/test: skip TestCallbackCallersSEH when internal linking

gopherbot pushed a commit that referenced this issue Jan 23, 2024
@qmuntal @cherrymui
[release-branch.go1.22] cmd/cgo/internal/test: skip TestCallbackCalle…
10e9ab5 
…rsSEH when internal linking

TestCallbackCallersSEH is flaky when using the internal linker. Skip
it for now until the flakiness is resolved.

Updates #65116

Change-Id: I7628b07eaff8be00757d5604722f30aede25fce5
Reviewed-on: https://go-review.googlesource.com/c/go/+/556635
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit adead1a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/557815
@gopherbot
Copy link

Closed by merging 10e9ab5 to release-branch.go1.22.

@cherrymui cherrymui modified the milestones: Go1.22, Go1.23 Jan 23, 2024
@cherrymui
Copy link
Member

The skip is submitted. Reopen for proper fix in Go 1.23.

@cherrymui cherrymui reopened this Jan 23, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@qmuntal @ezz-no
cmd/cgo/internal/test: skip TestCallbackCallersSEH when internal linking
373bc23 
TestCallbackCallersSEH is flaky when using the internal linker. Skip
it for now until the flakiness is resolved.

Updates golang#65116

Change-Id: I7628b07eaff8be00757d5604722f30aede25fce5
Reviewed-on: https://go-review.googlesource.com/c/go/+/556635
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@qmuntal qmuntal

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
Go Compiler / Runtime
Status: In Progress
Release Status & Release Blockers
Status: Done
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@qmuntal @gopherbot @cherrymui