From d240226fe5aa94a41f1bdd481e71b88d2a987272 Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Fri, 14 Apr 2023 14:07:37 -0400 Subject: [PATCH] cmd/compile: rework marking of dead hidden closure functions 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 Reviewed-by: Cuong Manh Le Run-TryBot: Than McIntosh TryBot-Result: Gopher Robot --- src/cmd/compile/internal/inline/inl.go | 95 ++++++++++++++++---------- test/fixedbugs/issue59638.go | 65 ++++++++++++++++++ 2 files changed, 124 insertions(+), 36 deletions(-) create mode 100644 test/fixedbugs/issue59638.go diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go index 1a65e16f5199f..24812c8c0d67f 100644 --- a/src/cmd/compile/internal/inline/inl.go +++ b/src/cmd/compile/internal/inline/inl.go @@ -229,24 +229,71 @@ 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) - } - }) - } - }) + // 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() 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) { + liveFuncs[fn] = true + var vis func(node ir.Node) + vis = func(node ir.Node) { + if clo, ok := node.(*ir.ClosureExpr); ok { + if !liveFuncs[clo.Func] { + liveFuncs[clo.Func] = true + markLiveFuncs(clo.Func) + } + } + } + ir.Visit(fn, vis) + } + + 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 +940,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, edit) - 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 0000000000000..bba6265322c0e --- /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 +}