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

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

Comments

@bcmills
Copy link
Member

@bcmills 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

@thanm
Copy link
Contributor

@thanm 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?

Loading

@thanm
Copy link
Contributor

@thanm 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.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 9, 2021

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

Loading

@dr2chase
Copy link
Contributor

@dr2chase 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?

Loading

@dr2chase
Copy link
Contributor

@dr2chase 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.

Loading

@thanm
Copy link
Contributor

@thanm thanm commented Apr 9, 2021

Thanks.

Loading

@thanm
Copy link
Contributor

@thanm 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.

Loading

gopherbot pushed a commit that referenced this issue Apr 9, 2021
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

@gopherbot gopherbot commented Apr 9, 2021

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

Loading

gopherbot pushed a commit that referenced this issue Apr 9, 2021
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 label Apr 9, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Apr 15, 2021

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

Loading

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Apr 15, 2021

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2021

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

Loading

@gopherbot gopherbot closed this in 52df929 Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants