code review 102040046: cmd/gc: fix escape analysis of func returning indirect ...

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 issue 8120.

[rsc, 2014-06-03 05:50:05]

Hello golang-codereviews@googlegroups.com (cc: iant, khr, r),

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2014-06-03 13:45:45]

https://codereview.appspot.com/102040046/diff/40001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (right):

https://codereview.appspot.com/102040046/diff/40001/src/cmd/gc/esc.c#newcode844
src/cmd/gc/esc.c:844: if(em & EscContentEscapes)
I think this CL is missing a change to go.h.

[rsc, 2014-06-03 14:02:15]

added, thanks

[rsc, 2014-06-03 14:02:16]

Hello golang-codereviews@googlegroups.com, iant@golang.org (cc:
golang-codereviews@googlegroups.com, khr@golang.org, r@golang.org),

Please take another look.

[iant, 2014-06-03 14:14:44]

LGTM

[rsc, 2014-06-03 15:36:06]

*** Submitted as https://code.google.com/p/go/source/detail?r=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 issue 8120.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews, khr, r
https://codereview.appspot.com/102040046

[rsc, 2014-06-03 15:40:05]

release CL is 103870043

[gobot, 2014-06-03 15:51:21]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/b040a71ca367aa01be8ba101f5b5254aeefde4d8

[rsc, 2014-06-03 15:52:03]

On Tue, Jun 3, 2014 at 11:51 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the windows-386 builder.
> See http://build.golang.org/log/b040a71ca367aa01be8ba101f5b5254aeefde4d8
>
> https://codereview.appspot.com/102040046/
>

not real. network flake