code review 6542060: cmd/gc: don't dereference a nil Type pointer in widstruct

cmd/gc: don't dereference a nil Type pointer in widstruct

The nil dereference in the next few lines doesn't seem
to cause a segmentation fault on Unix, but does seem
to halt the Go compiler.

The following is a test case:

>>>
package main

func mine(int b) int {
        return b + 2
}

func main() {
        mine()

        c = mine()
}
<<<

Without this change only the following is caught:

typecheck.go:3: undefined: b
typecheck.go:4: undefined: b

with it, we catch all the errors:

typecheck.go:3: undefined: b
typecheck.go:4: undefined: b
typecheck.go:10: undefined: c
typecheck.go:10: cannot assign to c .

[akumar, 2012-09-23 05:06:56]

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

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

[akumar, 2012-09-23 06:46:19]

I just realized that, that is perhaps not the best place to end the loop. I've
updated the CL.

PTAL.

[akumar, 2012-09-23 20:36:10]

Just to update the message for convenience, the new diff is:

Index: src/cmd/gc/align.c
===================================================================
--- a/src/cmd/gc/align.c
+++ b/src/cmd/gc/align.c
@@ -54,6 +54,8 @@
 	for(f=t->type; f!=T; f=f->down) {
 		if(f->etype != TFIELD)
 			fatal("widstruct: not TFIELD: %lT", f);
+		if(f->type == T)
+			break;
 		dowidth(f->type);
 		if(f->type->align > maxalign)
 			maxalign = f->type->align;

On 2012/09/23 06:46:19, akumar wrote:
> I just realized that, that is perhaps not the best place to end the loop. I've
> updated the CL.
> 
> PTAL.

[akumar, 2012-09-24 12:34:21]

ping?

On 23 September 2012 13:36,  <seed@mail.nanosouffle.net> wrote:
> Just to update the message for convenience, the new diff is:
>
>
> Index: src/cmd/gc/align.c
> ===================================================================
> --- a/src/cmd/gc/align.c
> +++ b/src/cmd/gc/align.c
> @@ -54,6 +54,8 @@
>
>         for(f=t->type; f!=T; f=f->down) {
>                 if(f->etype != TFIELD)
>                         fatal("widstruct: not TFIELD: %lT", f);
> +               if(f->type == T)
> +                       break;
>                 dowidth(f->type);
>                 if(f->type->align > maxalign)
>                         maxalign = f->type->align;
>
>
> On 2012/09/23 06:46:19, akumar wrote:
>>
>> I just realized that, that is perhaps not the best place to end the
>
> loop. I've
>>
>> updated the CL.
>
>
>> PTAL.
>
>
>
> http://codereview.appspot.com/6542060/

[minux1, 2012-09-24 12:52:58]

you need to at least make a test for it.

See tests under /test/ for examples.

[akumar, 2012-10-03 00:47:53]

PTAL.


On 2012/09/24 12:52:58, minux wrote:
> you need to at least make a test for it.
> 
> See tests under /test/ for examples.

Done.

[rsc, 2012-10-05 20:39:38]

The test as written won't work for gccgo. Can you use an errorcheck test instead
of a cmpout test? 

Thanks.

[akumar, 2012-10-05 21:23:25]

PTAL.

On 2012/10/05 20:39:38, rsc wrote:
> The test as written won't work for gccgo. Can you use an errorcheck test
instead
> of a cmpout test? 
> 
> Thanks.

Done.

[rsc, 2012-10-06 22:27:00]

LGTM

[minux1, 2012-10-07 06:12:14]

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

cmd/gc: don't dereference a nil Type pointer in widstruct

The nil dereference in the next few lines doesn't seem
to cause a segmentation fault on Unix, but does seem
to halt the Go compiler.

The following is a test case:

>>>
package main

func mine(int b) int {
        return b + 2
}

func main() {
        mine()

        c = mine()
}
<<<

Without this change only the following is caught:

typecheck.go:3: undefined: b
typecheck.go:4: undefined: b

with it, we catch all the errors:

typecheck.go:3: undefined: b
typecheck.go:4: undefined: b
typecheck.go:10: undefined: c
typecheck.go:10: cannot assign to c .

R=rsc, minux.ma
CC=golang-dev
http://codereview.appspot.com/6542060

Committer: Shenghou Ma <minux.ma@gmail.com>

[akumar, 2012-10-24 01:41:38]

*** Abandoned ***

[akumar, 2012-10-24 02:09:40]

On 2012/10/24 01:41:38, akumar wrote:
> *** Abandoned ***

Ugh, this was actually submitted, then changed. I merged the changes locally
then did `hg sync', which asked me to do `hg change -d 6542060'. My apologies
for listening to mercurial.