FAQ
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/

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.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Brad Fitzpatrick at Mar 22, 2013 at 2:46 am
    All the tests already pass, though! :-)
    On Thu, Mar 21, 2013 at 7:38 PM, 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/7728045/>

    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@googlegroups.com>
    .
    For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/opt_out>
    .

    --

    ---
    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.
  • Carl Shapiro at Mar 22, 2013 at 3:01 am

    On Thu, Mar 21, 2013 at 7:46 PM, Brad Fitzpatrick wrote:

    All the tests already pass, though! :-)

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

    --

    ---
    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.
  • Brad Fitzpatrick at Mar 22, 2013 at 3:04 am
    Even without the GC, can't you test Go -> C -> Go -> runtime.Callers ?
    On Thu, Mar 21, 2013 at 8:01 PM, Carl Shapiro wrote:
    On Thu, Mar 21, 2013 at 7:46 PM, Brad Fitzpatrick wrote:

    All the tests already pass, though! :-)

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

    ---
    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.
  • Carl Shapiro at Mar 22, 2013 at 6:54 pm

    On Thu, Mar 21, 2013 at 8:04 PM, Brad Fitzpatrick 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.

    --

    ---
    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.
  • Minux Ma at Mar 22, 2013 at 7:25 pm
    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#newcode347
    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#newcode396
    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.

    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.
  • Cshapiro at Mar 22, 2013 at 8:20 pm
    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#newcode347
    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#newcode396
    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.

    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.
  • Cshapiro at Mar 22, 2013 at 10:53 pm
    All review comments should be addressed.

    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.
  • Russ Cox at Mar 23, 2013 at 8:13 pm
    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

    --

    ---
    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.
  • Carl Shapiro at Mar 25, 2013 at 5:47 pm

    On Sat, Mar 23, 2013 at 1:13 PM, Russ Cox 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.

    --

    ---
    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.
  • Cshapiro at Mar 25, 2013 at 8:28 pm
    PTAL

    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.
  • Bradfitz at Mar 25, 2013 at 8:36 pm
    LGTM

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

    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.
  • Minux Ma at Mar 25, 2013 at 8:43 pm
    LGTM.

    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.
  • Minux at Mar 25, 2013 at 8:53 pm
    Do you want to add:

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

    --

    ---
    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.
  • Cshapiro at Mar 25, 2013 at 9:11 pm
    *** 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


    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.
  • Rsc at Mar 25, 2013 at 10:12 pm
    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#newcode370
    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.

    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.
  • Cshapiro at Mar 25, 2013 at 10:21 pm
    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#newcode370
    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.
  • Lucio De Re at Mar 26, 2013 at 7:45 am
    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 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#newcode370
    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.

    --

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedMar 22, '13 at 2:38a
activeMar 26, '13 at 7:45a
posts18
users6
websitegolang.org

People

Translate

site design / logo © 2021 Grokbase