code review 7728045: runtime: store asmcgocall return PC where the ARM unwin...

runtime: store asmcgocall return PC where the ARM unwind expects it

The ARM implementation of runtime.cgocallback_gofunc diverged
from the calling convention by leaving a word of garbage at
the top of the stack and storing the return PC above the
locals.  This change stores the return PC at the top of the
stack and removes the save area above the locals.

Update Issue 5124
This CL fixes first part of the ARM issues and added the unwind test.

[cshapiro, 2013-03-22 02:38:54]

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

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

[bradfitz, 2013-03-22 02:46:14]

All the tests already pass, though!  :-)

On Thu, Mar 21, 2013 at 7:38 PM, <cshapiro@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: store asmcgocall return PC where the ARM unwind expects it
>
> The ARM implementation of runtime.cgocallback_gofunc diverged
> from the calling convention by leaving a word of garbage at
> the top of the stack and storing the return PC above the
> locals.  This change stores the return PC at the top of the
> stack and removes the save area above the locals.
>
> Please review this at
https://codereview.appspot.**com/7728045/<https://codereview.appspot.com/7728...
>
> Affected files:
>   M src/pkg/runtime/asm_arm.s
>
>
> Index: src/pkg/runtime/asm_arm.s
> ==============================**==============================**=======
> --- a/src/pkg/runtime/asm_arm.s
> +++ b/src/pkg/runtime/asm_arm.s
> @@ -326,7 +326,7 @@
>
>  // cgocallback_gofunc(void (*fn)(void*), void *frame, uintptr framesize)
>  // See cgocall.c for more details.
> -TEXT   runtime·cgocallback_gofunc(SB)**,7,$16
> +TEXT   runtime·cgocallback_gofunc(SB)**,7,$12
>         // Load m and g from thread-local storage.
>         MOVW    _cgo_load_gm(SB), R0
>         CMP     $0, R0
> @@ -337,21 +337,21 @@
>         // In this case, we're running on the thread stack, so there's
>         // lots of space, but the linker doesn't know. Hide the call from
>         // the linker analysis by using an indirect call.
> -       MOVW    m, savedm-16(SP)
> +       MOVW    m, savedm-12(SP)
>         CMP     $0, m
>         B.NE havem
>         MOVW    $runtime·needm(SB), R0
>         BL      (R0)
>
>  havem:
> +       MOVW    fn+0(FP), R0
> +       MOVW    frame+4(FP), R1
> +       MOVW    framesize+8(FP), R2
> +
>         // Now there's a valid m, and we're running on its m->g0.
>         // Save current m->g0->sched.sp on stack and then set it to SP.
>         // Save current sp in m->g0->sched.sp in preparation for
>         // switch back to m->curg stack.
> -       MOVW    fn+0(FP), R0
> -       MOVW    frame+4(FP), R1
> -       MOVW    framesize+8(FP), R2
> -
>         MOVW    m_g0(m), R3
>         MOVW    (g_sched+gobuf_sp)(R3), R4
>         MOVW.W  R4, -4(R13)
> @@ -368,23 +368,20 @@
>         // This has the added benefit that it looks to the traceback
>         // routine like cgocallbackg is going to return to that
>         // PC (because we defined cgocallbackg to have
> -       // a frame size of 16, the same amount that we use below),
> +       // a frame size of 12, the same amount that we use below),
>         // so that the traceback will seamlessly trace back into
>         // the earlier calls.
>
> -       // Save current m->g0->sched.sp on stack and then set it to SP.
>         MOVW    m_curg(m), g
>         MOVW    (g_sched+gobuf_sp)(g), R4 // prepare stack as R4
>
>         // Push gobuf.pc
>         MOVW    (g_sched+gobuf_pc)(g), R5
> -       SUB     $4, R4
> -       MOVW    R5, 0(R4)
> +       MOVW.W  R5, -16(R4)
>
>         // Push arguments to cgocallbackg.
>         // Frame size here must match the frame size above
>         // to trick traceback routines into doing the right thing.
> -       SUB     $16, R4
>         MOVW    R0, 4(R4)
>         MOVW    R1, 8(R4)
>         MOVW    R2, 12(R4)
> @@ -394,9 +391,9 @@
>         BL      runtime·cgocallbackg(SB)
>
>         // Restore g->gobuf (== m->curg->gobuf) from saved values.
> -       MOVW    16(R13), R5
> +       MOVW    0(R13), R5
>         MOVW    R5, (g_sched+gobuf_pc)(g)
> -       ADD     $(16+4), R13 // SP clobbered! It is ok!
> +       ADD     $(12+4), R13 // SP clobbered! It is ok!
>         MOVW    R13, (g_sched+gobuf_sp)(g)
>
>         // Switch back to m->g0's stack and restore m->g0->sched.sp.
> @@ -411,7 +408,7 @@
>
>         // If the m on entry was nil, we called needm above to borrow an m
>         // for the duration of the call. Since the call is over, return it
> with dropm.
> -       MOVW    savedm-16(SP), R6
> +       MOVW    savedm-12(SP), R6
>         CMP     $0, R6
>         B.NE    3(PC)
>         MOVW    $runtime·dropm(SB), R0
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[cshapiro, 2013-03-22 03:01:33]

