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/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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@cherrymui
Copy link
Member

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 the NeedsFix The path to resolution is known, but the work has not been done. label Jul 21, 2021
@cherrymui cherrymui added this to the Go1.17 milestone Jul 21, 2021
@thanm
Copy link
Contributor

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
Contributor

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
Member Author

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 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
Member Author

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 commented Jul 21, 2021

Yes, exactly.

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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
@cuonglm
[dev.typeparams] cmd/compile: remove outdate TODO in escape analysis
73162a5 
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>
@mdempsky
Copy link
Contributor

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
Member Author

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
Contributor

Ah, great. Thanks for the clarification.

@golang golang locked and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@mdempsky @gopherbot @thanm @cherrymui