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: ICE when calling ABI0 function via func value #47317

Closed
cherrymui opened this issue Jul 21, 2021 · 11 comments
Closed

cmd/compile: ICE when calling ABI0 function via func value #47317

cherrymui opened this issue Jul 21, 2021 · 11 comments
Labels
Milestone

Comments

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jul 21, 2021

What version of Go are you using (go version)?

tip (49402be)

Does this issue reproduce with the latest release?

Reproducible with 1.17RC1
Not with 1.16.

What operating system and processor architecture are you using (go env)?

darwin/amd64

What did you do?

Compiling code that calls an ABI0 function using a func value, on a register-ABI platform.

x.go

package p

func F() interface{} {
	g := G
	g(1)
	return G
}

func G(x int) [2]int

a.s

TEXT	·G(SB),4,$0-24
	RET

What did you expect to see?

Build successfully.

What did you see instead?

./x.go:6:2: internal compiler error: offset for x at ./x.go:9:8 changed from 0 to 16

goroutine 8 [running]:
runtime/debug.Stack()
	/Users/cherryyz/src/go-tmp/src/runtime/debug/stack.go:24 +0x65
cmd/compile/internal/base.FatalfAt({0x262c0, 0xc0}, {0x19017a3, 0x29}, {0xc0005dd310, 0x4, 0x4})
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/base/print.go:227 +0x154
cmd/compile/internal/base.Fatalf(...)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/abi.(*ABIConfig).updateOffset(0x0, 0x251ffff, 0xc0003c0910, {0xc00037e380, {0x1a35260, 0xc0003bf110}, {0xc000026228, 0x1, 0x8}, 0x0}, ...)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/abi/abiutils.go:477 +0x298
cmd/compile/internal/abi.(*ABIConfig).ABIAnalyze(0xc0003bef70, 0x1a4b188, 0x70)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/abi/abiutils.go:441 +0x232
cmd/compile/internal/ssagen.(*state).call(0xc0005ce000, 0xc000176360, 0x0, 0x0)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/ssa.go:5012 +0x926
cmd/compile/internal/ssagen.(*state).callResult(0xc000176360, 0x0, 0x70)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/ssa.go:4915 +0x1b
cmd/compile/internal/ssagen.(*state).stmt(0xc0005ce000, {0x1a49d38, 0xc000176360})
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/ssa.go:1453 +0xf05
cmd/compile/internal/ssagen.(*state).stmtList(0xc0005ce000, {0xc0000ae500, 0x6, 0x1a2f620})
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/ssa.go:1414 +0x5d
cmd/compile/internal/ssagen.buildssa(0xc00013c2c0, 0x1)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/ssa.go:622 +0x1d33
cmd/compile/internal/ssagen.Compile(0xc00013c2c0, 0xc0005cdf90)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/pgen.go:165 +0x4c
cmd/compile/internal/gc.compileFunctions.func4.1(0x0)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/gc/compile.go:153 +0x3a
cmd/compile/internal/gc.compileFunctions.func3.1()
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/gc/compile.go:140 +0x4d
created by cmd/compile/internal/gc.compileFunctions.func3
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/gc/compile.go:138 +0x7f

In the case above, G is an ABI0 function. It is called via a func value (g). As far as I can tell, the ABI analysis for the call (g(1)) correctly uses ABIInternal (for ABI config), but the function's *Type being analyzed is the same *Type as the definition of ABI0 G. Perhaps the line g := G just makes g the same *Type as G.

@aclements @thanm @dr2chase what ensures that we always put the ABIInternal reference to a func value? What do we do with its *Type? The ABI analysis associates parameter/result offsets to the fields of function *Type. So if an ABI0 reference and an ABIInternal reference have the same *Type pointer, it blows up.

If I add an explicit type definition, var g func(int) [2]int = G, it compiles fine.

Originally found by @mdempsky and @cuonglm when investigating #47227. See also CL https://go-review.googlesource.com/c/go/+/334883 .

@cherrymui cherrymui added this to the Go1.17 milestone Jul 21, 2021
@thanm
Copy link
Contributor

@thanm thanm commented Jul 21, 2021

What's weird about this bug is that when I make a cursory look through the call-related code in ssagen and the back end, I don't actually see any places where we take a types.Field corresponding to a function parameter and look at its Offset field (instead the code is looking at the corresponding ABIParamAssignment object fo r the parameter. Do we really need to be setting f.Offset any more for these fields?

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 21, 2021

Do we really need to be setting f.Offset any more for these fields?

FWIW, I've been wanting to decouple parameter layout from types.Type/types.Field for a while, as they're not really type system details. If we can avoid touching f.Offset, I think that would be great. Nothing in the frontend should depend on f.Offset for parameters.

@cherrymui
Copy link
Contributor Author

@cherrymui cherrymui commented Jul 21, 2021

FWIW, I've been wanting to decouple parameter layout from types.Type/types.Field

I also think that's the right direction. If nothing depends on field.Offset, we could try not changing that. And have everything in the backend to use ABIParamAssignment.FrameOffset or something alike.

@thanm
Copy link
Contributor

@thanm thanm commented Jul 21, 2021

OK, I'll poke at this and see if I can prune away the ABI field offset update. I did find one suspicious use at

https://go.googlesource.com/go/+/3e48c0381fd1beb78e993e940c3b46ca9898ce6d/src/cmd/compile/internal/ssagen/ssa.go#5068

but I am pretty sure I can work around it.

@cherrymui
Copy link
Contributor Author

@cherrymui cherrymui commented Jul 21, 2021

That code is probably okay. It seems that is only for stack-allocated defer calls. If regabi is used, deferred function is argumentless, so it won't execute that line. Maybe you could condition it on GOEXPERIMENT not being enabled.

@thanm
Copy link
Contributor

@thanm thanm commented Jul 21, 2021

Yes, exactly.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 22, 2021

Change https://golang.org/cl/336629 mentions this issue: cmd/compile: do not change field offset in ABI analysis

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 22, 2021

Change https://golang.org/cl/336610 mentions this issue: [dev.typeparams] cmd/compile: remove outdate TODO in escape analysis

gopherbot pushed a commit that referenced this issue Jul 22, 2021
We now understand the root cause of #47227, it will be fixed in #47317.

Change-Id: Ifcd44f887a0bd3195818df33e409bd3e818e0b27
Reviewed-on: https://go-review.googlesource.com/c/go/+/336610
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot gopherbot closed this in 052da57 Jul 22, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 23, 2021

If nothing depends on field.Offset, we could try not changing that. And have everything in the backend to use ABIParamAssignment.FrameOffset or something alike.

Out of curiosity, where does 052da57 leave us w.r.t. this? Is f.Offset still needed by the backend for function parameters? It looks like maybe not?

@cherrymui
Copy link
Contributor Author

@cherrymui cherrymui commented Jul 23, 2021

Out of curiosity, where does 052da57 leave us w.r.t. this? Is f.Offset still needed by the backend for function parameters? It looks like maybe not?

It was used in exactly one place in the backend, which I changed to not use that. Now it should not be used.

On the dev.typeparams branch it should already be not used in the backend. The code I changed in the CL is already gone.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 23, 2021

Ah, great. Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants