From 9a555fc24c318bf1b07bdc07d5c02e372681e401 Mon Sep 17 00:00:00 2001 From: Dan Scales Date: Thu, 25 Feb 2021 12:13:23 -0800 Subject: [PATCH] cmd/compile: fix missing descend in Addrtaken for closures. ComputeAddrtaken needs to descend into closures, now that imported bodies can include closures. The bug was that we weren't properly setting Addrtaken for a variable inside a closure inside a function that we were importing. For now, still disable inlining of functions with closures for -G mode. I'll enable it in a later change -- there are just a few fixes related to the fact that we don't need to set Ntype for closure functions. Added a test derived from the cilium repro in the issue. Fixes #44370 Change-Id: Ida2a403636bf8740b471b3ad68b5474951811e19 Reviewed-on: https://go-review.googlesource.com/c/go/+/296649 Run-TryBot: Dan Scales Trust: Dan Scales TryBot-Result: Go Bot Reviewed-by: Keith Randall Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/base/flag.go | 1 + src/cmd/compile/internal/inline/inl.go | 4 +--- src/cmd/compile/internal/typecheck/subr.go | 9 +++++++-- test/fixedbugs/issue44370.dir/a.go | 20 ++++++++++++++++++++ test/fixedbugs/issue44370.dir/b.go | 11 +++++++++++ test/fixedbugs/issue44370.go | 7 +++++++ 6 files changed, 47 insertions(+), 5 deletions(-) create mode 100644 test/fixedbugs/issue44370.dir/a.go create mode 100644 test/fixedbugs/issue44370.dir/b.go create mode 100644 test/fixedbugs/issue44370.go diff --git a/src/cmd/compile/internal/base/flag.go b/src/cmd/compile/internal/base/flag.go index d8ca9885cb4..ade17fc0cdc 100644 --- a/src/cmd/compile/internal/base/flag.go +++ b/src/cmd/compile/internal/base/flag.go @@ -159,6 +159,7 @@ func ParseFlags() { Flag.LinkShared = &Ctxt.Flag_linkshared Flag.Shared = &Ctxt.Flag_shared Flag.WB = true + Debug.InlFuncsWithClosures = 1 Flag.Cfg.ImportMap = make(map[string]string) diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go index 1703be74e97..e961b108448 100644 --- a/src/cmd/compile/internal/inline/inl.go +++ b/src/cmd/compile/internal/inline/inl.go @@ -354,9 +354,7 @@ func (v *hairyVisitor) doNode(n ir.Node) bool { return true case ir.OCLOSURE: - if base.Debug.InlFuncsWithClosures == 0 { - // TODO(danscales): change default of InlFuncsWithClosures - // to 1 when #44370 is fixed + if base.Debug.InlFuncsWithClosures == 0 || base.Flag.G > 0 { v.reason = "not inlining functions with closures" return true } diff --git a/src/cmd/compile/internal/typecheck/subr.go b/src/cmd/compile/internal/typecheck/subr.go index b88a9f22839..c40cfa2288b 100644 --- a/src/cmd/compile/internal/typecheck/subr.go +++ b/src/cmd/compile/internal/typecheck/subr.go @@ -106,7 +106,8 @@ var DirtyAddrtaken = false func ComputeAddrtaken(top []ir.Node) { for _, n := range top { - ir.Visit(n, func(n ir.Node) { + var doVisit func(n ir.Node) + doVisit = func(n ir.Node) { if n.Op() == ir.OADDR { if x := ir.OuterValue(n.(*ir.AddrExpr).X); x.Op() == ir.ONAME { x.Name().SetAddrtaken(true) @@ -117,7 +118,11 @@ func ComputeAddrtaken(top []ir.Node) { } } } - }) + if n.Op() == ir.OCLOSURE { + ir.VisitList(n.(*ir.ClosureExpr).Func.Body, doVisit) + } + } + ir.Visit(n, doVisit) } } diff --git a/test/fixedbugs/issue44370.dir/a.go b/test/fixedbugs/issue44370.dir/a.go new file mode 100644 index 00000000000..c5bf1bcbc72 --- /dev/null +++ b/test/fixedbugs/issue44370.dir/a.go @@ -0,0 +1,20 @@ +// Copyright 2021 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 a + +// A StoppableWaitGroup waits for a collection of goroutines to finish. +type StoppableWaitGroup struct { + // i is the internal counter which can store tolerate negative values + // as opposed the golang's library WaitGroup. + i *int64 +} + +// NewStoppableWaitGroup returns a new StoppableWaitGroup. When the 'Stop' is +// executed, following 'Add()' calls won't have any effect. +func NewStoppableWaitGroup() *StoppableWaitGroup { + return &StoppableWaitGroup{ + i: func() *int64 { i := int64(0); return &i }(), + } +} diff --git a/test/fixedbugs/issue44370.dir/b.go b/test/fixedbugs/issue44370.dir/b.go new file mode 100644 index 00000000000..f0e0b4e55fd --- /dev/null +++ b/test/fixedbugs/issue44370.dir/b.go @@ -0,0 +1,11 @@ +// Copyright 2021 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 b + +import "./a" + +func JoinClusterServices() { + _ = a.NewStoppableWaitGroup() +} diff --git a/test/fixedbugs/issue44370.go b/test/fixedbugs/issue44370.go new file mode 100644 index 00000000000..d4068385888 --- /dev/null +++ b/test/fixedbugs/issue44370.go @@ -0,0 +1,7 @@ +// compiledir + +// Copyright 2021 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