diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go index d5bd1a50bf..1458f1a0e4 100644 --- a/src/cmd/compile/internal/inline/inl.go +++ b/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) - } } } diff --git a/test/fixedbugs/issue59638.go b/test/fixedbugs/issue59638.go new file mode 100644 index 0000000000..bba6265322 --- /dev/null +++ b/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 +} diff --git a/test/fixedbugs/issue59709.dir/aconfig.go b/test/fixedbugs/issue59709.dir/aconfig.go new file mode 100644 index 0000000000..01b3cf483b --- /dev/null +++ b/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 +} diff --git a/test/fixedbugs/issue59709.dir/bresource.go b/test/fixedbugs/issue59709.dir/bresource.go new file mode 100644 index 0000000000..9fae0994b0 --- /dev/null +++ b/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} +} diff --git a/test/fixedbugs/issue59709.dir/cmem.go b/test/fixedbugs/issue59709.dir/cmem.go new file mode 100644 index 0000000000..43c4fe9901 --- /dev/null +++ b/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, + }) +} diff --git a/test/fixedbugs/issue59709.dir/dcache.go b/test/fixedbugs/issue59709.dir/dcache.go new file mode 100644 index 0000000000..ea6321974e --- /dev/null +++ b/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) +} diff --git a/test/fixedbugs/issue59709.dir/main.go b/test/fixedbugs/issue59709.dir/main.go new file mode 100644 index 0000000000..c699a01fc1 --- /dev/null +++ b/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) +} diff --git a/test/fixedbugs/issue59709.go b/test/fixedbugs/issue59709.go new file mode 100644 index 0000000000..8fe8d8783f --- /dev/null +++ b/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