code review 4634073: gc: Escape analysis.

gc: Escape analysis.

For now it's switch-on-and-offable with -s, and the effects can be inspected
with -m.  Defaults are the old codepaths.

[rsc, 2011-07-01 17:43:35]

I think I get it mostly, and it does seem a lot
simpler than I expected, so great.  I will take
a closer look when I'm back.

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

http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c#newcode1
src/cmd/gc/esc.c:1: // Copyright 2009 The Go Authors. All rights reserved.
new code, so 2011

http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c#newcode50
src/cmd/gc/esc.c:50: escanalfunc(Node *func)
sort of awkward.  i suggest ,s/escanal/esc/ or s//escape/

http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c#newcode95
src/cmd/gc/esc.c:95: // Assert that leaking dst will cause expr to leak as well,
s/expr/n/

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

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c#newcode18
src/cmd/gc/esc.c:18: //  an outer scope has it's reference implicitly taken.
it's == it is
you mean its
below too

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c#newcode43
src/cmd/gc/esc.c:43: //    - dst is a runtime.Hashmap<K,V> and src is assigned
to the key or value part of one of its elements
Please don't use C++ notation.  map[K]V,  chan T.

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/go.h#newcode902
src/cmd/gc/go.h:902: void	escanal();
escape?

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c#newcod...
src/cmd/gc/typecheck.c:329: // unless this is already inside an addr( & ...* ...
&...), address does not escape if there is a further one
I have no idea what this comment is saying.
Is it describing the next line?  If so:

// The *x in &*x is not an indirect.

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c#newcod...
src/cmd/gc/typecheck.c:528: // n->etype means this node was flagged for not
actually escaping
// top&Eindir means this is &x in *&x.
// n->etype means code generator flagged it as non-escaping.

[lvd, 2011-07-01 17:47:55]

On Fri, Jul 1, 2011 at 19:43,  <rsc@golang.org> wrote:
> I think I get it mostly, and it does seem a lot
> simpler than I expected, so great.  I will take
> a closer look when I'm back.
>

thanks.  i think close to patchset 10 i had a version that complied
the tree with no bugs and small wins, but currently its all messed up
again.   i'll upload a fresh snapshot.

>
>
> http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c
> File src/cmd/gc/esc.c (right):
>
> http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c#newcode1
> src/cmd/gc/esc.c:1: // Copyright 2009 The Go Authors. All rights
> reserved.
> new code, so 2011
>
> http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c#newcode50
> src/cmd/gc/esc.c:50: escanalfunc(Node *func)
> sort of awkward.  i suggest ,s/escanal/esc/ or s//escape/
>
> http://codereview.appspot.com/4634073/diff/5001/src/cmd/gc/esc.c#newcode95
> src/cmd/gc/esc.c:95: // Assert that leaking dst will cause expr to leak
> as well,
> s/expr/n/
>
> http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c
> File src/cmd/gc/esc.c (right):
>
> http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c#newcode18
> src/cmd/gc/esc.c:18: //  an outer scope has it's reference implicitly
> taken.
> it's == it is
> you mean its

yes. non-native speakers make that mistake a lot.

> below too
>
> http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c#newcode43
> src/cmd/gc/esc.c:43: //    - dst is a runtime.Hashmap<K,V> and src is
> assigned to the key or value part of one of its elements
> Please don't use C++ notation.  map[K]V,  chan T.
>
> http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/go.h
> File src/cmd/gc/go.h (right):
>
> http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/go.h#newcode902
> src/cmd/gc/go.h:902: void       escanal();
> escape?

hate to have confusion with escaping of strings ets.  escapanalyze(),
how's that.

> http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c
> File src/cmd/gc/typecheck.c (right):
>
>
http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c#newcod...
> src/cmd/gc/typecheck.c:329: // unless this is already inside an addr( &
> ...* ... &...), address does not escape if there is a further one
> I have no idea what this comment is saying.
> Is it describing the next line?  If so:
>
> // The *x in &*x is not an indirect.
>
>
http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c#newcod...
> src/cmd/gc/typecheck.c:528: // n->etype means this node was flagged for
> not actually escaping
> // top&Eindir means this is &x in *&x.
> // n->etype means code generator flagged it as non-escaping.
>
> http://codereview.appspot.com/4634073/
>

[rsc, 2011-07-01 17:49:47]

>> it's == it is
>> you mean its
>
> yes. non-native speakers make that mistake a lot.

native ones too.

> hate to have confusion with escaping of strings ets.  escapanalyze(),
> how's that.

escapes
?

[lvd, 2011-07-01 17:53:00]

On Fri, Jul 1, 2011 at 19:49, Russ Cox <rsc@golang.org> wrote:
>>> it's == it is
>>> you mean its
>>
>> yes. non-native speakers make that mistake a lot.
>
> native ones too.
>
>> hate to have confusion with escaping of strings ets.  escapanalyze(),
>> how's that.
>
> escapes

escapes it be.

> ?
>

[lvd, 2011-07-04 11:33:49]

still in progress but addressed what you commented so far

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

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c#newcode18
src/cmd/gc/esc.c:18: //  an outer scope has it's reference implicitly taken.
On 2011/07/01 17:43:35, rsc wrote:
> it's == it is
> you mean its
> below too
> 

Done.

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/esc.c#newcode43
src/cmd/gc/esc.c:43: //    - dst is a runtime.Hashmap<K,V> and src is assigned
to the key or value part of one of its elements
On 2011/07/01 17:43:35, rsc wrote:
> Please don't use C++ notation.  map[K]V,  chan T.

