cmd/compile: ignore OXXX nodes in closure captured vars list

Added a debug flag "-d closure" to explain compilation of
closures (should this be done some other way? Should we
rewrite the "-m" flag to "-d escapes"?)  Used this to
discover that cause was an OXXX node in the captured vars
list, and in turn noticed that OXXX nodes are explicitly
ignored in all other processing of captured variables.

Couldn't figure out a reproducer, did verify that this OXXX
was not caused by an unnamed return value (which is one use
of these).  Verified lack of heap allocation by examining -S
output.

Assembly:
(runtime/mgc.go:1371) PCDATA $0, $2
(runtime/mgc.go:1371) CALL "".notewakeup(SB)
(runtime/mgc.go:1377) LEAQ "".gcBgMarkWorker.func1·f(SB), AX
(runtime/mgc.go:1404) MOVQ AX, (SP)
(runtime/mgc.go:1404) MOVQ "".autotmp_2242+88(SP), CX
(runtime/mgc.go:1404) MOVQ CX, 8(SP)
(runtime/mgc.go:1404) LEAQ go.string."GC worker (idle)"(SB), AX
(runtime/mgc.go:1404) MOVQ AX, 16(SP)
(runtime/mgc.go:1404) MOVQ $16, 24(SP)
(runtime/mgc.go:1404) MOVB $20, 32(SP)
(runtime/mgc.go:1404) MOVQ $0, 40(SP)
(runtime/mgc.go:1404) PCDATA $0, $2
(runtime/mgc.go:1404) CALL "".gopark(SB)

Added a check for compiling_runtime to ensure that this is
caught in the future.  Added a test to test the check.
Verified that 1.5.3 did NOT reject the test case when
compiled with -+ flag, so this is not a recently added bug.

Cause of bug is two-part -- there was no leaking closure
detection ever, and instead it relied on capture-of-variables
to trigger compiling_runtime test, but closures improved in
1.5.3 so that mere capture of a value did not also capture
the variable, which thus allowed closures to escape, as well
as this case where the escape was spurious.  In
fixedbugs/issue14999.go, compare messages for f and g;
1.5.3 would reject g, but not f.  1.4 rejects both because
1.4 heap-allocates parameter x for both.

Fixes #14999.

Change-Id: I40bcdd27056810628e96763a44f2acddd503aee1
Reviewed-on: https://go-review.googlesource.com/21322
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/gc/closure.go b/src/cmd/compile/internal/gc/closure.go
index 1c5c5eb1a2ae5ac762db8e930981a15a8cd0b264..80c8d309afbb93d4bd9dbc0b67dec3dcba53f2ef 100644 (file)
--- a/src/cmd/compile/internal/gc/closure.go
+++ b/src/cmd/compile/internal/gc/closure.go
@@ -397,10 +397,42 @@ func transformclosure(xfunc *Node) {
        lineno = lno
 }
 
+// hasemptycvars returns true iff closure func_ has an
+// empty list of captured vars. OXXX nodes don't count.
+func hasemptycvars(func_ *Node) bool {
+       for _, v := range func_.Func.Cvars.Slice() {
+               if v.Op == OXXX {
+                       continue
+               }
+               return false
+       }
+       return true
+}
+
+// closuredebugruntimecheck applies boilerplate checks for debug flags
+// and compiling runtime
+func closuredebugruntimecheck(r *Node) {
+       if Debug_closure > 0 {
+               if r.Esc == EscHeap {
+                       Warnl(r.Lineno, "heap closure, captured vars = %v", r.Func.Cvars)
+               } else {
+                       Warnl(r.Lineno, "stack closure, captured vars = %v", r.Func.Cvars)
+               }
+       }
+       if compiling_runtime > 0 && r.Esc == EscHeap {
+               yyerrorl(r.Lineno, "heap-allocated closure, not allowed in runtime.")
+       }
+}
+
 func walkclosure(func_ *Node, init *Nodes) *Node {
        // If no closure vars, don't bother wrapping.
-       if len(func_.Func.Cvars.Slice()) == 0 {
+       if hasemptycvars(func_) {
+               if Debug_closure > 0 {
+                       Warnl(func_.Lineno, "closure converted to global")
+               }
                return func_.Func.Closure.Func.Nname
+       } else {
+               closuredebugruntimecheck(func_)
        }
 
        // Create closure in the form of a composite literal.

diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go
index fd18ae531213e80270cf5565877162670dc37805..a6eb1923103d4f68e1be2aac7e0ffc66b2426aaf 100644 (file)
--- a/src/cmd/compile/internal/gc/main.go
+++ b/src/cmd/compile/internal/gc/main.go
@@ -31,10 +31,11 @@ var (
 )
 
 var (
-       Debug_append int
-       Debug_panic  int
-       Debug_slice  int
-       Debug_wb     int
+       Debug_append  int
+       Debug_closure int
+       Debug_panic   int
+       Debug_slice   int
+       Debug_wb      int
 )
 
 // Debug arguments.
@@ -46,6 +47,7 @@ var debugtab = []struct {
        val  *int
 }{
        {"append", &Debug_append},         // print information about append compilation
+       {"closure", &Debug_closure},       // print information about closure compilation
        {"disablenil", &Disable_checknil}, // disable nil checks
        {"gcprog", &Debug_gcprog},         // print dump of GC programs
        {"nil", &Debug_checknil},          // print information about nil checks

diff --git a/src/cmd/compile/internal/gc/sinit.go b/src/cmd/compile/internal/gc/sinit.go
index e3bc46ac0690ef8c0db6deb27c3b1e6ba2106e07..5144a2526e4f460fd18f2d6cbe7909ae509694c1 100644 (file)
--- a/src/cmd/compile/internal/gc/sinit.go
+++ b/src/cmd/compile/internal/gc/sinit.go
@@ -479,12 +479,17 @@ func staticassign(l *Node, r *Node, out *[]*Node) bool {
                break
 
        case OCLOSURE:
-               if len(r.Func.Cvars.Slice()) == 0 {
+               if hasemptycvars(r) {
+                       if Debug_closure > 0 {
+                               Warnl(r.Lineno, "closure converted to global")
+                       }
                        // Closures with no captured variables are globals,
                        // so the assignment can be done at link time.
                        n := *l
                        gdata(&n, r.Func.Closure.Func.Nname, Widthptr)
                        return true
+               } else {
+                       closuredebugruntimecheck(r)
                }
        }
 

diff --git a/test/fixedbugs/issue14999.go b/test/fixedbugs/issue14999.go
new file mode 100644 (file)
index 0000000..c16d1ca
--- /dev/null
+++ b/test/fixedbugs/issue14999.go
@@ -0,0 +1,18 @@
+// errorcheck -+
+
+// Copyright 2016 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
+
+func f(x int) func(int) int {
+       return func(y int) int { return x + y } // ERROR "heap-allocated closure, not allowed in runtime."
+}
+
+func g(x int) func(int) int { // ERROR "x escapes to heap, not allowed in runtime."
+       return func(y int) int { // ERROR "heap-allocated closure, not allowed in runtime."
+               x += y
+               return x + y
+       }
+}