cmd/compile: ICE 'order.stmt recover' building func with nested closures

[thanm]

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

$ go version
go version devel go1.21-e7b04a3e16 Thu Apr 13 23:15:09 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

No, only on tip.

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

linux/amd64

What did you do?

Build this code with "-gcflags=-l=4":

https://go.dev/play/p/G8xA8HfU2EH

Note that this has to be from a version of master just prior to 8854be4 (the revert of "cmd/compile: allow more inlining of functions that construct closures").

What did you expect to see?

Clean compile.

What did you see instead?

/usr/local/google/home/thanm/issue59404.go:28:28: internal compiler error: order.stmt recover

goroutine 1 [running]:
runtime/debug.Stack()
	/ssd2/go.master/src/runtime/debug/stack.go:24 +0x5e
cmd/compile/internal/base.FatalfAt({0xe9ffc0?, 0x0?}, {0xd4c298, 0xd}, {0xc0004995e8, 0x1, 0x1})
	/ssd2/go.master/src/cmd/compile/internal/base/print.go:234 +0x1d7
cmd/compile/internal/base.Fatalf(...)
	/ssd2/go.master/src/cmd/compile/internal/base/print.go:203
cmd/compile/internal/walk.(*orderState).stmt(0xc0004b5940, {0xe9ffc0?, 0xc0004a5d60?})
	/ssd2/go.master/src/cmd/compile/internal/walk/order.go:598 +0x1c1b
cmd/compile/internal/walk.(*orderState).stmtList(0xe9ffc0?, {0xc0004a6970, 0x1, 0x1})
	/ssd2/go.master/src/cmd/compile/internal/walk/order.go:366 +0x7a
cmd/compile/internal/walk.orderBlock(0xc0004abe50, 0xc0004bdb30)
	/ssd2/go.master/src/cmd/compile/internal/walk/order.go:455 +0x90
cmd/compile/internal/walk.order(0xc0004abe40)

Note that the code in question is a modified version of the test case for issue #59404.

[thanm]

With the test case in question, at the point where we start back end processing (order, walk, ssa, etc) here is the state of the world for the functions in the testcase in question.

In the blurb below, functions are shown with their nesting relationships, with the following additional flags:

"HIDDEN" is set for funcs where fn.IsHiddenClosure() returns true
"TRIV" means that ir.IsTrivialClosure() returns true
"DEAD" means that fn.IsDeadcodeClosure() returns true
"INL" means that the func was inlined
"HASLSYM" means that ir.initLSym() has already been called for the func
"ORPHAN" shows hidden closures not reachable from any top level func

 fn=Autodetect  [INL]
 | fn=Autodetect.func3  [HIDDEN] [TRIV]
 | | fn=Autodetect.func3.1  [HIDDEN]
 | | | fn=Autodetect.func3.1.1  [HIDDEN]
 | | fn=Autodetect.func3.2  [HIDDEN]
 | | | fn=Autodetect.func3.2.2  [HIDDEN]
 | | | fn=Autodetect.func3.2.1  [HIDDEN] [TRIV]
 fn=If.MonitoredResource  [INL]
 fn=If.Done 
 fn=Do 
 fn=aad 
 fn=Autodetect.func1.1  [HIDDEN] [ORPHAN] [INL] [HASLSYM] [DEAD]
 | fn=Autodetect.func1.1.1  [HIDDEN] [TRIV] [DEAD]
 | | fn=Autodetect.func1.1.1.1  [HIDDEN] [DEAD]
 | | fn=Autodetect.func1.1.1.2  [HIDDEN] [DEAD]
 | | | fn=Autodetect.func1.1.1.2.1  [HIDDEN] [TRIV] [DEAD]
 fn=Autodetect.func1  [HIDDEN] [ORPHAN] [INL] [HASLSYM]
 | fn=Autodetect.func1.func2  [HIDDEN] [TRIV]
 | | fn=Autodetect.func1.func2.1  [HIDDEN]
 | | fn=Autodetect.func1.func2.2  [HIDDEN]
 | | | fn=Autodetect.func1.func2.2.1  [HIDDEN] [TRIV]
 fn=init 
 fn=Autodetect.func2  [HIDDEN] [ORPHAN] [INL] [HASLSYM]
 | fn=Autodetect.func2.1  [HIDDEN] [TRIV] [DEAD]
 | | fn=Autodetect.func2.1.1  [HIDDEN] [DEAD]
 | | fn=Autodetect.func2.1.2  [HIDDEN] [DEAD]
 | | | fn=Autodetect.func2.1.2.1  [HIDDEN] [TRIV] [DEAD]
 fn=Interface.Done 
 fn=Interface.MonitoredResource 
 fn=(*If).Done 
 fn=(*If).MonitoredResource

The problem functions here are the ones marked as ORPHAN; they aren't actually contributing anything, and they consume resources in the back end given that we will have to compile them.

The other problem with the orphans is that because escape analysis works by calling ir.VisitFuncsBottomUp (which will not visit a function marked as a hidden closure, on the assumption that it will be found while walking some other top-level function), they they won't get touched by the code in escape that rewrites defers, etc. This means that they will trigger errors when run through walk/order.

I think at this point it might be better to change the inliner to do what amounts to a garbage collection after the dust settles, to pick out the orphans above and mark them as dead (as opposed to trying to do walks while the inlining process is happening).

[cuonglm]

But the function that trigger the ICE is p.Autodetect.func1.func2.2.1, which is not a dead one.

[thanm]

True, not marked dead, but should be. It is nested (indirectly) inside Autodetect.func1, which is an orphan. No good reason why we need to keep it around.

[gopherbot]

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

[cuonglm]

The CL was reverted.

[thanm]

I spent a little while analyzing this. There is a bug in my original CL: I placed the call togarbageCollectUnreferencedHiddenClosures here in the body of InlineDecls. This works fine if there are no generics, but InlineDecls also gets called during generic function instantiation here, and which is much too early.

It took me forever to find this-- when I looked at the "-m=3 -W=3" debug output I could see that closure functions were being marked as dead, but there was no debugging trace output to that effect. This was because that second call to InlineDecls intentionally disables "-m" (so as to suppress duplicate messages). Very confusing.

I'll revive the CL with a better fix (move the garbageCollectUnreferencedHiddenClosures call up into InlinePackage.

[thanm]

Test case that will reproduce the problem (assuming CL rolled forward): https://go.dev/play/p/UAn0udzWLTM

[thanm]

I filed a separate issue #59680 for this, I'll use that for the testcase and to track the fix.

[gopherbot]

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