code review 8663052: cmd/gc: initialize t->width in dgcsym() if required

cmd/gc: initialize t->width in dgcsym() if required

Update issue 5291.

[atom, 2013-04-24 09:55:49]

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

I'd like you to review this change to
https://code.google.com/p/go/

[DMorsing, 2013-04-24 11:09:28]

Which types are getting through to reflect without valid widths? I think this is
a bug elsewhere in gc and this is just a bandaid.

GC type gen is after function compilation, so any type without a valid width
will have been miscompiled anyway.

[atom, 2013-04-24 11:47:53]

On 2013/04/24 11:09:28, DMorsing wrote:
> Which types are getting through to reflect without valid widths? I think this
is
> a bug elsewhere in gc and this is just a bandaid.
> 
> GC type gen is after function compilation, so any type without a valid width
> will have been miscompiled anyway.

Calls to dowidth(Type*) are distributed over multiple places in src/cmd/gc/*.c
and the rule seems to be that it is called to initialize struct Type before
accessing certain fields of the struct.

It would be clearer to compute all widths in one pass and one place, but this
doesn't seem to be the case in the current implementation. 

Alternatively, one could introduce an accessor function such as getwidth(Type*
t). This would be safer to use that accessing t->width directly.

[DMorsing, 2013-04-24 12:57:44]

On 2013/04/24 11:47:53, atom wrote:
> On 2013/04/24 11:09:28, DMorsing wrote:
> > Which types are getting through to reflect without valid widths? I think
this
> is
> > a bug elsewhere in gc and this is just a bandaid.
> > 
> > GC type gen is after function compilation, so any type without a valid width
> > will have been miscompiled anyway.
> 
> Calls to dowidth(Type*) are distributed over multiple places in src/cmd/gc/*.c
> and the rule seems to be that it is called to initialize struct Type before
> accessing certain fields of the struct.
> 

That is correct, but all those calls happen before or during codegen. If the
width of a type isn't set after codegen, there's a good chance we're
miscompiling code using that type.

[iant, 2013-04-24 13:45:19]

On 2013/04/24 11:09:28, DMorsing wrote:
> Which types are getting through to reflect without valid widths? I think this
is
> a bug elsewhere in gc and this is just a bandaid.
> 
> GC type gen is after function compilation, so any type without a valid width
> will have been miscompiled anyway.

You can clpatch 8607045 to get a test case that shows the problem.

[iant, 2013-04-24 13:45:43]

On 2013/04/24 13:45:19, iant wrote:
> On 2013/04/24 11:09:28, DMorsing wrote:
> > Which types are getting through to reflect without valid widths? I think
this
> is
> > a bug elsewhere in gc and this is just a bandaid.
> > 
> > GC type gen is after function compilation, so any type without a valid width
> > will have been miscompiled anyway.
> 
> You can clpatch 8607045 to get a test case that shows the problem.

In fact, that test case ought to be part of this CL.

[DMorsing, 2013-04-24 14:53:20]

LGTM when the test has been added.

We're not miscompiling things. The problem is that we don't calculate the width
of types from other packages if we don't reference them in our code, but the .gc
section might still reference them.

https://codereview.appspot.com/8663052/diff/3/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

https://codereview.appspot.com/8663052/diff/3/src/cmd/gc/reflect.c#newcode1147
src/cmd/gc/reflect.c:1147: if(t->width == BADWIDTH)
s/width/type->width/

[atom, 2013-04-24 16:03:37]

https://codereview.appspot.com/8663052/diff/3/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

https://codereview.appspot.com/8663052/diff/3/src/cmd/gc/reflect.c#newcode1147
src/cmd/gc/reflect.c:1147: if(t->width == BADWIDTH)
On 2013/04/24 14:53:20, DMorsing wrote:
> s/width/type->width/

Done.

[atom, 2013-04-24 16:04:03]

On 2013/04/24 13:45:43, iant wrote:
> On 2013/04/24 13:45:19, iant wrote:
> > On 2013/04/24 11:09:28, DMorsing wrote:
> > > Which types are getting through to reflect without valid widths? I think
> this
> > is
> > > a bug elsewhere in gc and this is just a bandaid.
> > > 
> > > GC type gen is after function compilation, so any type without a valid
width
> > > will have been miscompiled anyway.
> > 
> > You can clpatch 8607045 to get a test case that shows the problem.
> 
> In fact, that test case ought to be part of this CL.

Done.

[iant, 2013-04-25 00:43:12]

Patch looks good, test case needs a bit of cleanup.

Thanks.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
File test/fixedbugs/issue5291.dir/pkg1.go (right):

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/pkg1.go:1: package pkg1
Needs a copyright header.  Sorry.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/pkg1.go:18: fmt.Println("Iteration", count)
Let's not bother to print this, I don't think it affects whether the bug hits or
not.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
File test/fixedbugs/issue5291.dir/prog.go (right):

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/prog.go:1: package main
Needs a copyright header.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.out
File test/fixedbugs/issue5291.out (right):

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.ou...
test/fixedbugs/issue5291.out:1: Iteration 0
If you remove the fmt.Print in pkg1.go then you don't need this file.

[atom, 2013-04-25 07:29:17]

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
File test/fixedbugs/issue5291.dir/pkg1.go (right):

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/pkg1.go:1: package pkg1
On 2013/04/25 00:43:12, iant wrote:
> Needs a copyright header.  Sorry.

Done.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/pkg1.go:18: fmt.Println("Iteration", count)
On 2013/04/25 00:43:12, iant wrote:
> Let's not bother to print this, I don't think it affects whether the bug hits
or
> not.

Done.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
File test/fixedbugs/issue5291.dir/prog.go (right):

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/prog.go:1: package main
On 2013/04/25 00:43:12, iant wrote:
> Needs a copyright header.

Done.

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.out
File test/fixedbugs/issue5291.out (right):

https://codereview.appspot.com/8663052/diff/15001/test/fixedbugs/issue5291.ou...
test/fixedbugs/issue5291.out:1: Iteration 0
On 2013/04/25 00:43:12, iant wrote:
> If you remove the fmt.Print in pkg1.go then you don't need this file.

Done.

[iant, 2013-04-25 14:36:37]

LGTM

Wait for r.

[r, 2013-04-25 15:10:22]

LGTM

https://codereview.appspot.com/8663052/diff/23001/test/fixedbugs/issue5291.di...
File test/fixedbugs/issue5291.dir/pkg1.go (right):

https://codereview.appspot.com/8663052/diff/23001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/pkg1.go:17: func Crashcall() (err error) {
CrashCall please

[atom, 2013-04-25 16:47:21]

*** Submitted as https://code.google.com/p/go/source/detail?r=1d079908dd84 ***

cmd/gc: initialize t->width in dgcsym() if required

Update issue 5291.

R=golang-dev, daniel.morsing, iant, r
CC=golang-dev
https://codereview.appspot.com/8663052

https://codereview.appspot.com/8663052/diff/23001/test/fixedbugs/issue5291.di...
File test/fixedbugs/issue5291.dir/pkg1.go (right):

https://codereview.appspot.com/8663052/diff/23001/test/fixedbugs/issue5291.di...
test/fixedbugs/issue5291.dir/pkg1.go:17: func Crashcall() (err error) {
On 2013/04/25 15:10:23, r wrote:
> CrashCall please

Done.