code review 115840048: runtime: round gohash stack frame to multiple of 8 on a...

runtime: round gohash stack frame to multiple of 8 on amd64p32

Even though pointers are 4 bytes the stack frame should be kept
a multiple of 8 bytes so that return addresses pushed on the stack
are properly aligned.

Fixes issue 8379.

[rsc, 2014-07-21 21:26:14]

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

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

[bradfitz, 2014-07-21 21:27:30]

Is there an associated bug or backstory?



On Mon, Jul 21, 2014 at 2:26 PM, <rsc@golang.org> wrote:

> Reviewers: minux,
>
> Message:
> Hello minux (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: round gohash stack frame to multiple of 8 on amd64p32
>
> Even though pointers are 4 bytes the stack frame should be kept
> a multiple of 8 bytes so that return addresses pushed on the stack
> are properly aligned.
>
> Please review this at https://codereview.appspot.com/115840048/
>
> Affected files (+1, -1 lines):
>   M src/pkg/runtime/asm_amd64p32.s
>
>
> Index: src/pkg/runtime/asm_amd64p32.s
> ===================================================================
> --- a/src/pkg/runtime/asm_amd64p32.s
> +++ b/src/pkg/runtime/asm_amd64p32.s
> @@ -1139,7 +1139,7 @@
>  //   func (alg unsafe.Pointer, p unsafe.Pointer, size uintpr, seed
> uintptr) uintptr
>  // to:
>  //   func (hash *uintptr, size uintptr, p unsafe.Pointer)
> -TEXT runtime·gohash(SB), NOSPLIT, $12-20
> +TEXT runtime·gohash(SB), NOSPLIT, $16-20
>         FUNCDATA $FUNCDATA_ArgsPointerMaps,gcargs_gohash<>(SB)
>         FUNCDATA $FUNCDATA_LocalsPointerMaps,gclocals_gohash<>(SB)
>         MOVL    a+0(FP), AX
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[dvyukov, 2014-07-21 21:28:48]

LGTM

[dvyukov, 2014-07-21 21:29:12]

On 2014/07/21 21:27:30, bradfitz wrote:
> Is there an associated bug or backstory?
> 
> 
> 
> On Mon, Jul 21, 2014 at 2:26 PM, <mailto:rsc@golang.org> wrote:
> 
> > Reviewers: minux,
> >
> > Message:
> > Hello minux (cc: mailto:golang-codereviews@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go/
> >
> >
> > Description:
> > runtime: round gohash stack frame to multiple of 8 on amd64p32
> >
> > Even though pointers are 4 bytes the stack frame should be kept
> > a multiple of 8 bytes so that return addresses pushed on the stack
> > are properly aligned.
> >
> > Please review this at https://codereview.appspot.com/115840048/
> >
> > Affected files (+1, -1 lines):
> >   M src/pkg/runtime/asm_amd64p32.s
> >
> >
> > Index: src/pkg/runtime/asm_amd64p32.s
> > ===================================================================
> > --- a/src/pkg/runtime/asm_amd64p32.s
> > +++ b/src/pkg/runtime/asm_amd64p32.s
> > @@ -1139,7 +1139,7 @@
> >  //   func (alg unsafe.Pointer, p unsafe.Pointer, size uintpr, seed
> > uintptr) uintptr
> >  // to:
> >  //   func (hash *uintptr, size uintptr, p unsafe.Pointer)
> > -TEXT runtime·gohash(SB), NOSPLIT, $12-20
> > +TEXT runtime·gohash(SB), NOSPLIT, $16-20
> >         FUNCDATA $FUNCDATA_ArgsPointerMaps,gcargs_gohash<>(SB)
> >         FUNCDATA $FUNCDATA_LocalsPointerMaps,gclocals_gohash<>(SB)
> >         MOVL    a+0(FP), AX
> >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

nacl builder issues "unaligned stack size 12" warnings:
http://build.golang.org/log/9770c83d6b619163a42b9c65101af94e4c1787c3

[dave_cheney.net, 2014-07-21 21:34:17]

Fixes issue 8379.


