code review 6932045: cmd/6g: fix componentgen for funarg structs.

cmd/6g: fix componentgen for funarg structs.

Fixes issue 4518.

[remyoudompheng, 2012-12-11 07:33:14]

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

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

[remyoudompheng, 2012-12-11 07:34:49]

This CL will need an appropriate test first. rsc's example looks fine but I
tried reproducing using code like:

var seven, thirteen = 7, 13

func F(a interface{}) (*int, *int) {
	DontInlineMe()
      switch a.(type) {
      case nil:
      case int:
            return &seven, &thirteen
      }
      panic("unreachable")
}

type T int

func Bogus1() (*int, *int)                    { return F(5) }
func Bogus2(a interface{}) (*int, *int)       { return F(a) }
func (t T) Bogus3() (*int, *int)              { return F(23) }
func (t T) Bogus4(a interface{}) (*int, *int) { return F(56) }

and couldn't reproduce the bug there. The bug happens when the generated code
for return F(...) is a direct assignment of the funarg struct.

[rsc, 2012-12-11 16:38:24]

I don't fully understand your mail. Are you saying the CL breaks the example you
posted?

https://codereview.appspot.com/6932045/diff/3002/src/cmd/6g/cgen.c
File src/cmd/6g/cgen.c (right):

https://codereview.appspot.com/6932045/diff/3002/src/cmd/6g/cgen.c#newcode1635
src/cmd/6g/cgen.c:1635: // funarg structs may not begin at offset zero.
Does this work?

I was expecting that the problem had something to do with the stack compaction
and that the fix would be there. That is, my guess was that the offset was
correct here but then was not adjusted properly during stack compaction.

[remyoudompheng, 2012-12-11 21:31:47]

On 2012/12/11 16:38:24, rsc wrote:
> I don't fully understand your mail. Are you saying the CL breaks the example
you
> posted?

Sorry, I meant: I can add a test with your example but it makes be unhappy that
I can't produce an example that doesn't use the type switch which looks like
noise.

I'm pretty sure there is a smaller snippet that reproduces the situation.

> 
> https://codereview.appspot.com/6932045/diff/3002/src/cmd/6g/cgen.c
> File src/cmd/6g/cgen.c (right):
> 
> https://codereview.appspot.com/6932045/diff/3002/src/cmd/6g/cgen.c#newcode1635
> src/cmd/6g/cgen.c:1635: // funarg structs may not begin at offset zero.
> Does this work?
> 
> I was expecting that the problem had something to do with the stack compaction
> and that the fix would be there. That is, my guess was that the offset was
> correct here but then was not adjusted properly during stack compaction.

The nodarg function is quite clear about the fact that the width field in
arguments is their offset wrt the first function arguments. So given a STRUCT
that is also a funarg, xoffset + width of a field is wrong.

[remyoudompheng, 2012-12-11 21:36:56]

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

Please take another look.

[rsc, 2012-12-11 22:16:39]

> Sorry, I meant: I can add a test with your example but it makes be
> unhappy that I can't produce an example that doesn't use the type switch
> which looks like noise.

I need to look more carefully at the code so don't read too much into
this but...
I thought the type switch was helping by creating a temporary on the
stack that turned out to registerize perfectly, so that there was
something for stack frame compaction to reclaim.

Will look at the code you sent a bit later.

Thanks.
Russ

[remyoudompheng, 2012-12-14 22:35:11]

Any opinion now?

[rsc, 2012-12-17 14:56:09]

LGTM

It makes sense to me now. I was assuming the offset change was due to the stack
compaction, but it was due to recording the FP offset instead of the SP offset
in the funarg node, which is what you're now accounting for.

[remyoudompheng, 2012-12-17 21:37:55]

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

cmd/6g: fix componentgen for funarg structs.

Fixes issue 4518.

R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/6932045