FAQ
Reviewers: golang-dev_googlegroups.com,

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

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


Description:
cmd/6g: fix componentgen for funarg structs.

Fixes issue 4518.

Please review this at https://codereview.appspot.com/6932045/

Affected files:
M src/cmd/6g/cgen.c


Index: src/cmd/6g/cgen.c
===================================================================
--- a/src/cmd/6g/cgen.c
+++ b/src/cmd/6g/cgen.c
@@ -1632,9 +1632,17 @@
case TSTRUCT:
loffset = nodl.xoffset;
roffset = nodr.xoffset;
+ // funarg structs may not begin at offset zero.
+ if(nl->type->etype == TSTRUCT && nl->type->funarg && nl->type->type)
+ loffset -= nl->type->type->width;
+ if(nr != N && nr->type->etype == TSTRUCT && nr->type->funarg &&
nr->type->type)
+ roffset -= nr->type->type->width;
+
+ print("nodl %+N\nnodr %+N\n", &nodl, &nodr);
for(t=nl->type->type; t; t=t->down) {
nodl.xoffset = loffset + t->width;
nodl.type = t->type;
+ print("nodl %+N\nnodr %+N\n", &nodl, &nodr);

if(nr == N)
clearslim(&nodl);

Search Discussions

  • Remyoudompheng at Dec 11, 2012 at 7:34 am
    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.

    https://codereview.appspot.com/6932045/
  • Rsc at Dec 11, 2012 at 4:38 pm
    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.

    https://codereview.appspot.com/6932045/
  • Remyoudompheng at Dec 11, 2012 at 9:32 pm

    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#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.


    https://codereview.appspot.com/6932045/
  • Russ Cox at Dec 11, 2012 at 10:16 pm

    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 at Dec 11, 2012 at 9:42 pm
    Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6932045/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 11, '12 at 7:33a
activeDec 11, '12 at 10:16p
posts6
users2
websitegolang.org

2 users in discussion

Remyoudompheng: 4 posts Russ Cox: 2 posts

People

Translate

site design / logo © 2022 Grokbase