FAQ
Hi Minux,

Do you have any suggestions to fix the backtracer? Do we need to remove
the NOSPLIT def from the asm call ?

Cheers

Dave

https://codereview.appspot.com/6939056/

Search Discussions

  • Russ Cox at Dec 18, 2012 at 11:52 pm
    What if you try main calling f calling g calling CompareAndSwapUint64?

    Russ
  • Minux at Dec 19, 2012 at 7:07 pm

    On Wed, Dec 19, 2012 at 7:52 AM, Russ Cox wrote:

    What if you try main calling f calling g calling CompareAndSwapUint64?
    panic: runtime error: invalid memory address or nil pointer dereference
    [signal 0xb code=0x1 addr=0x0 pc=0x2af08]

    goroutine 1 [running]:
    sync/atomic.CompareAndSwapUint64(0x0, 0x0)
    /root/fast/go.hg/src/pkg/sync/atomic/asm_linux_arm.s:132 +0x4
    main.f()
    /root/fast/go.hg/src/atomic.go:11 +0x20
    main.f()
    /root/fast/go.hg/src/atomic.go:11 +0x20

    goroutine 2 [syscall]:
    created by runtime.main
    /root/fast/go.hg/src/pkg/runtime/proc.c:225
  • Minux at Dec 19, 2012 at 8:58 pm

    On Wed, Dec 19, 2012 at 7:37 AM, wrote:

    Do you have any suggestions to fix the backtracer? Do we need to remove
    the NOSPLIT def from the asm call ?
    The only solution I found is to save LR onto stack before deref the pointer.

    diff -r e7cd0a82d669 src/pkg/sync/atomic/asm_linux_arm.s
    --- a/src/pkg/sync/atomic/asm_linux_arm.s Tue Dec 18 16:38:00 2012 -0800
    +++ b/src/pkg/sync/atomic/asm_linux_arm.s Wed Dec 19 20:56:34 2012 +0000
    @@ -127,6 +128,10 @@
    B ·CompareAndSwapUint64(SB)

    TEXT ·CompareAndSwapUint64(SB),7,$-4
    + MOVW.W R14,-4(R13) // just for the backtracer
    + MOVW addr+0(FP), R1
    + MOVW (R1), R0 // check that ptr is not nil
    + ADD $4, R13
    MOVW armCAS64(SB), R0
    CMP $0, R0
    MOVW.NE R0, PC
  • Minux at Dec 19, 2012 at 9:01 pm

    On Thu, Dec 20, 2012 at 4:58 AM, minux wrote:

    The only solution I found is to save LR onto stack before deref the
    pointer.
    Oh, this is not the only solution, but the only quick and easy solution.
    the other alternative is to fix the backtracer to not assume return address
    is always on the stack.

    Before I set out to fix the backtracer, I'd like to hear opinions about it.
  • Dave Cheney at Dec 19, 2012 at 9:03 pm
    I think the cost of those additional instructions is minimal. +1 to
    this approach with a TODO/issue to fix the backtracer.

    @minux, thank you again for your hard work. I"ll add this change to
    the CL. Do you want to raise the issue to improve the backtracer
    (probably don't need to be for Go 1.1.)
    On Thu, Dec 20, 2012 at 8:00 AM, minux wrote:
    On Thu, Dec 20, 2012 at 4:58 AM, minux wrote:

    The only solution I found is to save LR onto stack before deref the
    pointer.
    Oh, this is not the only solution, but the only quick and easy solution.
    the other alternative is to fix the backtracer to not assume return address
    is always on the stack.

    Before I set out to fix the backtracer, I'd like to hear opinions about it.
  • Minux at Dec 19, 2012 at 9:07 pm

    On Thu, Dec 20, 2012 at 5:03 AM, Dave Cheney wrote:

    @minux, thank you again for your hard work. I"ll add this change to
    the CL. Do you want to raise the issue to improve the backtracer
    (probably don't need to be for Go 1.1.)
    I will see if the backtracer is easy to fix, feel free to file an issue and
    assign
    to me with label Go1.1Maybe, as i think this issue won't affect most people.

    this CL LGTM with the 2 new lines of code (please add some comment for
    it though).
  • Dave at Dec 20, 2012 at 5:40 am
    @minux,

    I applied your suggestion but on my pandaboard it was not a success

    pandaboard(~/src) % go run issue4422.go
    panic: runtime error: invalid memory address or nil pointer dereference
    [signal 0xb code=0x1 addr=0x0 pc=0xffff0f70]

    goroutine 1 [running]:

    goroutine 2 [syscall]:
    created by runtime.main
    /home/dfc/go/src/pkg/runtime/proc.c:225
    exit status 2

    At this point I'm not really adding anything to this CL so I will remove
    my name from the issue and others can use this CL as a starting point.

    https://codereview.appspot.com/6939056/
  • Minux at Dec 20, 2012 at 6:37 pm

    On Thu, Dec 20, 2012 at 1:40 PM, wrote:

    @minux,

    I applied your suggestion but on my pandaboard it was not a succes
    my bad.
    you need move the line "MOVW.W R14,-4(R13) // just for the backtracer"
    below
    the line "MOVW addr+0(FP), R1", as the linker is confused by the our
    manipulation
    of R13.
  • Russ Cox at Dec 20, 2012 at 7:18 pm
    The whole runtime assumes faults do not happen when LR is live, and
    this code is breaking that assumption. Adding the statements Minux
    suggested should make the assumption true again, so that seems fine to
    me.
  • Minux at Dec 20, 2012 at 9:10 pm

    On Friday, December 21, 2012, Russ Cox wrote:

    The whole runtime assumes faults do not happen when LR is live, and
    i think this assumption is not valid, even for Go programs when the inliner
    is disabled, as the linker won't save lr to stack if it is a leaf func.

    $ cat t.go
    package main

    func f() {
    var a *int
    *a = 1
    }

    func main() {
    f()
    }
    $ go tool 5g -l t.go
    $ go tool 5l -a t.5
    codeblk [0x2000,0x17fe8) at offset 0x1000
    002000 main.f | (3) TEXT main.f+0(SB),$-4-0
    002000 e3a02000 | (4) MOVW $0,R2
    002004 e1a00002 | (5) MOVW R2,R0
    002008 e3a01001 | (5) MOVW $1,R1
    00200c e5821000 | (5) MOVW R1,0(R2)
    002010 e28ef000 | (6) B ,0(R14)
    002014 main.main | (8) TEXT main.main+0(SB),$0-0
    // snip
    $ ./5.out
    panic: runtime error: invalid memory address or nil pointer dereference
    [signal 0xb code=0x2 addr=0x0 pc=0x200c]

    goroutine 1 [running]:
    main.f()
    /var/mobile/go/t.go:5 +0xc // where is the main.main function?

    goroutine 2 [syscall]:
    created by runtime.main
    /var/mobile/go/src/pkg/runtime/proc.c:226

    i think we need to fix the backtracer to always consider lr for the first
    frame, and then trace the frames on stack. I think this is safe, as we
    do't use lr for any other purpose.

    this code is breaking that assumption. Adding the statements Minux
    suggested should make the assumption true again, so that seems fine to
    me.
  • Russ Cox at Dec 22, 2012 at 3:29 pm

    i think we need to fix the backtracer to always consider lr for the first
    frame, and then trace the frames on stack. I think this is safe, as we
    do't use lr for any other purpose.
    the backtracer already does that. the problem is that if a signal happens
    while lr is live, the extra stack frames laid down by the signal handler
    have nowhere to store the lr.

    right now when a fault happens on arm we make it look like the faulting
    code called sigpanic directly. that makes it easy to backtrace over the
    fault, but it leaves nowhere to put the lr. however, we also never return
    from sigpanic, so we don't have to be 100% accurate about the details as
    long as we keep the backtracer in the loop.

    the current signal_linux_arm.c says:

    // Make it look like a call to the signal func.
    // Have to pass arguments out of band since
    // augmenting the stack frame would break
    // the unwinding code.
    gp->sig = sig;
    gp->sigcode0 = info->si_code;
    gp->sigcode1 = r->fault_address;
    gp->sigpc = r->arm_pc;

    // If this is a leaf function, we do smash LR,
    // but we're not going back there anyway.
    // Don't bother smashing if r->arm_pc is 0,
    // which is probably a call to a nil func: the
    // old link register is more useful in the stack trace.
    if(r->arm_pc != 0)
    r->arm_lr = r->arm_pc;
    // In case we are panicking from external C code
    r->arm_r10 = (uintptr)gp;
    r->arm_r9 = (uintptr)m;
    r->arm_pc = (uintptr)runtime·sigpanic;
    return;

    it looks like we need to make this push the arm lr register onto the stack,
    and then the traceback code can pull it off here:

    // Unwind to next frame.
    pc = lr;
    lr = 0;
    sp = fp;
    fp = nil;
    after that it would do

    if(waspanic)
    lr = *sp++;

    or something like that.

    russ
  • Minux at Dec 22, 2012 at 3:40 pm

    On Sat, Dec 22, 2012 at 11:29 PM, Russ Cox wrote:

    the backtracer already does that. the problem is that if a signal happens
    while lr is live, the extra stack frames laid down by the signal handler
    have nowhere to store the lr.
    can we store the lr into (otherwise unused) m->tls array in
    runtime.sighandler()?
    assuming only one signal (or less than 8 signals) could occur in a row.
  • Russ Cox at Dec 22, 2012 at 3:45 pm
    i think you should store it on the stack.
    sigpanic can trigger a panic, which can trigger deferred calls, which can
    then panic, ad infinitum.
    also instead of panicking those deferred calls can block, making the
    goroutine reschedule, so it might end up executing on different m's
    throughout.

    storing lr on the stack keeps it attached to the goroutine and just
    requires helping the backtrace routines get past it. they already know
    about waspanic, so it's not a big deal to add a few more lines there.

    russ
  • Minux at Dec 22, 2012 at 3:50 pm

    On Sat, Dec 22, 2012 at 11:45 PM, Russ Cox wrote:

    i think you should store it on the stack.
    sigpanic can trigger a panic, which can trigger deferred calls, which can
    then panic, ad infinitum.
    also instead of panicking those deferred calls can block, making the
    goroutine reschedule, so it might end up executing on different m's
    throughout.

    storing lr on the stack keeps it attached to the goroutine and just
    requires helping the backtrace routines get past it. they already know
    about waspanic, so it's not a big deal to add a few more lines there.
    sure. will prepare a CL.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 18, '12 at 11:38p
activeDec 22, '12 at 3:50p
posts15
users3
websitegolang.org

3 users in discussion

Minux: 8 posts Russ Cox: 4 posts Dave: 3 posts

People

Translate

site design / logo © 2022 Grokbase