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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Keith Randall <[email protected]>

Author: dr2chase

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.

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

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)
 		}
 	}
 

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
+	}
+}