On Tue, Jul 22, 2014 at 7:29 AM, dvyukov via golang-codereviews <
golang-codereviews@googlegroups.com> wrote:

> On 2014/07/21 21:27:30, bradfitz wrote:
>
>> Is there an associated bug or backstory?
>>
>
>
>
>  On Mon, Jul 21, 2014 at 2:26 PM, <mailto:rsc@golang.org> wrote:
>>
>
>  > Reviewers: minux,
>> >
>> > Message:
>> > Hello minux (cc: mailto:golang-codereviews@googlegroups.com),
>>
>> >
>> > I'd like you to review this change to
>> > https://code.google.com/p/go/
>> >
>> >
>> > Description:
>> > runtime: round gohash stack frame to multiple of 8 on amd64p32
>> >
>> > Even though pointers are 4 bytes the stack frame should be kept
>> > a multiple of 8 bytes so that return addresses pushed on the stack
>> > are properly aligned.
>> >
>> > Please review this at https://codereview.appspot.com/115840048/
>> >
>> > Affected files (+1, -1 lines):
>> >   M src/pkg/runtime/asm_amd64p32.s
>> >
>> >
>> > Index: src/pkg/runtime/asm_amd64p32.s
>> > ===================================================================
>> > --- a/src/pkg/runtime/asm_amd64p32.s
>> > +++ b/src/pkg/runtime/asm_amd64p32.s
>> > @@ -1139,7 +1139,7 @@
>> >  //   func (alg unsafe.Pointer, p unsafe.Pointer, size uintpr, seed
>> > uintptr) uintptr
>> >  // to:
>> >  //   func (hash *uintptr, size uintptr, p unsafe.Pointer)
>> > -TEXT runtime·gohash(SB), NOSPLIT, $12-20
>> > +TEXT runtime·gohash(SB), NOSPLIT, $16-20
>> >         FUNCDATA $FUNCDATA_ArgsPointerMaps,gcargs_gohash<>(SB)
>> >         FUNCDATA $FUNCDATA_LocalsPointerMaps,gclocals_gohash<>(SB)
>> >         MOVL    a+0(FP), AX
>> >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>>
> Groups
>
>> > "golang-codereviews" group.
>> > To unsubscribe from this group and stop receiving emails from it,
>>
> send an
>
>> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
>>
>> > For more options, visit https://groups.google.com/d/optout.
>> >
>>
>
> nacl builder issues "unaligned stack size 12" warnings:
> http://build.golang.org/log/9770c83d6b619163a42b9c65101af94e4c1787c3
>
>
> https://codereview.appspot.com/115840048/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[minux, 2014-07-21 21:36:11]

On Mon, Jul 21, 2014 at 5:29 PM, <dvyukov@google.com> wrote:

> On 2014/07/21 21:27:30, bradfitz wrote:
>
>> Is there an associated bug or backstory?
>>
> On Mon, Jul 21, 2014 at 2:26 PM, <mailto:rsc@golang.org> wrote:
>>
> nacl builder issues "unaligned stack size 12" warnings:
> http://build.golang.org/log/9770c83d6b619163a42b9c65101af94e4c1787c3

Yeah, that's it.

LGTM.

[rsc, 2014-07-21 21:38:37]

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

runtime: round gohash stack frame to multiple of 8 on amd64p32

Even though pointers are 4 bytes the stack frame should be kept
a multiple of 8 bytes so that return addresses pushed on the stack
are properly aligned.

Fixes issue 8379.

LGTM=dvyukov, minux
R=minux, bradfitz, dvyukov, dave
CC=golang-codereviews
https://codereview.appspot.com/115840048

[gobot, 2014-07-21 22:24:19]

This CL appears to have broken the nacl-arm builder.
See http://build.golang.org/log/6dac54b5cb4d1abac23959e70e4bc050d477d1ec

[minux, 2014-07-22 02:25:29]

On Mon, Jul 21, 2014 at 6:24 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the nacl-arm builder.
> See http://build.golang.org/log/6dac54b5cb4d1abac23959e70e4bc050d477d1ec

Not really. It's due to misconfiguration.