On Thu, Mar 21, 2013 at 7:46 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> All the tests already pass, though!  :-)


True.  However, once the GC unwinds the stack they will not.  Specifically,
the cgo TestCallbackGC will fail.

[bradfitz, 2013-03-22 03:04:43]

Even without the GC, can't you test Go -> C -> Go -> runtime.Callers ?

On Thu, Mar 21, 2013 at 8:01 PM, Carl Shapiro <cshapiro@golang.org> wrote:

> On Thu, Mar 21, 2013 at 7:46 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> All the tests already pass, though!  :-)
>
>
> True.  However, once the GC unwinds the stack they will not.
>  Specifically, the cgo TestCallbackGC will fail.
>

[cshapiro, 2013-03-22 18:54:28]

On Thu, Mar 21, 2013 at 8:04 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Even without the GC, can't you test Go -> C -> Go -> runtime.Callers ?


There may be some miscommunication.  This change alone is not sufficient to
make the stack unwind work.  See my latest message on the "unwinding arm
stacks through call-outs and call-backs" thread.

It will be possible to write a test that does not require the GC.  More
work is required before such a test can be made to pass.

[minux1, 2013-03-22 19:25:05]

looks mostly fine to me.

https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s#ne...
src/pkg/runtime/asm_arm.s:347: MOVW	fn+0(FP), R0
why move this code block?

https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s#ne...
src/pkg/runtime/asm_arm.s:396: ADD	$(12+4), R13 // SP clobbered! It is ok!
if you want, you could use another register in place of R13
and remove the comment.

[cshapiro1, 2013-03-22 20:20:05]

https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s#ne...
src/pkg/runtime/asm_arm.s:347: MOVW	fn+0(FP), R0
On 2013/03/22 19:25:06, minux wrote:
> why move this code block?

My intention was to move the comment down to the code it makes reference to.

On the x86 the argument moves would be on line 374.  I tried moving this code
down there but got distracted by other issues before I could make it work.  I
can leave a TODO here or give that change another try.

What do you think?

https://codereview.appspot.com/7728045/diff/5001/src/pkg/runtime/asm_arm.s#ne...
src/pkg/runtime/asm_arm.s:396: ADD	$(12+4), R13 // SP clobbered! It is ok!
Sounds good.  I will make that change and re-upload.

[cshapiro1, 2013-03-22 22:53:44]

All review comments should be addressed.

[rsc, 2013-03-23 20:13:04]

