code review 5302082: gc: Don't pollute the xmethod list with non-methods.

gc: Don't pollute the xmethod list with non-methods.

Fixes issue 2355.

I have a test, but not sure if it's worth adding.  Instead i've made
the patching-over in reflect.c methods more fatal and more descriptive.

[lvd, 2011-11-03 14:09:15]

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

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

[lvd, 2011-11-03 14:23:35]

hold on a sec, getthis() != T is not a good test for being a method..

On Thu, Nov 3, 2011 at 15:09, <lvd@google.com> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> gc: Don't pollute the xmethod list with non-methods.
>
> Fixes issue 2355.
>
> I have a test, but not sure if it's worth adding.  Instead i've made
> the patching-over in reflect.c methods more fatal and more descriptive.
>
> Please review this at
http://codereview.appspot.com/**5302082/<http://codereview.appspot.com/5302082/>
>
> Affected files:
>  M src/cmd/gc/reflect.c
>  M src/cmd/gc/subr.c
>
>
> Index: src/cmd/gc/reflect.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/reflect.c
> +++ b/src/cmd/gc/reflect.c
> @@ -158,10 +158,13 @@
>        // generating code if necessary.
>        a = nil;
>        for(f=mt->xmethod; f; f=f->down) {
> -               if(f->type->etype != TFUNC)
> -                       continue;
>                if(f->etype != TFIELD)
> -                       fatal("methods: not field");
> +                       fatal("methods: not field %T", f);
> +               if (f->type->etype != TFUNC || !getthisx(f->type))
> +                       fatal("non-method on %T method %S %T\n", mt,
> f->sym, f);
> +               if (!getthisx(f->type)->type)
> +                       fatal("receiver with no type on %T method %S
> %T\n", mt, f->sym, f);
> +
>                method = f->sym;
>                if(method == nil)
>                        continue;
> Index: src/cmd/gc/subr.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/subr.c
> +++ b/src/cmd/gc/subr.c
> @@ -2178,8 +2178,11 @@
>                        if(c == 0)
>                                continue;
>                        if(c == 1) {
> -                               sl->good = 1;
> -                               sl->field = f;
> +                               // addot1 may have dug out arbitrary
> fields, we only want methods.
> +                               if(f->type->etype == TFUNC &&
> getthis(f->type)) {
> +                                       sl->good = 1;
> +                                       sl->field = f;
> +                               } // else print("not adding non-method %T:
> %T\n", sl->field, f);
>                        }
>                        break;
>                }
>
>
>

[rsc, 2011-11-03 14:31:07]

thistuple > 0

[lvd, 2011-11-03 14:36:03]

On Thu, Nov 3, 2011 at 15:31, Russ Cox <rsc@golang.org> wrote:

> thistuple > 0
>


yes

[lvd, 2011-11-03 14:41:54]

On 2011/11/03 14:36:03, lvd wrote:
> On Thu, Nov 3, 2011 at 15:31, Russ Cox <mailto:rsc@golang.org> wrote:
> 
> > thistuple > 0
> >
> 
> 
> yes

ready now.  ptal.

[rsc, 2011-11-03 16:47:33]

http://codereview.appspot.com/5302082/diff/13001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

http://codereview.appspot.com/5302082/diff/13001/src/cmd/gc/reflect.c#newcode163
src/cmd/gc/reflect.c:163: if (f->type->etype != TFUNC || f->type->thistuple ==
0)
s/if (/if(/
like rest of file

[rsc, 2011-11-03 16:48:31]

LGTM

http://codereview.appspot.com/5302082/diff/15001/test/fixedbugs/bug372.go
File test/fixedbugs/bug372.go (right):

http://codereview.appspot.com/5302082/diff/15001/test/fixedbugs/bug372.go#new...
test/fixedbugs/bug372.go:1: // $G $D/$F.go && $L $F.$A && ./$A.out || echo BUG:
issue2355
bug372 please

http://codereview.appspot.com/5302082/diff/15001/test/fixedbugs/bug372.go#new...
test/fixedbugs/bug372.go:6: 
add

// Issue 2355

here

[lvd, 2011-11-03 16:50:39]

http://codereview.appspot.com/5302082/diff/15001/test/fixedbugs/bug372.go
File test/fixedbugs/bug372.go (right):

http://codereview.appspot.com/5302082/diff/15001/test/fixedbugs/bug372.go#new...
test/fixedbugs/bug372.go:1: // $G $D/$F.go && $L $F.$A && ./$A.out || echo BUG:
issue2355
On 2011/11/03 16:48:31, rsc wrote:
> bug372 please

Done.

http://codereview.appspot.com/5302082/diff/15001/test/fixedbugs/bug372.go#new...
test/fixedbugs/bug372.go:6: 
On 2011/11/03 16:48:31, rsc wrote:
> add
> 
> // Issue 2355
> 
> here

Done.

[lvd, 2011-11-03 16:51:20]

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

gc: Don't pollute the xmethod list with non-methods.

Fixes issue 2355.

I have a test, but not sure if it's worth adding.  Instead i've made
the patching-over in reflect.c methods more fatal and more descriptive.

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