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: ICE 'invalid defer call' due to CL 479095 #59404

Closed
thanm opened this issue Apr 3, 2023 · 6 comments
Closed

cmd/compile: ICE 'invalid defer call' due to CL 479095 #59404

thanm opened this issue Apr 3, 2023 · 6 comments
Assignees
@thanm
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@thanm
Copy link
Contributor

thanm commented Apr 3, 2023

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

$ go version
go version devel go1.21-8edcdddb23 Fri Mar 31 23:25:07 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

Only on tip.

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

linux/amd64

What did you do?

$ git clone https://github.com/census-ecosystem/opencensus-go-exporter-stackdriver.git
$ cd opencensus-go-exporter-stackdriver
$ git checkout v0.13.5
...
$ go test -race -short ./...

What did you expect to see?

Clean build of test

What did you see instead?

# contrib.go.opencensus.io/exporter/stackdriver/monitoredresource
monitoredresource/monitored_resources.go:56:5: internal compiler error: invalid defer call: (*sync.WaitGroup).Done(wg)

goroutine 1 [running]:
runtime/debug.Stack()
	/ssd2/xgo/src/runtime/debug/stack.go:24 +0x5e
cmd/compile/internal/base.FatalfAt({0xe9ace0?, 0x0?}, {0xd4d827, 0x13}, {0xc0005515a8, 0x2, 0x2})
	/ssd2/xgo/src/cmd/compile/internal/base/print.go:234 +0x1d7
cmd/compile/internal/walk.walkGoDefer(0xc000656780)
	/ssd2/xgo/src/cmd/compile/internal/walk/stmt.go:207 +0xba
cmd/compile/internal/walk.walkStmt({0xe9b5e0, 0xc000656780?})
	/ssd2/xgo/src/cmd/compile/internal/walk/stmt.go:123 +0x3c5
cmd/compile/internal/walk.walkStmtList(...)
	/ssd2/xgo/src/cmd/compile/internal/walk/stmt.go:175
cmd/compile/internal/walk.Walk(0xc000654dc0)
	/ssd2/xgo/src/cmd/compile/internal/walk/walk.go:43 +0x14d
cmd/compile/internal/gc.prepareFunc(0xc000654dc0)
	/ssd2/xgo/src/cmd/compile/internal/gc/compile.go:105 +0x107
cmd/compile/internal/gc.enqueueFunc(0xc00011f8c0)
	/ssd2/xgo/src/cmd/compile/internal/gc/compile.go:71 +0x30a
cmd/compile/internal/gc.Main(0xd75678)
	/ssd2/xgo/src/cmd/compile/internal/gc/main.go:363 +0x1b0a
main.main()
	/ssd2/xgo/src/cmd/compile/main.go:57 +0xf9

?   	contrib.go.opencensus.io/exporter/stackdriver/internal/testpb	[no test files]
FAIL	contrib.go.opencensus.io/exporter/stackdriver [build failed]

At first blush what appears to be happening is that the code in escape analysis that "normalizes" defers (converts them via wrapping into calls to no-args/no-results functions) is missing a defer. More work needed to find out exactly why.

Note that CL 479095 has since been reverted upstream, so the bug will no longer manifest directly on tip.
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 3, 2023
@thanm thanm added this to the Go1.21 milestone Apr 3, 2023
@thanm thanm self-assigned this Apr 3, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 3, 2023
@thanm thanm mentioned this issue Apr 3, 2023
Closed
@thanm
Copy link
Contributor Author

thanm commented Apr 3, 2023 •
edited

This appears to be a problem in escape analysis and/or ir.VisitFuncsBottomUp.

Here is a somewhat smaller example to repro: https://go.dev/play/p/UUFnYaFwv50

Here's what typecheck.Target.Decls looks like just before escape analysis if inlining is disabled:

 Autodetect 
 If.MonitoredResource
 aad 
 Autodetect.func1.1.1 hidden=true
 Autodetect.func1.1.2 hidden=true
 Autodetect.func1.1 hidden=true
 Autodetect.func1 hidden=true
 init 
 Interface.MonitoredResource
 (*If).MonitoredResource

Note that all of the closures have the "hidden closure" flag set, since they are all reachable from the top level routine Autodetect.

Now here's what the same list looks like with inlining turned on:

 ./repro.go:34:3: inlining call to Autodetect.func1
<autogenerated>:1: inlining call to If.MonitoredResource
Autodetect
 If.MonitoredResource
 aad
 Autodetect.func1.1.1 hidden=true
 Autodetect.func1.1.2 hidden=true
 Autodetect.func1.1 hidden=true
 Autodetect.func1 hidden=true
 init
 Autodetect.func2.1 hidden=true
 Autodetect.func2.2 hidden=true
 Autodetect.func2 hidden=true
 Interface.MonitoredResource
 (*If).MonitoredResource

What's happened here is that when Autodetect.func1 is inlined, we wind up duplicating the closure functions within it (this happens here).

Once the call to Autodetect.func1 is complete, the closures it contains are no longer reachable from any top level routine, but they are still marked with the "Hidden" flag. This means that they get skipped by ir.VisitFuncsBottomUp, due to this code here.

Because they are not exposed to escape analysis, the code that canonicalizes go/defer (here) never kicks in, but since the functions are still on the top level Decls list, they still get compiled, thus the assert in walk.

Seems to me the best way to handle this would be in escape analysis, I'll look into cooking up a CL of some sort for that.

@gopherbot

This comment was marked as duplicate.

Sign in to view
@gopherbot
Copy link

Change https://go.dev/cl/482355 mentions this issue: cmd/compile: deadcode unreferenced hidden closures in escape analysis

@gopherbot
Copy link

Change https://go.dev/cl/482715 mentions this issue: cmd/compile: mark unreference hidden closures dead

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 7, 2023
@thanm thanm mentioned this issue Apr 14, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/484859 mentions this issue: cmd/compile: rework marking of dead hidden closure functions

gopherbot pushed a commit that referenced this issue Apr 17, 2023
@thanm
cmd/compile: rework marking of dead hidden closure functions
d240226 
This patch generalizes the code in the inliner that marks unreferenced
hidden closure functions as dead. Rather than doing the marking on the
fly (previous approach), this new approach does a single pass at the
end of inlining, which catches more dead functions.

Fixes #59638.
Updates #59404.
Updates #59547.

Change-Id: I54fd63e9e37c9123b08a3e7def7d1989919bba91
Reviewed-on: https://go-review.googlesource.com/c/go/+/484859
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/486377 mentions this issue: cmd/compile: rework marking of dead hidden closure functions

@golang golang locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@thanm thanm

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
3 participants
@dmitshur @gopherbot @thanm