code review 4972056: gc: treat DOTMETH like DOT in escape analysis.

gc: treat DOTMETH like DOT in escape analysis.

Fixes issue 2225

[nigeltao, 2011-09-05 00:49:56]

s/XXX/2225/ in the CL description.

[dave_cheney.net, 2011-09-05 09:41:26]

LGTM, sample code passes on 5g, 6g and 8g.

[rsc, 2011-09-05 12:08:53]

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

http://codereview.appspot.com/4972056/diff/4001/src/cmd/gc/esc.c#newcode401
src/cmd/gc/esc.c:401: case ODOTMETH:
I think this is confusing, because it makes it look like
r.Read is an actual value when it isn't.  When I see code
like this I think "how can this happen?" and then 
"what? doesn't esccall already take care of this?"

I would rather see this handled where it happens and
where it isn't being taken care of, in esc's case OPROC.
That says

    escassign(&theSink, n->left->left);

and should say

    // receiver or function being called
    if(n->left->op == OCALLMETH || n->left->op == OCALLINTER)
        escassign(&theSink, n->left->left->left);
    else
        escassign(&theSink, n->left->left);

[lvd, 2011-09-05 13:48:52]

On Mon, Sep 5, 2011 at 14:08, <rsc@golang.org> wrote:

>
>
http://codereview.appspot.com/**4972056/diff/4001/src/cmd/gc/**esc.c<http://c...
> File src/cmd/gc/esc.c (right):
>
> http://codereview.appspot.com/**4972056/diff/4001/src/cmd/gc/**
>
esc.c#newcode401<http://codereview.appspot.com/4972056/diff/4001/src/cmd/gc/esc.c#newcode401>
> src/cmd/gc/esc.c:401: case ODOTMETH:
> I think this is confusing, because it makes it look like
> r.Read is an actual value when it isn't.


it is an actual value.  instead of a dotmeth it could be a plain function
that is the result of a computation.


>  When I see code
> like this I think "how can this happen?" and then
> "what? doesn't esccall already take care of this?"
>
>
funny, b/c i looked at the -mm output and saw that the sink->odotmeth is
there, but didnt make the next step, just like we do with struct members.


> I would rather see this handled where it happens and
> where it isn't being taken care of, in esc's case OPROC.
> That says
>
>    escassign(&theSink, n->left->left);
>
> and should say
>
>    // receiver or function being called
>    if(n->left->op == OCALLMETH || n->left->op == OCALLINTER)
>        escassign(&theSink, n->left->left->left);
>    else
>        escassign(&theSink, n->left->left);



i think that is horribly confusing and more bound to be wrong or introduce
other bugs.


>
>
>
http://codereview.appspot.com/**4972056/<http://codereview.appspot.com/4972056/>
>

[rsc, 2011-09-05 14:00:32]

On Mon, Sep 5, 2011 at 09:48, Luuk van Dijk <lvd@google.com> wrote:
> it is an actual value.

It is not.

$ cat x.go
package main

import "bytes"

func main() {
   var b bytes.Buffer
   _ = b.Read
}
$ 6g x.go
x.go:7: method b.(*Buffer).Read is not an expression, must be called
$

> instead of a dotmeth it could be a plain function
> that is the result of a computation.

That case is already handled correctly.
It is go x.m() that is the special case.

> i think that is horribly confusing and more bound to be wrong or introduce
> other bugs.

It is handling the receiver separately, just as the receiver
must be handled in esccall.

The receiver is always a special kind of function argument.

The only case that is mishandled here is go/defer x.m().
The fix should be in the handling of go/defer, not a
change to the general expression handling.

If someday x.m becomes a value - and we've talked about
doing that - then there shouldn't be code that looks like
it handles it when it does not.

Russ

[lvd, 2011-09-07 05:53:28]

+ all the cc's on the thread.

On Wed, Sep 7, 2011 at 07:51, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Mon, Sep 5, 2011 at 16:00, Russ Cox <rsc@golang.org> wrote:
>
>> On Mon, Sep 5, 2011 at 09:48, Luuk van Dijk <lvd@google.com> wrote:
>> > it is an actual value.
>>
>> It is not.
>>
>
> <...>
>
>
>>
>>
> If someday x.m becomes a value - and we've talked about
>> doing that - then there shouldn't be code that looks like
>> it handles it when it does not.
>>
>>
> Added a comment to clarify.
>
>
>> Russ
>>
>
>

[rsc, 2011-09-07 13:53:26]

LGTM

[lvd, 2011-09-07 13:56:18]

Hello rsc@golang.org, nigeltao@golang.org, dave@cheney.net (cc:
bradfitz@golang.org, golang-dev@googlegroups.com, mikioh.mikioh@gmail.com),

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

[lvd, 2011-09-07 17:03:18]

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

gc: treat DOTMETH like DOT in escape analysis.

Fixes issue 2225

R=rsc, nigeltao, dave
CC=bradfitz, golang-dev, mikioh.mikioh
http://codereview.appspot.com/4972056