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

Jump to bottom

cmd/compile/internal/abi: "panic: Offset changed from 0 to 16" in test/abi/bad_select_crash.go on Windows #45465

Closed
bcmills opened this issue Apr 9, 2021 · 11 comments
Closed

cmd/compile/internal/abi: "panic: Offset changed from 0 to 16" in test/abi/bad_select_crash.go on Windows #45465

bcmills opened this issue Apr 9, 2021 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.17

Comments

@bcmills
Copy link
Contributor

bcmills commented Apr 9, 2021

https://build.golang.org/log/af0f875aa5ce2feb49d0fcf13d1c4643b51815e1:

##### ../test
# go run run.go -- abi/bad_select_crash.go
exit status 2
# syscall
panic: Offset changed from 0 to 16

goroutine 91 [running]:
cmd/compile/internal/abi.(*ABIConfig).updateOffset(0xc0000aa220, 0xc000456120, 0xc000ac2fa0, 0xc00002d580, 0xf041d0, 0xc0000c64e0, 0xc0006180d0, 0x1, 0x8, 0x0, ...)
	C:/workdir/go/src/cmd/compile/internal/abi/abiutils.go:475 +0x27d
cmd/compile/internal/abi.(*ABIConfig).ABIAnalyze(0xc0000aa220, 0xc000821400, 0xc0000aa200, 0xc0000aa220)
	C:/workdir/go/src/cmd/compile/internal/abi/abiutils.go:439 +0x287
cmd/compile/internal/ssagen.(*state).call(0xc00044a000, 0xc0003ab5f0, 0xc000610000, 0x0)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:4959 +0x9fe
cmd/compile/internal/ssagen.(*state).callResult(0xc00044a000, 0xc0003ab5f0, 0x0, 0x0)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:4862 +0x4a
cmd/compile/internal/ssagen.(*state).stmt(0xc00044a000, 0xf10c78, 0xc0003ab5f0)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:1425 +0x11ec
cmd/compile/internal/ssagen.(*state).stmtList(0xc00044a000, 0xc000092d50, 0x1, 0x1)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:1386 +0x79
cmd/compile/internal/ssagen.(*state).stmt(0xc00044a000, 0xf10ae8, 0xc0000a08c0)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:1404 +0x1c5
cmd/compile/internal/ssagen.(*state).stmtList(0xc00044a000, 0xc000500c00, 0x2b, 0x40)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:1386 +0x79
cmd/compile/internal/ssagen.buildssa(0xc000a1fa20, 0x3, 0x0)
	C:/workdir/go/src/cmd/compile/internal/ssagen/ssa.go:622 +0x229b
cmd/compile/internal/ssagen.Compile(0xc000a1fa20, 0x3)
	C:/workdir/go/src/cmd/compile/internal/ssagen/pgen.go:151 +0x66
cmd/compile/internal/gc.compileFunctions.func2.1(0xc000130000, 0xc000a1fa20, 0xc001211c00, 0xc000c9c8c0)
	C:/workdir/go/src/cmd/compile/internal/gc/compile.go:130 +0x66
created by cmd/compile/internal/gc.compileFunctions.func2
	C:/workdir/go/src/cmd/compile/internal/gc/compile.go:128 +0x95

FAIL	abi\bad_select_crash.go	3.315s
2021/04/09 04:43:00 Failed: exit status 1
go tool dist: FAILED

This test has been consistently failing on the windows-amd64-2016 and windows-amd64-longtest builders since CL 308309, which makes the Windows TryBots and SlowBots much less useful for testing other changes.

The regression may have been partially masked by an earlier longtest break at CL 307818 (fixed in CL 308469), and by #45456 (fixed in CL 308653).

CC @dr2chase @thanm @cherrymui @golang/release
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) labels Apr 9, 2021
@bcmills bcmills added this to the Go1.17 milestone Apr 9, 2021
@thanm
Copy link
Contributor

thanm commented Apr 9, 2021

A spent a little while this morning looking at this and I am mystified.

I can reproduce this on linux with

GOEXPERIMENT=regabi,regabiargs GOOS=windows go build syscall

The assert is taking place while compiling RegEnumKeyEx in zsyscall_windows.go:

func RegEnumKeyEx(key Handle, index uint32, name *uint16, nameLen *uint32, reserved *uint32, class *uint16, classLen *uint32, lastWriteTime *Filetime) (regerrno error) {
	r0, _, _ := Syscall9(procRegEnumKeyExW.Addr(), 8, uintptr(key), uintptr(index), uintptr(unsafe.Pointer(name)), uintptr(unsafe.Pointer(nameLen)), uintptr(unsafe.Pointer(reserved)), uintptr(unsafe.Pointer(class)), uintptr(unsafe.Pointer(classLen)), uintptr(unsafe.Pointer(lastWriteTime)), 0)
	if r0 != 0 {
		regerrno = Errno(r0)
	}
	return
}

