[release-branch.go1.3] cmd/gc: fix escape analysis of func returning indirect of parameter

««« CL 102040046 / a078b2056ebc
cmd/gc: fix escape analysis of func returning indirect of parameter

I introduced this bug when I changed the escape
analysis to run in phases based on call graph
dependency order, in order to be more precise about
inputs escaping back to outputs (functions returning
their arguments).

Given

        func f(z **int) *int { return *z }

we were tagging the function as 'z does not escape
and is not returned', which is all true, but not
enough information.

If used as:

        var x int
        p := &x
        q := &p
        leak(f(q))

then the compiler might try to keep x, p, and q all
on the stack, since (according to the recorded
information) nothing interesting ends up being
passed to leak.

In fact since f returns *q = p, &x is passed to leak
and x needs to be heap allocated.

To trigger the bug, you need a chain that the
compiler wants to keep on the stack (like x, p, q
above), and you need a function that returns an
indirect of its argument, and you need to pass the
head of the chain to that function. This doesn't
come up very often: this bug has been present since
June 2012 (between Go 1 and Go 1.1) and we haven't
seen it until now. It helps that most functions that
return indirects are getters that are simple enough
to be inlined, avoiding the bug.

Earlier versions of Go also had the benefit that if
&x really wasn't used beyond x's lifetime, nothing
broke if you put &x in a heap-allocated structure
accidentally. With the new stack copying, though,
heap-allocated structures containing &x are not
updated when the stack is copied and x moves,
leading to crashes in Go 1.3 that were not crashes
in Go 1.2 or Go 1.1.

The fix is in two parts.