no, the map[K]V is a pointer to one of these things.  the distinction is
important.

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/go.h#newcode902
src/cmd/gc/go.h:902: void	escanal();
On 2011/07/01 17:43:35, rsc wrote:
> escape?

Done.

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c#newcod...
src/cmd/gc/typecheck.c:329: // unless this is already inside an addr( & ...* ...
&...), address does not escape if there is a further one
On 2011/07/01 17:43:35, rsc wrote:
> I have no idea what this comment is saying.
> Is it describing the next line?  If so:
> 
> // The *x in &*x is not an indirect.

Done.

http://codereview.appspot.com/4634073/diff/9022/src/cmd/gc/typecheck.c#newcod...
src/cmd/gc/typecheck.c:528: // n->etype means this node was flagged for not
actually escaping
On 2011/07/01 17:43:35, rsc wrote:
> // top&Eindir means this is &x in *&x.
> // n->etype means code generator flagged it as non-escaping.

Done.

[lvd, 2011-07-13 13:25:53]

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

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

[rsc, 2011-07-14 00:39:57]

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c
File src/cmd/gc/gen.c (right):

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode91
src/cmd/gc/gen.c:91: if (n == nodfp)
please ,s/if (/if(/g
in all files

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode121
src/cmd/gc/gen.c:121: n->xoffset = 0;
can delete

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode258
src/cmd/gc/go.h:258: Type*	paramfld; // TField for this PPARAM
TFIELD

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode273
src/cmd/gc/go.h:273: NodeList* asrc;		// flow(this, src)
comefrom?

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode905
src/cmd/gc/go.h:905: void	escapes();
(void)

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
src/cmd/gc/typecheck.c:527: // top&Eindir means this is &x in *&x.  (or the arg
to built-in print)
"or the arg to built-in print".  huh?  really?
that would be a mistake.

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
src/cmd/gc/typecheck.c:1603: if (!debug['s']) addrescapes(n->left);
2 lines

[lvd, 2011-07-15 14:16:26]

addressed your comments. 

trying the More Simple version in a separate client, pls hold off review for a
bit.

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c
File src/cmd/gc/gen.c (right):

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode91
src/cmd/gc/gen.c:91: if (n == nodfp)
On 2011/07/14 00:39:57, rsc wrote:
> please ,s/if (/if(/g
> in all files

Done.

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode121
src/cmd/gc/gen.c:121: n->xoffset = 0;
On 2011/07/14 00:39:57, rsc wrote:
> can delete

Done.

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode258
src/cmd/gc/go.h:258: Type*	paramfld; // TField for this PPARAM
On 2011/07/14 00:39:57, rsc wrote:
> TFIELD

Done.

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode273
src/cmd/gc/go.h:273: NodeList* asrc;		// flow(this, src)
On 2011/07/14 00:39:57, rsc wrote:
> comefrom?

sounds a bit intercal.  it was for 'Assignment SouRCe.  how about flowsrc?

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
src/cmd/gc/typecheck.c:527: // top&Eindir means this is &x in *&x.  (or the arg
to built-in print)
On 2011/07/14 00:39:57, rsc wrote:
> "or the arg to built-in print".  huh?  really?
> that would be a mistake.

no it makes sense, see line 1197: this is the way to say that arguments to
built-in print are safe pre esc.c, i.e. print(&x) should not force x to be
heapalloced.

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
src/cmd/gc/typecheck.c:1603: if (!debug['s']) addrescapes(n->left);
On 2011/07/14 00:39:57, rsc wrote:
> 2 lines

Done.

[rsc, 2011-07-15 15:47:30]

> no it makes sense, see line 1197: this is the way to say that arguments
> to built-in print are safe pre esc.c, i.e. print(&x) should not force x
> to be heapalloced.

oh.  that's an interesting change.

[rog, 2011-07-16 11:25:39]

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

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/esc.c#newcode20
src/cmd/gc/esc.c:20: //      reference implicitly taken.
I'm sure this is on the road map, but perhaps worth mentioning:

At some point in the future it would be nice if this was only the case when a
closed-over variable might be modified after the closure is created.

For instance, in this common pattern, n need not be heap allocated.

func f(n int) func() {
    return func() {
        for i := 0; i < n; i++ {
            something(i)
        }
     }
}

http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/esc.c#newcode103
src/cmd/gc/esc.c:103: //  of this excercise.
s/excercise/exercise/

[lvd, 2011-07-18 14:32:10]

I uploaded a new version of the 'complex' CL, which i think is actually
correct, even though it's still a bit more conservative at some points than
it needs to be, but at least it's correct in the sense that
bytes.Buffer.Bytes() is actually flagged as unsafe as it really is.

This is for the '-s' mode, not '-ss', which is supposed to do full alias
analysis.

I'm still looking to see if the 'simple' version you proposed is actually
going to end up being simpler, but before that i'm first adding a directory
test/esc and a facility to check the output of $(GC) -m for the cases in
there.

On Fri, Jul 15, 2011 at 16:16, <lvd@google.com> wrote:

> addressed your comments.
>
> trying the More Simple version in a separate client, pls hold off review
> for a bit.
>
>
>
>
http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**gen.c<http://...
> File src/cmd/gc/gen.c (right):
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
gen.c#newcode91<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode91>
> src/cmd/gc/gen.c:91: if (n == nodfp)
> On 2011/07/14 00:39:57, rsc wrote:
>
>> please ,s/if (/if(/g
>> in all files
>>
>
> Done.
>
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
gen.c#newcode121<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode121>
> src/cmd/gc/gen.c:121: n->xoffset = 0;
> On 2011/07/14 00:39:57, rsc wrote:
>
>> can delete
>>
>
> Done.
>
>
>
http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**go.h<http://c...
> File src/cmd/gc/go.h (right):
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
go.h#newcode258<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode258>
> src/cmd/gc/go.h:258: Type*      paramfld; // TField for this PPARAM
> On 2011/07/14 00:39:57, rsc wrote:
>
>> TFIELD
>>
>
> Done.
>
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
go.h#newcode273<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode273>
> src/cmd/gc/go.h:273: NodeList* asrc;            // flow(this, src)
> On 2011/07/14 00:39:57, rsc wrote:
>
>> comefrom?
>>
>
> sounds a bit intercal.  it was for 'Assignment SouRCe.  how about
> flowsrc?
>
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
typecheck.c<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c>
> File src/cmd/gc/typecheck.c (right):
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
typecheck.c#newcode527<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newcode527>
> src/cmd/gc/typecheck.c:527: // top&Eindir means this is &x in *&x.  (or
> the arg to built-in print)
> On 2011/07/14 00:39:57, rsc wrote:
>
>> "or the arg to built-in print".  huh?  really?
>> that would be a mistake.
>>
>
> no it makes sense, see line 1197: this is the way to say that arguments
> to built-in print are safe pre esc.c, i.e. print(&x) should not force x
> to be heapalloced.
>
>
> http://codereview.appspot.com/**4634073/diff/39009/src/cmd/gc/**
>
typecheck.c#newcode1603<http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newcode1603>
> src/cmd/gc/typecheck.c:1603: if (!debug['s']) addrescapes(n->left);
> On 2011/07/14 00:39:57, rsc wrote:
>
>> 2 lines
>>
>
> Done.
>
>
>
http://codereview.appspot.com/**4634073/<http://codereview.appspot.com/4634073/>
>

[rsc, 2011-07-19 14:15:12]

i will wait for the simple version.
it would be much better to write the test cases without -m.
otherwise they apply only to a single version of the
compiler and can't be shared, for example, with gccgo.
test/escape.go manages to be generic.

On Mon, Jul 18, 2011 at 10:32, Luuk van Dijk <lvd@google.com> wrote:
> I uploaded a new version of the 'complex' CL, which i think is actually
> correct, even though it's still a bit more conservative at some points than
> it needs to be, but at least it's correct in the sense that
> bytes.Buffer.Bytes() is actually flagged as unsafe as it really is.
> This is for the '-s' mode, not '-ss', which is supposed to do full alias
> analysis.
>
> I'm still looking to see if the 'simple' version you proposed is actually
> going to end up being simpler, but before that i'm first adding a directory
> test/esc and a facility to check the output of $(GC) -m for the cases in
> there.
> On Fri, Jul 15, 2011 at 16:16, <lvd@google.com> wrote:
>>
>> addressed your comments.
>>
>> trying the More Simple version in a separate client, pls hold off review
>> for a bit.
>>
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c
>> File src/cmd/gc/gen.c (right):
>>
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode91
>> src/cmd/gc/gen.c:91: if (n == nodfp)
>> On 2011/07/14 00:39:57, rsc wrote:
>>>
>>> please ,s/if (/if(/g
>>> in all files
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode121
>> src/cmd/gc/gen.c:121: n->xoffset = 0;
>> On 2011/07/14 00:39:57, rsc wrote:
>>>
>>> can delete
>>
>> Done.
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h
>> File src/cmd/gc/go.h (right):
>>
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode258
>> src/cmd/gc/go.h:258: Type*      paramfld; // TField for this PPARAM
>> On 2011/07/14 00:39:57, rsc wrote:
>>>
>>> TFIELD
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode273
>> src/cmd/gc/go.h:273: NodeList* asrc;            // flow(this, src)
>> On 2011/07/14 00:39:57, rsc wrote:
>>>
>>> comefrom?
>>
>> sounds a bit intercal.  it was for 'Assignment SouRCe.  how about
>> flowsrc?
>>
>> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c
>> File src/cmd/gc/typecheck.c (right):
>>
>>
>>
http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
>> src/cmd/gc/typecheck.c:527: // top&Eindir means this is &x in *&x.  (or
>> the arg to built-in print)
>> On 2011/07/14 00:39:57, rsc wrote:
>>>
>>> "or the arg to built-in print".  huh?  really?
>>> that would be a mistake.
>>
>> no it makes sense, see line 1197: this is the way to say that arguments
>> to built-in print are safe pre esc.c, i.e. print(&x) should not force x
>> to be heapalloced.
>>
>>
>>
http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
>> src/cmd/gc/typecheck.c:1603: if (!debug['s']) addrescapes(n->left);
>> On 2011/07/14 00:39:57, rsc wrote:
>>>
>>> 2 lines
>>
>> Done.
>>
>> http://codereview.appspot.com/4634073/
>
>

[lvd, 2011-07-19 14:33:28]

On Tue, Jul 19, 2011 at 16:15, Russ Cox <rsc@golang.org> wrote:

> i will wait for the simple version.
>

won't be /much/ simpler


> it would be much better to write the test cases without -m.
> otherwise they apply only to a single version of the
> compiler and can't be shared, for example, with gccgo.
> test/escape.go manages to be generic.
>

but not very specific and it makes the tests spun out over a lot of places.
 I'd like to be able to write a 3 line function to check that some basic
case behaves as expected, and check not just calls to escapes() but also the
tagging of the parameters with noflow.  I'm open to suggestions.

btw bytes.String() is now (correctly) safe, b/c OARRAYBYTESTR and friends
are actually safe.  uploaded a last patchset for that.


>
> On Mon, Jul 18, 2011 at 10:32, Luuk van Dijk <lvd@google.com> wrote:
> > I uploaded a new version of the 'complex' CL, which i think is actually
> > correct, even though it's still a bit more conservative at some points
> than
> > it needs to be, but at least it's correct in the sense that
> > bytes.Buffer.Bytes() is actually flagged as unsafe as it really is.
> > This is for the '-s' mode, not '-ss', which is supposed to do full alias
> > analysis.
> >
> > I'm still looking to see if the 'simple' version you proposed is actually
> > going to end up being simpler, but before that i'm first adding a
> directory
> > test/esc and a facility to check the output of $(GC) -m for the cases in
> > there.
> > On Fri, Jul 15, 2011 at 16:16, <lvd@google.com> wrote:
> >>
> >> addressed your comments.
> >>
> >> trying the More Simple version in a separate client, pls hold off review
> >> for a bit.
> >>
> >>
> >> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c
> >> File src/cmd/gc/gen.c (right):
> >>
> >>
> >>
> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode91
> >> src/cmd/gc/gen.c:91: if (n == nodfp)
> >> On 2011/07/14 00:39:57, rsc wrote:
> >>>
> >>> please ,s/if (/if(/g
> >>> in all files
> >>
> >> Done.
> >>
> >>
> >>
> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/gen.c#newcode121
> >> src/cmd/gc/gen.c:121: n->xoffset = 0;
> >> On 2011/07/14 00:39:57, rsc wrote:
> >>>
> >>> can delete
> >>
> >> Done.
> >>
> >> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h
> >> File src/cmd/gc/go.h (right):
> >>
> >>
> >>
> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode258
> >> src/cmd/gc/go.h:258: Type*      paramfld; // TField for this PPARAM
> >> On 2011/07/14 00:39:57, rsc wrote:
> >>>
> >>> TFIELD
> >>
> >> Done.
> >>
> >>
> >>
> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/go.h#newcode273
> >> src/cmd/gc/go.h:273: NodeList* asrc;            // flow(this, src)
> >> On 2011/07/14 00:39:57, rsc wrote:
> >>>
> >>> comefrom?
> >>
> >> sounds a bit intercal.  it was for 'Assignment SouRCe.  how about
> >> flowsrc?
> >>
> >> http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c
> >> File src/cmd/gc/typecheck.c (right):
> >>
> >>
> >>
>
http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
> >> src/cmd/gc/typecheck.c:527: // top&Eindir means this is &x in *&x.  (or
> >> the arg to built-in print)
> >> On 2011/07/14 00:39:57, rsc wrote:
> >>>
> >>> "or the arg to built-in print".  huh?  really?
> >>> that would be a mistake.
> >>
> >> no it makes sense, see line 1197: this is the way to say that arguments
> >> to built-in print are safe pre esc.c, i.e. print(&x) should not force x
> >> to be heapalloced.
> >>
> >>
> >>
>
http://codereview.appspot.com/4634073/diff/39009/src/cmd/gc/typecheck.c#newco...
> >> src/cmd/gc/typecheck.c:1603: if (!debug['s']) addrescapes(n->left);
> >> On 2011/07/14 00:39:57, rsc wrote:
> >>>
> >>> 2 lines
> >>
> >> Done.
> >>
> >> http://codereview.appspot.com/4634073/
> >
> >
>

[rsc, 2011-07-26 15:53:51]

notes from lvd:

so the basic simplification is:
anything on the lhs of an assignment that isnt a NAME becomes &theSink
and no more bother with derefdepth()
which was the number of '*''s in the type name basically
i did spend a couple of days trying to get that to work too
but there be dragons
although now i know where they are
i can think of a way around them
there are still tons of print statements conveniently commented out, esp if you
want to know what goes on in finding the function argument nodes/types
and i recommen drunning with -mmmmmm on a simple 3 line program to get a feel
the testsuite is a mess, i'll clean that up later
basically for every case bumped into i made a 4 line x.go

[rsc, 2011-08-02 20:24:18]

Still haven't read case OCALLFUNC and eawalk closely.

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

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode5
src/cmd/gc/esc.c:5: // Theory:
Please update this comment to be shorter and more correct.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode169
src/cmd/gc/esc.c:169: escapes()
(void)

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode183
src/cmd/gc/esc.c:183: Strlit* safetag = strlit("noflow");
variables at top please

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode186
src/cmd/gc/esc.c:186: for(l=xtop; l; l=l->next)
please insert all braces.
it's too hard to follow as is.
use of continue might help too:

if(l->n->op != ODCLFUNC)
    continue;

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode206
src/cmd/gc/esc.c:206: eafunc(Node *n) {
brace on next line

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode212
src/cmd/gc/esc.c:212: for(ll=curfn->dcl; ll; ll=ll->next)
braces

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode257
src/cmd/gc/esc.c:257: if(stmt->trecur)
Delete.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode261
src/cmd/gc/esc.c:261: setlineno(stmt);
Usually you do

lno = setlineno(stmt);
...
lineno = lno;

It's fine if this isn't in eastmt but it 
needs to be somewhere (eafunc?).

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode265
src/cmd/gc/esc.c:265: fatal("Missing typecheck.");
lowercase errors please

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode300
src/cmd/gc/esc.c:300: case  OBLOCK:
s/  / /

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode316
src/cmd/gc/esc.c:316: case TSTRING:
TSTRING can be different.
It never flows.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode357
src/cmd/gc/esc.c:357: eaexpr(stmt->ntest->left, stmt->ntest->right);
There is a different type switch variable for each case.
I don't see that reflected here.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode424
src/cmd/gc/esc.c:424: stmt->trecur = 0;
Delete.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode438
src/cmd/gc/esc.c:438: // stmt->left is a (pseud)ocall,
s/(pseud)o//

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode449
src/cmd/gc/esc.c:449: eaexpr(N, stmt->left);
add
eaexprlist(N, stmt->list);

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode496
src/cmd/gc/esc.c:496: break;
delete

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode504
src/cmd/gc/esc.c:504: break;
return

(actually merge with case ODOT)

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode507
src/cmd/gc/esc.c:507: eaexpr(N, dst->right);
eaexpr(dst->left, dst->right);  // map keeps key

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode522
src/cmd/gc/esc.c:522: setlineno(expr);
lno = ...

lineno = lno at end

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode532
src/cmd/gc/esc.c:532: if(expr->trecur)
Delete.

The "t" in trecur stands for type.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode549
src/cmd/gc/esc.c:549: flows(dst, expr);
should this be in the else case only?

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode550
src/cmd/gc/esc.c:550: // to the right of & there can be only
clearer comment:

// dst = &*expr or dst = &(*expr).f are same as dst = expr.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode557
src/cmd/gc/esc.c:557: eaexpr(dst, expr->left);
shouldn't this be expr->left->left?

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode565
src/cmd/gc/esc.c:565: eaexpr(N, expr->left);
This implies that *x never flows out.
That seems wrong.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode573
src/cmd/gc/esc.c:573: if(dst)
delete

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode600
src/cmd/gc/esc.c:600: case ONOT:	case OCOM:
match other code (1 per line)

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode607
src/cmd/gc/esc.c:607: case OADD:	case OAND:	case OANDAND:
match other code (1 per line)

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode620
src/cmd/gc/esc.c:620: // array/string conversions must make copies to comply
with the spec
s/must make copies.*/make copies/

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode737
src/cmd/gc/esc.c:737: eaexpr(&theSink , ll->n);
s/ ,/,/

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode744
src/cmd/gc/esc.c:744: case ODOTPTR:
It doesn't seem right that ODOTPTR is different from OIND.
Merge ODOTPTR with OIND case above?

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode745
src/cmd/gc/esc.c:745: // expr->right is just an (untypechecked) ONAME
// expr->right is the field/method name.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode755
src/cmd/gc/esc.c:755: eaexpr(N, expr->right->left);  // expr->rigth is the OKEY
s/rigth/right/

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode772
src/cmd/gc/esc.c:772: 
delete blank line

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode776
src/cmd/gc/esc.c:776: // the closure flows to the lhs of the assignment, or to
theSink if we're in a return.
the part about the return is weird to say.
that applies to everything, not just closures.
i'd just delete this comment.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode879
src/cmd/gc/esc.c:879: derefdepth(Type* t)
Delete.  Call haspointers(t) instead.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/go.h#newcode905
src/cmd/gc/go.h:905: void	escapes();
(void)

[rsc, 2011-08-02 21:15:45]

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

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode669
src/cmd/gc/esc.c:669: fn = fn->sym->def;       // resolve to definition if we
have it
Why wouldn't we have it?

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode690
src/cmd/gc/esc.c:690: if(lr->n->left && !lr->n->left->isddd) lr=lr->next;
2 lines
Also not sure ddd handling is correct.
It would be very nice if the ddd slice
could be noted not to escape.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode709
src/cmd/gc/esc.c:709: if(!t->note || strcmp(t->note->s, "noflow"))
strcmp != 0

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode720
src/cmd/gc/esc.c:720: eaexpr(&theSink , ll->n);
s/ ,/,/

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode722
src/cmd/gc/esc.c:722: eaexpr(N , ll->n);
s/ ,/,/

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode723
src/cmd/gc/esc.c:723: if(t->down) t=t->down;
2 lines

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode725
src/cmd/gc/esc.c:725: 
delete

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode733
src/cmd/gc/esc.c:733: // fallthrough if we don't have enough information
How can that happen?  You should always have a type.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode849
src/cmd/gc/esc.c:849: sink->noescape = 0xff;
seems like 3 would suffice

[lvd, 2011-08-05 13:53:32]

mostly done (except the case->1line).

added the tests in 1 file, test/escape2.  to investigate an individual one i
reccommend cutting and pasting one into zz.go and 6g -smmm that. 

there are 4 tests failing, all related to DOTPTR as a source confusing the field
with the struct.   i'll go grab a tgif beer and stare at that.

note that the math/unsafe.go functions (also in the test) passed because they
are uninteresting b/c they'r enot of pointer value.  however the real test is in
cases like escape2.go:inaddr3

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

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode5
src/cmd/gc/esc.c:5: // Theory:
On 2011/08/02 20:24:18, rsc wrote:
> Please update this comment to be shorter and more correct.

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode169
src/cmd/gc/esc.c:169: escapes()
On 2011/08/02 20:24:18, rsc wrote:
> (void)

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode183
src/cmd/gc/esc.c:183: Strlit* safetag = strlit("noflow");
On 2011/08/02 20:24:18, rsc wrote:
> variables at top please

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode186
src/cmd/gc/esc.c:186: for(l=xtop; l; l=l->next)
On 2011/08/02 20:24:18, rsc wrote:
> please insert all braces.
> it's too hard to follow as is.
> use of continue might help too:
> 
> if(l->n->op != ODCLFUNC)
>     continue;

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode206
src/cmd/gc/esc.c:206: eafunc(Node *n) {
On 2011/08/02 20:24:18, rsc wrote:
> brace on next line

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode212
src/cmd/gc/esc.c:212: for(ll=curfn->dcl; ll; ll=ll->next)
On 2011/08/02 20:24:18, rsc wrote:
> braces

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode257
src/cmd/gc/esc.c:257: if(stmt->trecur)
On 2011/08/02 20:24:18, rsc wrote:
> Delete.

replaced the return with a fatal, built the tree, no crashes. same in eawalk, so
done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode261
src/cmd/gc/esc.c:261: setlineno(stmt);
On 2011/08/02 20:24:18, rsc wrote:
> Usually you do
> 
> lno = setlineno(stmt);
> ...
> lineno = lno;
> 
> It's fine if this isn't in eastmt but it 
> needs to be somewhere (eafunc?).

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode265
src/cmd/gc/esc.c:265: fatal("Missing typecheck.");
On 2011/08/02 20:24:18, rsc wrote:
> lowercase errors please

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode300
src/cmd/gc/esc.c:300: case  OBLOCK:
On 2011/08/02 20:24:18, rsc wrote:
> s/  / /

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode316
src/cmd/gc/esc.c:316: case TSTRING:
On 2011/08/02 20:24:18, rsc wrote:
> TSTRING can be different.
> It never flows.

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode357
src/cmd/gc/esc.c:357: eaexpr(stmt->ntest->left, stmt->ntest->right);
On 2011/08/02 20:24:18, rsc wrote:
> There is a different type switch variable for each case.
> I don't see that reflected here.

fixed and added test for it.  now i know where those NONAMEs in the graph came
from.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode424
src/cmd/gc/esc.c:424: stmt->trecur = 0;
On 2011/08/02 20:24:18, rsc wrote:
> Delete.

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode438
src/cmd/gc/esc.c:438: // stmt->left is a (pseud)ocall,
On 2011/08/02 20:24:18, rsc wrote:
> s/(pseud)o//
in the grammar it's a pseudocall, distinct from a plain one.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode449
src/cmd/gc/esc.c:449: eaexpr(N, stmt->left);
On 2011/08/02 20:24:18, rsc wrote:
> add
> eaexprlist(N, stmt->list);
there is no eaexprlist, but done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode496
src/cmd/gc/esc.c:496: break;
On 2011/08/02 20:24:18, rsc wrote:
> delete

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode504
src/cmd/gc/esc.c:504: break;
On 2011/08/02 20:24:18, rsc wrote:
> return
> 
> (actually merge with case ODOT)

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode507
src/cmd/gc/esc.c:507: eaexpr(N, dst->right);
On 2011/08/02 20:24:18, rsc wrote:
> eaexpr(dst->left, dst->right);  // map keeps key

actually &theSink, not dst->left, because maps are one of those implicit pointer
types we give up on

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode522
src/cmd/gc/esc.c:522: setlineno(expr);
On 2011/08/02 20:24:18, rsc wrote:
> lno = ...
> 
> lineno = lno at end

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode532
src/cmd/gc/esc.c:532: if(expr->trecur)
On 2011/08/02 20:24:18, rsc wrote:
> Delete.
> 
> The "t" in trecur stands for type.

i know, i just borrowed it to avoid introducing a new Node field.  but it's gone
now.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode549
src/cmd/gc/esc.c:549: flows(dst, expr);
On 2011/08/02 20:24:18, rsc wrote:
> should this be in the else case only?

no, the '&x' node must always be reached from dst so that we can tag x as
escaping

anyway, the way i cut this from the full aliasing version was a bit too sipmle
minded.  fixed by rethinking eawalk,  this also obsolets the three comments
below, one of which was correct.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode600
src/cmd/gc/esc.c:600: case ONOT:	case OCOM:
On 2011/08/02 20:24:18, rsc wrote:
> match other code (1 per line)

will do as very last step before checking in

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode620
src/cmd/gc/esc.c:620: // array/string conversions must make copies to comply
with the spec
On 2011/08/02 20:24:18, rsc wrote:
> s/must make copies.*/make copies/

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode669
src/cmd/gc/esc.c:669: fn = fn->sym->def;       // resolve to definition if we
have it
On 2011/08/02 21:15:45, rsc wrote:
> Why wouldn't we have it?

i lost track of all the cases, but i think this happens in cases like this:

func apply(f func(int i) int, val int) {
   return f(val)
}

it also matters if the function was imported or not

i can put back in a fatal() and find you an example

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode709
src/cmd/gc/esc.c:709: if(!t->note || strcmp(t->note->s, "noflow"))
On 2011/08/02 21:15:45, rsc wrote:
> strcmp != 0

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode720
src/cmd/gc/esc.c:720: eaexpr(&theSink , ll->n);
On 2011/08/02 21:15:45, rsc wrote:
> s/ ,/,/

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode722
src/cmd/gc/esc.c:722: eaexpr(N , ll->n);
On 2011/08/02 21:15:45, rsc wrote:
> s/ ,/,/

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode723
src/cmd/gc/esc.c:723: if(t->down) t=t->down;
On 2011/08/02 21:15:45, rsc wrote:
> 2 lines

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode737
src/cmd/gc/esc.c:737: eaexpr(&theSink , ll->n);
On 2011/08/02 20:24:18, rsc wrote:
> s/ ,/,/

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode745
src/cmd/gc/esc.c:745: // expr->right is just an (untypechecked) ONAME
On 2011/08/02 20:24:18, rsc wrote:
> // expr->right is the field/method name.

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode755
src/cmd/gc/esc.c:755: eaexpr(N, expr->right->left);  // expr->rigth is the OKEY
On 2011/08/02 20:24:18, rsc wrote:
> s/rigth/right/

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode772
src/cmd/gc/esc.c:772: 
On 2011/08/02 20:24:18, rsc wrote:
> delete blank line

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode776
src/cmd/gc/esc.c:776: // the closure flows to the lhs of the assignment, or to
theSink if we're in a return.
On 2011/08/02 20:24:18, rsc wrote:
> the part about the return is weird to say.
> that applies to everything, not just closures.
> i'd just delete this comment.

Done.

http://codereview.appspot.com/4634073/diff/63001/src/cmd/gc/esc.c#newcode879
src/cmd/gc/esc.c:879: derefdepth(Type* t)
On 2011/08/02 20:24:18, rsc wrote:
> Delete.  Call haspointers(t) instead.

Done.

[lvd, 2011-08-05 20:55:23]

Hello rsc@golang.org, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[lvd, 2011-08-22 14:06:01]

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

Please take another look.

[lvd, 2011-08-22 14:10:56]

PTAL.

at last a version again that builds the tree and passes all tests, including
the ones in escape2.go.

I did not merge the big switch, instead focussed on getting the
escflood/walk right, with he handling of loop scopes.  For that i needed to
trust the flow graph, which i did in 'my' version.

Once this is in, i'll be happy to re-try to simplify escstmt/escexpr.

/L

On Fri, Aug 5, 2011 at 22:55, <lvd@google.com> wrote:

> Hello rsc@golang.org, rogpeppe@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
>
http://codereview.appspot.com/**4634073/<http://codereview.appspot.com/4634073/>
>

[lvd, 2011-08-22 14:16:52]

On Mon, Aug 22, 2011 at 16:10, Luuk van Dijk <lvd@google.com> wrote:

> PTAL.
>
> at last a version again that builds the tree and passes all tests,
> including the ones in escape2.go.
>

woops. that was b/c i forgot to un-comment-out the switching on part.  hold
horses.


>
> I did not merge the big switch, instead focussed on getting the
> escflood/walk right, with he handling of loop scopes.  For that i needed to
> trust the flow graph, which i did in 'my' version.
>
> Once this is in, i'll be happy to re-try to simplify escstmt/escexpr.
>
> /L
>
>
> On Fri, Aug 5, 2011 at 22:55, <lvd@google.com> wrote:
>
>> Hello rsc@golang.org, rogpeppe@gmail.com (cc:
>> golang-dev@googlegroups.com),
>>
>> Please take another look.
>>
>>
>>
>>
http://codereview.appspot.com/**4634073/<http://codereview.appspot.com/4634073/>
>>
>
>

[lvd, 2011-08-24 11:16:08]

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

Please take another look.

[lvd, 2011-08-24 11:27:00]

>
>
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**4634073/<http://codereview.appspot.com/4634073/>
>

The closure variables are now handled way at the top again, by introducing
an edge from the closure to a fake addr of the cvars' target, mimicing
exactly the assignment walk is going to to later, and completely natural if
you think of a closure object as the struct that runtime.closure generates
for you.    that makes escwalk very simple now.

i regrouped the cases in both switches to common effects, and lumped all the
trivial ones into the default, and took out the diagnostic for 'unhandled
cases'.  that makes expr much shorter.

after much soulsearching and some attempts to get started i ended up  *not*
merge escstmt and escexpr, because
a) i think it's not subtle or complicated, or at least not more than your
unified one, but that could be because i looked at this for months now.
probably related, i didn't see a clear path of least resistance transforming
the code from this state to the other one without losing track of introduced
bugs.   I would be fine with checking this in and then you reorganizing it.
otoh, i dont think it solves a problem and i'd also be more comfortable i
think to work on it the way it is.

btw, i also ran make test in the old exp/go/eval, which caught more bugs
(namely all) than all of the rest of the tree (almost zero) while i worked
on removing the closure and paramref cases from escwalk.

i left the diagnostic edgecount and dstcount in but commented out.  useful
if you want to know why pkg/go/printer takes so long.

What would be a benchmark where we could see the effect of all this in Real
Life?

/L

[rsc, 2011-08-24 14:27:46]

LGTM after fixes below.

Let's submit this disabled for now.
I will send a CL for the recursion.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/dcl.c
File src/cmd/gc/dcl.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/dcl.c#newcode824
src/cmd/gc/dcl.c:824: if(funarg && n->left && n->left->class == PPARAM) {
drop { }

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

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode54
src/cmd/gc/esc.c:54: static int	floodgen;	// loop prevention in flood/walk
There's already a walkgen in go.h.
Please s/floodgen/walkgen/ in Node* and
use that one instead.  One is enough.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode68
src/cmd/gc/esc.c:68: safetag = strlit("noflow");
Please call this noescape.
Flow has too many meanings in a compiler,
and 'escape analysis' is a well-established term.
This is part of the object file format, so it will
appear in places other than this source code.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode134
src/cmd/gc/esc.c:134: escstmtlist(NodeList* stmts)
s/stmts/l/ (like all the other lists)

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode141
src/cmd/gc/esc.c:141: escstmt(Node *stmt)
s/stmt/n/ please
Will make the diffs smaller when I merge escstmt and escexpr.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode360
src/cmd/gc/esc.c:360: escexpr(Node *dst, Node *expr)
s/expr/n/ please
same reason

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode644
src/cmd/gc/esc.c:644: if (dst->flowsrc == nil) {
Please rename this field escflow or escflowsrc or escsomething.
I want it to be obvious which Node* fields go with esc.c.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode646
src/cmd/gc/esc.c:646: //		dstcount++;
These are basically free.  Fine to uncomment even
if they're not being printed.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/go.h#newcode158
src/cmd/gc/go.h:158: uchar	recur;		// reference depth (see esc.c)
Seems like an old comment.  Revert?
In fact I think the whole field is unused.  Delete?

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/go.h#newcode931
src/cmd/gc/go.h:931: void	escapes();
(void)

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/lex.c#newcode164
src/cmd/gc/lex.c:164: debug['s'] = 1;  // Scapeanalysis
Disable before submit.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/subr.c#newcode1123
src/cmd/gc/subr.c:1123: if (!c)
s/if (/if(/

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/subr.c#newcode1131
src/cmd/gc/subr.c:1131: if (n->loopdepth)
s/if (/if(/

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1608: if (!debug['s'])
s/if (/if(/

[rsc, 2011-08-24 15:39:43]

s/Escape/escape/ in the CL description

[lvd, 2011-08-24 16:51:40]

mostly done except the hotly disputed one :-)

testing one more time before submitting

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/dcl.c
File src/cmd/gc/dcl.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/dcl.c#newcode824
src/cmd/gc/dcl.c:824: if(funarg && n->left && n->left->class == PPARAM) {
On 2011/08/24 14:27:46, rsc wrote:
> drop { }

Done.

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

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode54
src/cmd/gc/esc.c:54: static int	floodgen;	// loop prevention in flood/walk
On 2011/08/24 14:27:46, rsc wrote:
> There's already a walkgen in go.h.
> Please s/floodgen/walkgen/ in Node* and
> use that one instead.  One is enough.

narrow interfaces, local changes, isolate state.... eg in debug printouts of the
graph flood it's nice to know the printed number hasnt been messed with by
whatever future change in typecheck or walk. basically the argument you have
below for naming the node fields escXXX

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode68
src/cmd/gc/esc.c:68: safetag = strlit("noflow");
On 2011/08/24 14:27:46, rsc wrote:
> Please call this noescape.
> Flow has too many meanings in a compiler,
> and 'escape analysis' is a well-established term.

i had to explain some people that what i was working on had nothing to do with
figuring out how many '\'s to insert into a c-string.  escape is probably more
overloaded than flow.  besides, i've thought of 'flow' as what is analyzed here,
and escape more specific as what must happen to an object when its address flows
out. but thats all nitpicking.

how bout i just tag it with the letter 's' for safe?  or 'l' for leaksnot, local
or leaf.  i kinda like 'leaksnot' now i see it written 

anyway, i changed it to "n" and made it such in escexprcall that it only needs
to be changed here

> This is part of the object file format, so it will
> appear in places other than this source code.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode644
src/cmd/gc/esc.c:644: if (dst->flowsrc == nil) {
On 2011/08/24 14:27:46, rsc wrote:
> Please rename this field escflow or escflowsrc or escsomething.
> I want it to be obvious which Node* fields go with esc.c.

Done.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/esc.c#newcode646
src/cmd/gc/esc.c:646: //		dstcount++;
On 2011/08/24 14:27:46, rsc wrote:
> These are basically free.  Fine to uncomment even
> if they're not being printed.

Done.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/go.h#newcode158
src/cmd/gc/go.h:158: uchar	recur;		// reference depth (see esc.c)
On 2011/08/24 14:27:46, rsc wrote:
> Seems like an old comment.  Revert?
> In fact I think the whole field is unused.  Delete?

Done.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/go.h#newcode931
src/cmd/gc/go.h:931: void	escapes();
On 2011/08/24 14:27:46, rsc wrote:
> (void)

Done.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/lex.c#newcode164
src/cmd/gc/lex.c:164: debug['s'] = 1;  // Scapeanalysis
On 2011/08/24 14:27:46, rsc wrote:
> Disable before submit.

you betcha! i took it out altogether just in case.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/subr.c#newcode1123
src/cmd/gc/subr.c:1123: if (!c)
On 2011/08/24 14:27:46, rsc wrote:
> s/if (/if(/

Done.

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/subr.c#newcode1131
src/cmd/gc/subr.c:1131: if (n->loopdepth)
On 2011/08/24 14:27:46, rsc wrote:
> s/if (/if(/

Done.

and all the others

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

http://codereview.appspot.com/4634073/diff/100013/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1608: if (!debug['s'])
On 2011/08/24 14:27:46, rsc wrote:
> s/if (/if(/

Done.

[lvd, 2011-08-24 17:07:17]

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

gc: Escape analysis.

For now it's switch-on-and-offable with -s, and the effects can be inspected
with -m.  Defaults are the old codepaths.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4634073

[kevlar, 2011-08-24 17:56:10]

I've been following this with barely-contained excitement.  Thanks for all
of the hard work, Luuk (and reviewers)!

On Wed, Aug 24, 2011 at 10:07 AM, <lvd@google.com> wrote:

> *** Submitted as
>
http://code.google.com/p/go/**source/detail?r=6ad8c55fbb30<http://code.google...
>
> gc: Escape analysis.
>
> For now it's switch-on-and-offable with -s, and the effects can be
> inspected
> with -m.  Defaults are the old codepaths.
>
> R=rsc
> CC=golang-dev
>
> http://codereview.appspot.com/**4634073<http://codereview.appspot.com/4634073>
>
>
>
http://codereview.appspot.com/**4634073/<http://codereview.appspot.com/4634073/>
>