cmd/compile: rework marking of dead hidden closure functions

[This is a roll-forward of CL 484859, this time including a fix for
issue #59709. The call to do dead function marking was taking place in
the wrong spot, causing it to run more than once if generics were
instantiated.]

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.

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

src/cmd/compile/internal/inline/inl.go

@@ -178,6 +178,11 @@ func pgoInlineEpilogue(p *pgo.Profile, decls []ir.Node) {
 // InlinePackage finds functions that can be inlined and clones them before walk expands them.
 func InlinePackage(p *pgo.Profile) {
 	InlineDecls(p, typecheck.Target.Decls, true)
+
+	// Perform a garbage collection of hidden closures functions that
+	// are no longer reachable from top-level functions following
+	// inlining. See #59404 and #59638 for more context.
+	garbageCollectUnreferencedHiddenClosures()
 }
 
 // InlineDecls applies inlining to the given batch of declarations.
@@ -229,24 +234,64 @@ func InlineDecls(p *pgo.Profile, decls []ir.Node, doInline bool) {
 		}
 	})
 
-	// Rewalk post-inlining functions to check for closures that are
-	// still visible but were (over-agressively) marked as dead, and
-	// undo that marking here. See #59404 for more context.
-	ir.VisitFuncsBottomUp(decls, func(list []*ir.Func, recursive bool) {
-		for _, n := range list {
-			ir.Visit(n, func(node ir.Node) {
-				if clo, ok := node.(*ir.ClosureExpr); ok && clo.Func.IsHiddenClosure() {
-					clo.Func.SetIsDeadcodeClosure(false)
-				}
-			})
-		}
-	})
-
 	if p != nil {
 		pgoInlineEpilogue(p, decls)
 	}
 }
 
+// garbageCollectUnreferencedHiddenClosures makes a pass over all the
+// top-level (non-hidden-closure) functions looking for nested closure
+// functions that are reachable, then sweeps through the Target.Decls
+// list and marks any non-reachable hidden closure function as dead.
+// See issues #59404 and #59638 for more context.
+func garbageCollectUnreferencedHiddenClosures() {
+
+	liveFuncs := make(map[*ir.Func]bool)
+
+	var markLiveFuncs func(fn *ir.Func)
+	markLiveFuncs = func(fn *ir.Func) {
+		if liveFuncs[fn] {
+			return
+		}
+		liveFuncs[fn] = true
+		ir.Visit(fn, func(n ir.Node) {
+			if clo, ok := n.(*ir.ClosureExpr); ok {
+				markLiveFuncs(clo.Func)
+			}
+		})
+	}
+
+	for i := 0; i < len(typecheck.Target.Decls); i++ {
+		if fn, ok := typecheck.Target.Decls[i].(*ir.Func); ok {
+			if fn.IsHiddenClosure() {
+				continue
+			}
+			markLiveFuncs(fn)
+		}
+	}
+
+	for i := 0; i < len(typecheck.Target.Decls); i++ {
+		if fn, ok := typecheck.Target.Decls[i].(*ir.Func); ok {
+			if !fn.IsHiddenClosure() {
+				continue
+			}
+			if fn.IsDeadcodeClosure() {
+				continue
+			}
+			if liveFuncs[fn] {
+				continue
+			}
+			fn.SetIsDeadcodeClosure(true)
+			if base.Flag.LowerM > 2 {
+				fmt.Printf("%v: unreferenced closure %v marked as dead\n", ir.Line(fn), fn)
+			}
+			if fn.Inl != nil && fn.LSym == nil {
+				ir.InitLSym(fn, true)
+			}
+		}
+	}
+}
+
 // CanInline determines whether fn is inlineable.
 // If so, CanInline saves copies of fn.Body and fn.Dcl in fn.Inl.
 // fn and fn.Body will already have been typechecked.
@@ -893,30 +938,6 @@ func inlnode(n ir.Node, bigCaller bool, inlCalls *[]*ir.InlinedCallExpr, edit fu
 		}
 		if fn := inlCallee(call.X, profile); fn != nil && typecheck.HaveInlineBody(fn) {
 			n = mkinlcall(call, fn, bigCaller, inlCalls)
-			if fn.IsHiddenClosure() {
-				// Visit function to pick out any contained hidden
-				// closures to mark them as dead, since they will no
-				// longer be reachable (if we leave them live, they
-				// will get skipped during escape analysis, which
-				// could mean that go/defer statements don't get
-				// desugared, causing later problems in walk). See
-				// #59404 for more context. Note also that the code
-				// below can sometimes be too aggressive (marking a closure
-				// dead even though it was captured by a local var).
-				// In this case we'll undo the dead marking in a cleanup
-				// pass that happens at the end of InlineDecls.
-				var vis func(node ir.Node)
-				vis = func(node ir.Node) {
-					if clo, ok := node.(*ir.ClosureExpr); ok && clo.Func.IsHiddenClosure() && !clo.Func.IsDeadcodeClosure() {
-						if base.Flag.LowerM > 2 {
-							fmt.Printf("%v: closure %v marked as dead\n", ir.Line(clo.Func), clo.Func)
-						}
-						clo.Func.SetIsDeadcodeClosure(true)
-						ir.Visit(clo.Func, vis)
-					}
-				}
-				ir.Visit(fn, vis)
-			}
 		}
 	}
 