First, in the analysis of a function, recognize when
a value obtained via indirect of a parameter ends up
being returned. Mark those parameters as having
content escape back to the return results (but we
don't bother to write down which result).

Second, when using the analysis to analyze, say,
f(q), mark parameters with content escaping as
having any indirections escape to the heap. (We
don't bother trying to match the content to the
return value.)

The fix could be less precise (simpler).
In the first part we might mark all content-escaping
parameters as plain escaping, and then the second
part could be dropped. Or we might assume that when
calling f(q) all the things pointed at by q escape
always (for any f and q).

The fix could also be more precise (more complex).
We might record the specific mapping from parameter
to result along with the number of indirects from the
parameter to the thing being returned as the result,
and then at the call sites we could set up exactly the
right graph for the called function. That would make
notleaks(f(q)) be able to keep x on the stack, because
the reuslt of f(q) isn't passed to anything that leaks it.

The less precise the fix, the more stack allocations
become heap allocations.

This fix is exactly as precise as it needs to be so that
none of the current stack allocations in the standard
library turn into heap allocations.

Fixes #8120.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews, khr, r
https://golang.org/cl/102040046
»»»

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://golang.org/cl/103870043

3 files changed, 67 insertions, 11 deletions

diff --git a/src/cmd/gc/esc.c b/src/cmd/gc/esc.c
index 4091682485..a75517813d 100644
--- a/src/cmd/gc/esc.c
+++ b/src/cmd/gc/esc.c
@@ -204,6 +204,13 @@ struct EscState {
 	// flow to.
 	Node	theSink;
 	
+	// If an analyzed function is recorded to return
+	// pieces obtained via indirection from a parameter,
+	// and later there is a call f(x) to that function,
+	// we create a link funcParam <- x to record that fact.
+	// The funcParam node is handled specially in escflood.
+	Node	funcParam;	
+	
 	NodeList*	dsts;		// all dst nodes
 	int	loopdepth;	// for detecting nested loop scopes
 	int	pdepth;		// for debug printing in recursions.
@@ -269,7 +276,13 @@ analyze(NodeList *all, int recursive)
 	e->theSink.sym = lookup(".sink");
 	e->theSink.escloopdepth = -1;
 	e->recursive = recursive;
-
+	
+	e->funcParam.op = ONAME;
+	e->funcParam.orig = &e->funcParam;
+	e->funcParam.class = PAUTO;
+	e->funcParam.sym = lookup(".param");
+	e->funcParam.escloopdepth = 10000000;
+	
 	for(l=all; l; l=l->next)
 		if(l->n->op == ODCLFUNC)
 			l->n->esc = EscFuncPlanned;
@@ -822,12 +835,17 @@ escassignfromtag(EscState *e, Strlit *note, NodeList *dsts, Node *src)
 		escassign(e, &e->theSink, src);
 		return em;
 	}
-	
+
 	if(em == EscNone)
 		return em;
+	
+	// If content inside parameter (reached via indirection)
+	// escapes back to results, mark as such.
+	if(em & EscContentEscapes)
+		escassign(e, &e->funcParam, src);
 
 	em0 = em;
-	for(em >>= EscBits; em && dsts; em >>= 1, dsts=dsts->next)
+	for(em >>= EscReturnBits; em && dsts; em >>= 1, dsts=dsts->next)
 		if(em & 1)
 			escassign(e, dsts->n, src);
 
@@ -1090,19 +1108,30 @@ escwalk(EscState *e, int level, Node *dst, Node *src)
 
 	// Input parameter flowing to output parameter?
 	if(dst->op == ONAME && dst->class == PPARAMOUT && dst->vargen <= 20) {
-		if(src->op == ONAME && src->class == PPARAM && level == 0 && src->curfn == dst->curfn) {
-			if(src->esc != EscScope && src->esc != EscHeap) {
+		if(src->op == ONAME && src->class == PPARAM && src->curfn == dst->curfn && src->esc != EscScope && src->esc != EscHeap) {
+			if(level == 0) {
 				if(debug['m'])
 					warnl(src->lineno, "leaking param: %hN to result %S", src, dst->sym);
 				if((src->esc&EscMask) != EscReturn)
 					src->esc = EscReturn;
-				src->esc |= 1<<((dst->vargen-1) + EscBits);
+				src->esc |= 1<<((dst->vargen-1) + EscReturnBits);
+				goto recurse;
+			} else if(level > 0) {
+				if(debug['m'])
+					warnl(src->lineno, "%N leaking param %hN content to result %S", src->curfn->nname, src, dst->sym);
+				if((src->esc&EscMask) != EscReturn)
+					src->esc = EscReturn;
+				src->esc |= EscContentEscapes;
 				goto recurse;
 			}
 		}
 	}
 
-	leaks = (level <= 0) && (dst->escloopdepth < src->escloopdepth);
+	// The second clause is for values pointed at by an object passed to a call
+	// that returns something reached via indirect from the object.
+	// We don't know which result it is or how many indirects, so we treat it as leaking.
+	leaks = level <= 0 && dst->escloopdepth < src->escloopdepth ||
+		level < 0 && dst == &e->funcParam && haspointers(src->type);
 
 	switch(src->op) {
 	case ONAME:
diff --git a/src/cmd/gc/go.h b/src/cmd/gc/go.h
index 3e24258639..413e71069d 100644
--- a/src/cmd/gc/go.h
+++ b/src/cmd/gc/go.h
@@ -236,8 +236,10 @@ enum
 	EscNone,
 	EscReturn,
 	EscNever,
-	EscBits = 4,
+	EscBits = 3,
 	EscMask = (1<<EscBits) - 1,
+	EscContentEscapes = 1<<EscBits, // value obtained by indirect of parameter escapes to some returned result
+	EscReturnBits = EscBits+1,
 };
 
 struct	Node
diff --git a/test/escape2.go b/test/escape2.go
index 382e8e6d64..8cb3b6df66 100644
--- a/test/escape2.go
+++ b/test/escape2.go
@@ -135,7 +135,7 @@ func (b *Bar) Leak() *int { // ERROR "leaking param: b"
 	return &b.i // ERROR "&b.i escapes to heap"
 }
 
-func (b *Bar) AlsoNoLeak() *int { // ERROR "b does not escape"
+func (b *Bar) AlsoNoLeak() *int { // ERROR "leaking param b content to result ~r0"
 	return b.ii
 }
 
@@ -149,7 +149,7 @@ func (b Bar) LeaksToo() *int { // ERROR "leaking param: b"
 	return b.ii
 }
 
-func (b *Bar) LeaksABit() *int { // ERROR "b does not escape"
+func (b *Bar) LeaksABit() *int { // ERROR "leaking param b content to result ~r0"
 	v := 0    // ERROR "moved to heap: v"
 	b.ii = &v // ERROR "&v escapes"
 	return b.ii
@@ -182,7 +182,7 @@ func (b *Bar2) Leak() []int { // ERROR "leaking param: b"
 	return b.i[:] // ERROR "b.i escapes to heap"
 }
 
-func (b *Bar2) AlsoNoLeak() []int { // ERROR "b does not escape"
+func (b *Bar2) AlsoNoLeak() []int { // ERROR "leaking param b content to result ~r0"
 	return b.ii[0:1]
 }
 
@@ -1443,3 +1443,28 @@ func bar151d() {
 	b := a[:]            // ERROR "a escapes to heap"
 	foo151(&b[4:8:8][0]) // ERROR "&b\[4:8:8\]\[0\] escapes to heap"
 }
+
+// issue 8120
+
+type U struct {
+	s *string
+}
+
+func (u *U) String() *string { // ERROR "leaking param u content to result ~r0"
+	return u.s
+}
+
+type V struct {
+	s *string
+}
+
+func NewV(u U) *V { // ERROR "leaking param: u"
+	return &V{u.String()} // ERROR "&V literal escapes to heap" "u does not escape"
+}
+
+func foo152() {
+	a := "a"   // ERROR "moved to heap: a"
+	u := U{&a} // ERROR "&a escapes to heap"
+	v := NewV(u)
+	println(v)
+}