and the call to ABIAnalyze that is crashing is on the signature for Syscall9. Here is the output of ABIAnalyzeFuncType for the func:

IN 0: R{ I0 } spilloffset: 0 typ: uintptr
IN 1: R{ I1 } spilloffset: 8 typ: uintptr
IN 2: R{ I2 } spilloffset: 16 typ: uintptr
IN 3: R{ I3 } spilloffset: 24 typ: uintptr
IN 4: R{ I4 } spilloffset: 32 typ: uintptr
IN 5: R{ I5 } spilloffset: 40 typ: uintptr
IN 6: R{ I6 } spilloffset: 48 typ: uintptr
IN 7: R{ I7 } spilloffset: 56 typ: uintptr
IN 8: R{ I8 } spilloffset: 64 typ: uintptr
IN 9: R{ } offset: 0 typ: uintptr
IN 10: R{ } offset: 8 typ: uintptr
OUT 0: R{ I0 } spilloffset: -1 typ: uintptr
OUT 1: R{ I1 } spilloffset: -1 typ: uintptr
OUT 2: R{ I2 } spilloffset: -1 typ: Errno
offsetToSpillArea: 16 spillAreaSize: 72

This looks fine. For the first parameter, updateOffset calls a.FrameOffset(), which correctly returns a value of 16; the existing setting of zero for f.Offset is incorrect.

The question is, how did that zero get there? It should be types.BOGUS_FUNARG_OFFSET, since the field in question is in a function signature and not in a struct.

It appears (at least from what I can see, maybe I am missing something) that something somewhere is updating f.Offset for field between the point in type when it is initialized (with types.BOGUS_FUNARG_OFFSET) and the point where we're looking at it in ABIAnalyze. I have no idea where that is happening, however.

@dr2chase maybe you have some idea what is happening?

@thanm
Copy link
Contributor

thanm commented Apr 9, 2021

I am going to disable the test for the time being just so that the builders can get on track.

@gopherbot
Copy link

Change https://golang.org/cl/308889 mentions this issue: test/abi: disable test on windows for now

@dr2chase
Copy link
Contributor

dr2chase commented Apr 9, 2021

Beat you to the disable. Seems like @mknyszek would have seen this in his Windows work already, but I'll eyeball it anyhow.
Maybe we're holding it wrong?

@dr2chase
Copy link
Contributor

dr2chase commented Apr 9, 2021

Ah, hypothesis is that we're analyzing the call twice with two different ABIs. That would be worth tracking down.

@thanm
Copy link
Contributor

thanm commented Apr 9, 2021

Thanks.

@thanm
Copy link
Contributor

thanm commented Apr 9, 2021

From the debugging code that I added, it looks like this is the first time ABIAnalyze is being called for the type in question. So at least on the surface it doesn't look as though it was previously analyzed under ABI0 , or something like that.

gopherbot pushed a commit that referenced this issue Apr 9, 2021
@dr2chase
test/abi: disable test on windows for now
d138ee2 
This tickles some other bug, do this to clear builders.

Updates #40724.
Updates #45465.

Change-Id: Id51efbcf474865da231fcbc6216e5d604f99c296
Reviewed-on: https://go-review.googlesource.com/c/go/+/308889
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/308934 mentions this issue: test/abi: disable test with old-style build tag known to run.go

gopherbot pushed a commit that referenced this issue Apr 9, 2021
@dr2chase
test/abi: disable test with old-style build tag known to run.go
d119680 
A quick check of the source to run.go suggests that it does not
look for the new-style build tags.

Updates #45465.

Change-Id: Ib4be040935d71e732f81d52c4a22c2b514195f40
Reviewed-on: https://go-review.googlesource.com/c/go/+/308934
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: David Chase <drchase@google.com>
@bcmills bcmills removed the Soon This needs to be done soon. (regressions, serious bugs, outages) label Apr 9, 2021
@toothrot
Copy link
Contributor

@dr2chase Any updates on this? It's marked as a release-blocker for Go 1.17.

@dr2chase
Copy link
Contributor

It's fixed, sorry:
https://go-review.googlesource.com/c/go/+/309110

@gopherbot
Copy link

Change https://golang.org/cl/310649 mentions this issue: test/abi: reenable test on windows

@golang golang locked and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
5 participants
@toothrot @dr2chase @bcmills @gopherbot @thanm