code review 7090043: cmd/5c: fix handling of side effects when assigning a s...

cmd/5c: fix handling of side effects when assigning a struct literal.

Also undo revision a5b96b602690 used to workaround the bug.

Fixes issue 4643.

[remyoudompheng, 2013-01-11 07:12:10]

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

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

[dfc, 2013-01-11 22:19:32]

Here is the asm afterwards

                        *bitbufpos++ = (BitTarget){obj, ti, bitp, shift};
   59180:       e59d0020        ldr     r0, [sp, #32]
   59184:       e2801010        add     r1, r0, #16
   59188:       e58d1020        str     r1, [sp, #32]
   5918c:       e58d0014        str     r0, [sp, #20]
   59190:       e59d1014        ldr     r1, [sp, #20]
   59194:       e5816000        str     r6, [r1]
   59198:       e59d1014        ldr     r1, [sp, #20]
   5919c:       e59d203c        ldr     r2, [sp, #60]   ; 0x3c
   591a0:       e5812004        str     r2, [r1, #4]
   591a4:       e59d1014        ldr     r1, [sp, #20]
   591a8:       e581c008        str     ip, [r1, #8]
   591ac:       e59d1014        ldr     r1, [sp, #20]
   591b0:       e581700c        str     r7, [r1, #12]

[minux1, 2013-01-12 07:24:32]

LGTM.

The bug doesn't exist in cmd/6c and cmd/8c since
their inclusion (although the comment above the
if is out-of-date, but it's up to you whether to
fix them).

PS: the bug still exists in Plan 9 upstream.
Remy, please consider push the patch to upstream.

[lucio, 2013-01-12 07:34:57]

I'm going to shoot from the hip here, so don't be shy to correct me.
I assumed that the problem only affected the Go tool because it
involved the assignment of a struct literal and these are not (from
memory) permissible constructs in traditional C.  Of course, I don't
necessarily remember right.

Anyway, my recommendation regarding feeding corrections upstream in
the Plan 9/ARM realm, is to pass them on to Richard Miller who's
porting Plan 9 to the Raspberry Pi.  I've included him in the CC:
list.

For Richard's benefit: the URL for the codereview entry is at the
bottom of this message.

Lucio.

On 1/12/13, minux.ma@gmail.com <minux.ma@gmail.com> wrote:
> LGTM.
>
> The bug doesn't exist in cmd/6c and cmd/8c since
> their inclusion (although the comment above the
> if is out-of-date, but it's up to you whether to
> fix them).
>
> PS: the bug still exists in Plan 9 upstream.
> Remy, please consider push the patch to upstream.
>
> https://codereview.appspot.com/7090043/
>

[minux1, 2013-01-12 07:42:39]

On Sat, Jan 12, 2013 at 3:34 PM, Lucio De Re <lucio.dere@gmail.com> wrote:

> I'm going to shoot from the hip here, so don't be shy to correct me.
> I assumed that the problem only affected the Go tool because it
> involved the assignment of a struct literal and these are not (from
> memory) permissible constructs in traditional C.  Of course, I don't
> necessarily remember right.
>
It's a Plan 9 C extension.
Quoting http://doc.cat-v.org/plan_9/4th_edition/papers/comp

this assignment may appear anywhere an assignment is legal:

 r = (Rectangle){add(p, q), (Point){x, y+3}};

[bradfitz, 2013-01-12 07:43:25]

Richard, see https://code.google.com/p/go/issues/detail?id=4643

On Fri, Jan 11, 2013 at 11:34 PM, Lucio De Re <lucio.dere@gmail.com> wrote:

> I'm going to shoot from the hip here, so don't be shy to correct me.
> I assumed that the problem only affected the Go tool because it
> involved the assignment of a struct literal and these are not (from
> memory) permissible constructs in traditional C.  Of course, I don't
> necessarily remember right.
>
> Anyway, my recommendation regarding feeding corrections upstream in
> the Plan 9/ARM realm, is to pass them on to Richard Miller who's
> porting Plan 9 to the Raspberry Pi.  I've included him in the CC:
> list.
>
> For Richard's benefit: the URL for the codereview entry is at the
> bottom of this message.
>
> Lucio.
>
> On 1/12/13, minux.ma@gmail.com <minux.ma@gmail.com> wrote:
> > LGTM.
> >
> > The bug doesn't exist in cmd/6c and cmd/8c since
> > their inclusion (although the comment above the
> > if is out-of-date, but it's up to you whether to
> > fix them).
> >
> > PS: the bug still exists in Plan 9 upstream.
> > Remy, please consider push the patch to upstream.
> >
> > https://codereview.appspot.com/7090043/
> >
>

[remyoudompheng, 2013-01-12 08:17:23]

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

cmd/5c: fix handling of side effects when assigning a struct literal.

Also undo revision a5b96b602690 used to workaround the bug.

Fixes issue 4643.

R=rsc, golang-dev, dave, minux.ma, lucio.dere, bradfitz
CC=golang-dev
https://codereview.appspot.com/7090043