code review 6939056: sync/atomic: nil check before cas

sync/atomic: nil check before cas

Fixes issue 4422.

Implements the suggestion by Remy and Minux as described in the issue.

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

goroutine 1 [running]:
sync/atomic.CompareAndSwapUint32(0x0, 0x0, 0x1, 0x2, 0x0, ...)
        /home/dfc/go/src/pkg/sync/atomic/asm_linux_arm.s:33 +0x8
sync.(*Mutex).Lock(0x0, 0x1e1b4)
        /home/dfc/go/src/pkg/sync/mutex.go:43 +0x38
main.main()
        /home/dfc/src/issue4422.go:9 +0x28

[dave_cheney.net, 2012-12-15 12:41:33]

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

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

[minux1, 2012-12-15 12:45:54]

https://codereview.appspot.com/6939056/diff/2001/src/pkg/sync/atomic/asm_linu...
File src/pkg/sync/atomic/asm_linux_arm.s (right):

https://codereview.appspot.com/6939056/diff/2001/src/pkg/sync/atomic/asm_linu...
src/pkg/sync/atomic/asm_linux_arm.s:130: TEXT ·CompareAndSwapUint64(SB),7,$-4
i think you also need to do the same here.

[rsc, 2012-12-15 18:15:55]

Looks good once Minux is happy.

[minux1, 2012-12-15 19:02:37]

https://codereview.appspot.com/6939056/diff/2001/src/pkg/sync/atomic/asm_linu...
File src/pkg/sync/atomic/asm_linux_arm.s (right):

https://codereview.appspot.com/6939056/diff/2001/src/pkg/sync/atomic/asm_linu...
src/pkg/sync/atomic/asm_linux_arm.s:83: MOVW	addr+0(FP), R2 // ptr
after some thought, i think we can just add a load from
R2 here (just don't use register R0 or R1 as destination).
the other two cases are both done by our code, and the
backtracer should be able to do its job.

[dave_cheney.net, 2012-12-16 01:23:22]

Thank you for your comments. PTAL.

https://codereview.appspot.com/6939056/diff/2001/src/pkg/sync/atomic/asm_linu...
File src/pkg/sync/atomic/asm_linux_arm.s (right):

https://codereview.appspot.com/6939056/diff/2001/src/pkg/sync/atomic/asm_linu...
src/pkg/sync/atomic/asm_linux_arm.s:83: MOVW	addr+0(FP), R2 // ptr
On 2012/12/15 19:02:37, minux wrote:
> after some thought, i think we can just add a load from
> R2 here (just don't use register R0 or R1 as destination).
> the other two cases are both done by our code, and the
> backtracer should be able to do its job.

Done. But I did use R0 as the destination as it is overwritten in the following
instruction. This gives us (for cas64)

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x2aa3c]

goroutine 1 [running]:
main.main()
        /home/dfc/src/issue4422.go:9 +0x48

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

[minux1, 2012-12-16 01:32:35]

On Sun, Dec 16, 2012 at 9:23 AM, <dave@cheney.net> wrote:

> Done. But I did use R0 as the destination as it is overwritten in the
> following instruction. This gives us (for cas64)
>
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0x0 pc=0x2aa3c]
>
> goroutine 1 [running]:
> main.main()
>         /home/dfc/src/issue4422.go:9 +0x48
>
> goroutine 2 [syscall]:
> created by runtime.main
>         /home/dfc/go/src/pkg/runtime/**proc.c:225
> exit status 2
>
I overlooked the fact that kernelCAS64 is a local function which won't be
shown in
back traces. One solution is remove all <> from function names, the other
is just
do the check for nil in CompareAndSwapUint64.

[dave_cheney.net, 2012-12-16 01:42:25]

PTAL. Now we get

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x2aaf0]

goroutine 1 [running]:
sync/atomic.CompareAndSwapUint64(0x0, 0x1)
        /home/dfc/go/src/pkg/sync/atomic/asm_linux_arm.s:132 +0x4

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

[minux1, 2012-12-16 01:58:53]

On 2012/12/16 01:42:25, dfc wrote:
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0x0 pc=0x2aaf0]
> 
> goroutine 1 [running]:
> sync/atomic.CompareAndSwapUint64(0x0, 0x1)
>         /home/dfc/go/src/pkg/sync/atomic/asm_linux_arm.s:132 +0x4
If you set GOTRACEBACK=2 does the output change?

https://codereview.appspot.com/6939056/diff/12002/src/pkg/sync/atomic/asm_lin...
File src/pkg/sync/atomic/asm_linux_arm.s (right):

https://codereview.appspot.com/6939056/diff/12002/src/pkg/sync/atomic/asm_lin...
src/pkg/sync/atomic/asm_linux_arm.s:82: MOVW	addr+0(FP), R2	// ptr
why reorder this line?

[dave_cheney.net, 2012-12-16 02:16:34]

Done, and removed all the other cleanups in that file, the diff is now very
short.

loco(~/src) % GOTRACEBACK=2 go run issue4422.go 2>&1 | head -n20
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x2aaf0]

goroutine 1 [running]:
runtime.panic(0x2c008, 0x10500100)
        /home/dfc/go/src/pkg/runtime/panic.c:154 +0x120
runtime.panicstring(0x53d73, 0x0)
        /home/dfc/go/src/pkg/runtime/panic.c:384 +0x8c
runtime.sigpanic()
        /home/dfc/go/src/pkg/runtime/thread_linux.c:199 +0x114
sync/atomic.CompareAndSwapUint64(0x0, 0x1)
        /home/dfc/go/src/pkg/sync/atomic/asm_linux_arm.s:132 +0x4
runtime.main()
        /home/dfc/go/src/pkg/runtime/proc.c:248 +0x90
runtime.main()
        /home/dfc/go/src/pkg/runtime/proc.c:248 +0x90
schedule(0x0, 0x1)
        /home/dfc/go/src/pkg/runtime/proc.c:876
runtime.main()
        /home/dfc/go/src/pkg/runtime/proc.c:225

https://codereview.appspot.com/6939056/diff/12002/src/pkg/sync/atomic/asm_lin...
File src/pkg/sync/atomic/asm_linux_arm.s (right):

https://codereview.appspot.com/6939056/diff/12002/src/pkg/sync/atomic/asm_lin...
src/pkg/sync/atomic/asm_linux_arm.s:82: MOVW	addr+0(FP), R2	// ptr
On 2012/12/16 01:58:53, minux wrote:
> why reorder this line?

Whoops. fixed

[dave_cheney.net, 2012-12-16 02:16:56]

Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2012-12-16 02:23:40]

On Sun, Dec 16, 2012 at 10:16 AM, <dave@cheney.net> wrote:

> loco(~/src) % GOTRACEBACK=2 go run issue4422.go 2>&1 | head -n20
>
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0x0 pc=0x2aaf0]
>
> goroutine 1 [running]:
> runtime.panic(0x2c008, 0x10500100)
>         /home/dfc/go/src/pkg/runtime/**panic.c:154 +0x120
> runtime.panicstring(0x53d73, 0x0)
>         /home/dfc/go/src/pkg/runtime/**panic.c:384 +0x8c
> runtime.sigpanic()
>         /home/dfc/go/src/pkg/runtime/**thread_linux.c:199 +0x114
>
> sync/atomic.**CompareAndSwapUint64(0x0, 0x1)
>         /home/dfc/go/src/pkg/sync/**atomic/asm_linux_arm.s:132 +0x4
>
why no main.main() here? the backtracer is still confused.

>  runtime.main()
>         /home/dfc/go/src/pkg/runtime/**proc.c:248 +0x90
> runtime.main()
>         /home/dfc/go/src/pkg/runtime/**proc.c:248 +0x90

schedule(0x0, 0x1)
>         /home/dfc/go/src/pkg/runtime/**proc.c:876
> runtime.main()
>         /home/dfc/go/src/pkg/runtime/**proc.c:225

[dave_cheney.net, 2012-12-18 23:37:57]

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

[rsc, 2012-12-18 23:52:37]

What if you try main calling f calling g calling CompareAndSwapUint64?

Russ

[minux1, 2012-12-19 19:06:52]

On Wed, Dec 19, 2012 at 7:52 AM, Russ Cox <rsc@golang.org> 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

[minux1, 2012-12-19 20:58:56]

On Wed, Dec 19, 2012 at 7:37 AM, <dave@cheney.net> 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

[minux1, 2012-12-19 21:01:03]

On Thu, Dec 20, 2012 at 4:58 AM, minux <minux.ma@gmail.com> 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.net, 2012-12-19 21:03:13]

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 <minux.ma@gmail.com> wrote:
>
> On Thu, Dec 20, 2012 at 4:58 AM, minux <minux.ma@gmail.com> 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.

[minux1, 2012-12-19 21:07:30]

On Thu, Dec 20, 2012 at 5:03 AM, Dave Cheney <dave@cheney.net> 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_cheney.net, 2012-12-20 05:40:28]

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

[minux1, 2012-12-20 18:37:53]

On Thu, Dec 20, 2012 at 1:40 PM, <dave@cheney.net> 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.

[rsc, 2012-12-20 19:18:22]

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.

[minux1, 2012-12-20 21:10:39]

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.

[rsc, 2012-12-22 15:29:38]

> 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

[minux1, 2012-12-22 15:40:37]

On Sat, Dec 22, 2012 at 11:29 PM, Russ Cox <rsc@golang.org> 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.

[rsc, 2012-12-22 15:45:26]

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

[minux1, 2012-12-22 15:50:33]

On Sat, Dec 22, 2012 at 11:45 PM, Russ Cox <rsc@golang.org> 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.

[rsc, 2013-02-01 19:57:57]

Minux, still planning to prepare the LR CL?

[rsc, 2013-02-01 19:58:01]

Minux, still planning to prepare the LR CL?

[minux1, 2013-02-03 20:01:12]

On Sat, Feb 2, 2013 at 3:57 AM, <rsc@golang.org> wrote:

> Minux, still planning to prepare the LR CL?
>
https://codereview.appspot.com/7289047