Can you please add a test for this? Otherwise it will just break again.
It should be possible to write a test using runtime.Caller and/or
runtime.Stack.

Russ

[cshapiro1, 2013-03-25 17:47:31]

On Sat, Mar 23, 2013 at 1:13 PM, Russ Cox <rsc@golang.org> wrote:

> Can you please add a test for this? Otherwise it will just break again.
> It should be possible to write a test using runtime.Caller and/or
> runtime.Stack.
>

No, I cannot.  The unwind does not work with this change alone.

[cshapiro1, 2013-03-25 20:28:38]

PTAL

[bradfitz, 2013-03-25 20:36:08]

LGTM

More for the test code than the assembly, but I'll trust you, especially now
that there's a test.

[minux1, 2013-03-25 20:43:55]

LGTM.

[minux1, 2013-03-25 20:45:18]

Do you want to add:

Update Issue 5124
This CL fixes first part of the ARM issues and added the unwind test.

[cshapiro, 2013-03-25 21:11:24]

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

runtime: store asmcgocall return PC where the ARM unwind expects it

The ARM implementation of runtime.cgocallback_gofunc diverged
from the calling convention by leaving a word of garbage at
the top of the stack and storing the return PC above the
locals.  This change stores the return PC at the top of the
stack and removes the save area above the locals.

Update Issue 5124
This CL fixes first part of the ARM issues and added the unwind test.

R=golang-dev, bradfitz, minux.ma, cshapiro, rsc
CC=golang-dev
https://codereview.appspot.com/7728045

[rsc, 2013-03-25 22:12:01]

The changes look good otherwise.

https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s#n...
src/pkg/runtime/asm_arm.s:370: MOVW	fn+4(FP), R0
This is very confusing. Can you please move the instructions back up above so
they can use the correct offsets? Here you're writing fn+4(FP), even though fn
is really at 0(FP), because you need to adjust for the -4 of the MOVW.W above.
Unless there's a very good reason - and if so it should be commented - the fix
is to do these loads above, before changing R13 behind the assembler's back.

[cshapiro1, 2013-03-25 22:21:04]

https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s#n...
src/pkg/runtime/asm_arm.s:370: MOVW	fn+4(FP), R0
The motivation here is to make the code read like the x86 code.  Where this code
was before, the preceeding comment was separated from its referent.

I don't find this so confusing so I will defer to you on what might be more
perspicuous.  The 2 options I see are 1) leave this where it is and add a note
about the FP being adjusted 2) move this code below line 346

Either way is fine with me.  Let me know what you like.

[lucio, 2013-03-26 07:45:45]

Can I vote in favour of exploiting the "_arm" suffix and making the
code differ from _386?  It is the only place where previous art can be
treated as irrelevant and whereas I don't know whether the 386 code is
intentionally obfuscating or merely clever, I don't see why the arm
code should follow that example.

To be a bit facitious, would a comment like

  // let's do things like for the 386

not look silly in an arm module?

Lucio.

PS: The reason I'm speaking up is that I'm about to submit a CL to
port Go to plan9/386 and in helping prepare the CL I became convinced
that it is poor judgement to follow the 386 examples too far.


On 3/26/13, cshapiro@google.com <cshapiro@google.com> wrote:
>
> https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s
> File src/pkg/runtime/asm_arm.s (right):
>
>
https://codereview.appspot.com/7728045/diff/32001/src/pkg/runtime/asm_arm.s#n...
> src/pkg/runtime/asm_arm.s:370: MOVW	fn+4(FP), R0
> The motivation here is to make the code read like the x86 code.  Where
> this code was before, the preceeding comment was separated from its
> referent.
>
> I don't find this so confusing so I will defer to you on what might be
> more perspicuous.  The 2 options I see are 1) leave this where it is and
> add a note about the FP being adjusted 2) move this code below line 346
>
> Either way is fine with me.  Let me know what you like.
>
> https://codereview.appspot.com/7728045/
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>