test/fixedbugs/issue59638.go

@@ -0,0 +1,65 @@
+// build -gcflags=-l=4
+
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+type Interface interface {
+	MonitoredResource() (resType string, labels map[string]string)
+	Done()
+}
+
+func Autodetect(x int) Interface {
+	return func() Interface {
+		func() Interface {
+			x++
+			Do(func() {
+				var ad, gd Interface
+
+				go func() {
+					defer gd.Done()
+					ad = aad()
+				}()
+				go func() {
+					defer ad.Done()
+					gd = aad()
+					defer func() { recover() }()
+				}()
+
+				autoDetected = ad
+				if gd != nil {
+					autoDetected = gd
+				}
+			})
+			return autoDetected
+		}()
+		return nil
+	}()
+}
+
+var autoDetected Interface
+var G int
+
+type If int
+
+func (x If) MonitoredResource() (resType string, labels map[string]string) {
+	return "", nil
+}
+
+//go:noinline
+func (x If) Done() {
+	G++
+}
+
+//go:noinline
+func Do(fn func()) {
+	fn()
+}
+
+//go:noinline
+func aad() Interface {
+	var x If
+	return x
+}

test/fixedbugs/issue59709.dir/aconfig.go

@@ -0,0 +1,10 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package aconfig
+
+type Config struct {
+	name string
+	blah int
+}

test/fixedbugs/issue59709.dir/bresource.go

@@ -0,0 +1,27 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package bresource
+
+type Resource[T any] struct {
+	name        string
+	initializer Initializer[T]
+	cfg         ResConfig
+	value       T
+}
+
+func Should[T any](r *Resource[T], e error) bool {
+	return r.cfg.ShouldRetry(e)
+}
+
+type ResConfig struct {
+	ShouldRetry func(error) bool
+	TearDown    func()
+}
+
+type Initializer[T any] func(*int) (T, error)
+
+func New[T any](name string, f Initializer[T], cfg ResConfig) *Resource[T] {
+	return &Resource[T]{name: name, initializer: f, cfg: cfg}
+}

test/fixedbugs/issue59709.dir/cmem.go

@@ -0,0 +1,37 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cmem
+
+import (
+	"./aconfig"
+	"./bresource"
+)
+
+type MemT *int
+
+var G int
+
+type memResource struct {
+	x *int
+}
+
+func (m *memResource) initialize(*int) (res *int, err error) {
+	return nil, nil
+}
+
+func (m *memResource) teardown() {
+}
+
+func NewResource(cfg *aconfig.Config) *bresource.Resource[*int] {
+	res := &memResource{
+		x: &G,
+	}
+
+	return bresource.New("Mem", res.initialize, bresource.ResConfig{
+		// We always would want to retry the Memcache initialization.
+		ShouldRetry: func(error) bool { return true },
+		TearDown:    res.teardown,
+	})
+}

test/fixedbugs/issue59709.dir/dcache.go

@@ -0,0 +1,39 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package dcache
+
+import (
+	"./aconfig"
+	"./bresource"
+	"./cmem"
+)
+
+type Module struct {
+	cfg  *aconfig.Config
+	err  error
+	last any
+}
+
+//go:noinline
+func TD() {
+}
+
+func (m *Module) Configure(x string) error {
+	if m.err != nil {
+		return m.err
+	}
+	res := cmem.NewResource(m.cfg)
+	m.last = res
+
+	return nil
+}
+
+func (m *Module) Blurb(x string, e error) bool {
+	res, ok := m.last.(*bresource.Resource[*int])
+	if !ok {
+		panic("bad")
+	}
+	return bresource.Should(res, e)
+}

test/fixedbugs/issue59709.dir/main.go

@@ -0,0 +1,17 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+	"./dcache"
+)
+
+func main() {
+	var m dcache.Module
+	m.Configure("x")
+	m.Configure("y")
+	var e error
+	m.Blurb("x", e)
+}

test/fixedbugs/issue59709.go

@@ -0,0 +1,7 @@
+// rundir
+
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package ignored