code review 4948041: gc: fix some spurious leaks

gc: fix some spurious leaks

Probably will spark some discussion.  ☺

[rsc, 2011-08-24 20:49:15]

Hello lvd (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-08-24 20:51:07]

The interesting cases are the new ones in
test/escape2.go that say '// does not leak'.
The code before this CL treated many of
them as leaking.

[lvd, 2011-08-24 20:54:17]

apart from the CL description you also seem to eliminate some things, presumably
b/c they're not necessary?  i don't understand why they work tho, see below.

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (left):

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#oldcode528
src/cmd/gc/esc.c:528: // Assignments to global variables get lumped into
theSink.
whoa this is much nicer to do here, isn't it?

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (right):

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#newcode182
src/cmd/gc/esc.c:182: // Everything but fixed array is a dereference.
so where is that fact recorded now?

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#newcode368
src/cmd/gc/esc.c:368: // Append returns first argument.
why are these gone?

[rsc, 2011-08-24 20:56:19]

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (left):

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#oldcode528
src/cmd/gc/esc.c:528: // Assignments to global variables get lumped into
theSink.
On 2011/08/24 20:54:17, lvd wrote:
> whoa this is much nicer to do here, isn't it?

All the other places that rewrite to dst = &theSink
are up above, and I was trying to figure out why 
global x = foo makes foo escape because it wasn't
up there with them, so I moved it up.

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (right):

http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#newcode182
src/cmd/gc/esc.c:182: // Everything but fixed array is a dereference.
On 2011/08/24 20:54:17, lvd wrote:
> so where is that fact recorded now?

It wasn't recorded before that I know of.

[lvd, 2011-08-25 08:07:40]

On Wed, Aug 24, 2011 at 22:56, <rsc@golang.org> wrote:

>
>
http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**esc.c<http://c...
> File src/cmd/gc/esc.c (left):
>
> http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**
>
esc.c#oldcode528<http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#oldcode528>
> src/cmd/gc/esc.c:528: // Assignments to global variables get lumped into
> theSink.
> On 2011/08/24 20:54:17, lvd wrote:
>
>> whoa this is much nicer to do here, isn't it?
>>
>
> All the other places that rewrite to dst = &theSink
> are up above, and I was trying to figure out why
> global x = foo makes foo escape because it wasn't
> up there with them, so I moved it up.


actually, it should work even if left  out altogether.  i wanted
node->loopdepth == 0 to mean 'uninitialized' to check some sanity in some
places, so then i made global scope  -1 function top-level +1.  but then i
was too lazy to find the proper place to initialize ->loopdepth to -1, and i
already had theSink, so thats how it became how it was.

but then i took al that sanity checking out, and the leaks test in walk will
work with global variables having 0.

maybe thats nicer.  i'll try it out later.


>
>
>
http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**esc.c<http://c...
> File src/cmd/gc/esc.c (right):
>
> http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**
>
esc.c#newcode182<http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#newcode182>
> src/cmd/gc/esc.c:182: // Everything but fixed array is a dereference.
> On 2011/08/24 20:54:17, lvd wrote:
>
>> so where is that fact recorded now?
>>
>
> It wasn't recorded before that I know of.
>
>
>
http://codereview.appspot.com/**4948041/<http://codereview.appspot.com/4948041/>
>

after this is in, shall i make a CL to reverse the meaning of -s?  (and take
the "NAME-" out of the -m messages, forgot about that) and then we should be
ready to switch it on, not?

[lvd, 2011-08-25 08:11:59]

LGTM by the way

On Thu, Aug 25, 2011 at 10:07, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Wed, Aug 24, 2011 at 22:56, <rsc@golang.org> wrote:
>
>>
>>
http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**esc.c<http://c...
>> File src/cmd/gc/esc.c (left):
>>
>> http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**
>>
esc.c#oldcode528<http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#oldcode528>
>> src/cmd/gc/esc.c:528: // Assignments to global variables get lumped into
>> theSink.
>> On 2011/08/24 20:54:17, lvd wrote:
>>
>>> whoa this is much nicer to do here, isn't it?
>>>
>>
>> All the other places that rewrite to dst = &theSink
>> are up above, and I was trying to figure out why
>> global x = foo makes foo escape because it wasn't
>> up there with them, so I moved it up.
>
>
> actually, it should work even if left  out altogether.  i wanted
> node->loopdepth == 0 to mean 'uninitialized' to check some sanity in some
> places, so then i made global scope  -1 function top-level +1.  but then i
> was too lazy to find the proper place to initialize ->loopdepth to -1, and i
> already had theSink, so thats how it became how it was.
>
> but then i took al that sanity checking out, and the leaks test in walk
> will work with global variables having 0.
>
> maybe thats nicer.  i'll try it out later.
>
>
>>
>>
>>
http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**esc.c<http://c...
>> File src/cmd/gc/esc.c (right):
>>
>> http://codereview.appspot.com/**4948041/diff/4001/src/cmd/gc/**
>>
esc.c#newcode182<http://codereview.appspot.com/4948041/diff/4001/src/cmd/gc/esc.c#newcode182>
>> src/cmd/gc/esc.c:182: // Everything but fixed array is a dereference.
>> On 2011/08/24 20:54:17, lvd wrote:
>>
>>> so where is that fact recorded now?
>>>
>>
>> It wasn't recorded before that I know of.
>>
>>
>>
http://codereview.appspot.com/**4948041/<http://codereview.appspot.com/4948041/>
>>
>
> after this is in, shall i make a CL to reverse the meaning of -s?  (and
> take the "NAME-" out of the -m messages, forgot about that) and then we
> should be ready to switch it on, not?
>
>
>

[rsc, 2011-08-25 13:19:53]

Thanks.

Re switching on debug['s'], I still have a few corner cases
I'd like to test.  Probably just one or two more rounds of this.

[rsc, 2011-08-25 13:26:24]

*** Submitted as http://code.google.com/p/go/source/detail?r=2ffcdee27931 ***

gc: fix some spurious leaks

Probably will spark some discussion.  ☺

R=lvd
CC=golang-dev
http://codereview.appspot.com/4948041