code review 7098050: runtime: dump full stacks on throw and signals

runtime: dump the full stack of a throwing goroutine
Useful for debugging of runtime bugs.
+ Do not print "stack segment boundary" unless GOTRACEBACK>1.
+ Do not traceback system goroutines unless GOTRACEBACK>1.

[dvyukov, 2013-01-12 13:45:51]

Hello golang-dev@googlegroups.com,

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

[minux1, 2013-01-12 14:14:25]

FTR, this is mostly the patch proposed by Russ here:
https://codereview.appspot.com/7069060/#msg7
The only change is in runtime.dopanic.

LGTM.

[dvyukov, 2013-01-12 14:29:37]

Perhaps we can make it finer grained -- set the flag in signal handlers only on
SIGSEGV/SIGBUS. And I do not think it is useful for deadlocks, deadlock is the
only case that calls throw on user error.
What do you think?

[bradfitz, 2013-01-12 16:55:59]

I agree that user errors (a deadlock) shouldn't result in tons of
scary-looking error spew.

On Sat, Jan 12, 2013 at 6:29 AM, <dvyukov@google.com> wrote:

> Perhaps we can make it finer grained -- set the flag in signal handlers
> only on SIGSEGV/SIGBUS. And I do not think it is useful for deadlocks,
> deadlock is the only case that calls throw on user error.
> What do you think?
>
>
>
https://codereview.appspot.**com/7098050/<https://codereview.appspot.com/7098...
>

[minux1, 2013-01-12 16:59:18]

On Sat, Jan 12, 2013 at 10:29 PM, <dvyukov@google.com> wrote:

> Perhaps we can make it finer grained -- set the flag in signal handlers
> only on SIGSEGV/SIGBUS. And I do not think it is useful for deadlocks,
> deadlock is the only case that calls throw on user error.
> What do you think?
>
I think we can restrict it to only internally generated panics (runtime
throws
and deadly signals that might be due to faults in runtime/toolchain (e.g.
SIGILL, SIGABRT, and perhaps SIGSEGV/SIGBUS).

yes, i agree runtime stack frames in deadlocks is not necessary, and if
they want, they can always use GOTRACEBACK=2 directly.

[akumar_mail.nanosouffle.net, 2013-01-13 05:12:43]

UNIX signals don't necessarily map one-to-one and onto Plan 9
notes from the kernel; are other options available perhaps?

On 12 January 2013 08:58, minux <minux.ma@gmail.com> wrote:
>
> On Sat, Jan 12, 2013 at 10:29 PM, <dvyukov@google.com> wrote:
>>
>> Perhaps we can make it finer grained -- set the flag in signal handlers
>> only on SIGSEGV/SIGBUS. And I do not think it is useful for deadlocks,
>> deadlock is the only case that calls throw on user error.
>> What do you think?
>
> I think we can restrict it to only internally generated panics (runtime
> throws
> and deadly signals that might be due to faults in runtime/toolchain (e.g.
> SIGILL, SIGABRT, and perhaps SIGSEGV/SIGBUS).
>
> yes, i agree runtime stack frames in deadlocks is not necessary, and if
> they want, they can always use GOTRACEBACK=2 directly.

[dvyukov, 2013-01-13 14:14:36]

On 2013/01/13 05:12:43, akumar_mail.nanosouffle.net wrote:
> UNIX signals don't necessarily map one-to-one and onto Plan 9
> notes from the kernel; are other options available perhaps?

I am thinking about setting the flag in thread_linux/plan9/mac.c, so the
platform-specific code can decided for what signals dump full stack.


> On 12 January 2013 08:58, minux <mailto:minux.ma@gmail.com> wrote:
> >
> > On Sat, Jan 12, 2013 at 10:29 PM, <mailto:dvyukov@google.com> wrote:
> >>
> >> Perhaps we can make it finer grained -- set the flag in signal handlers
> >> only on SIGSEGV/SIGBUS. And I do not think it is useful for deadlocks,
> >> deadlock is the only case that calls throw on user error.
> >> What do you think?
> >
> > I think we can restrict it to only internally generated panics (runtime
> > throws
> > and deadly signals that might be due to faults in runtime/toolchain (e.g.
> > SIGILL, SIGABRT, and perhaps SIGSEGV/SIGBUS).
> >
> > yes, i agree runtime stack frames in deadlocks is not necessary, and if
> > they want, they can always use GOTRACEBACK=2 directly.

[dvyukov, 2013-01-13 14:16:24]

On 2013/01/12 16:55:59, bradfitz wrote:
> I agree that user errors (a deadlock) shouldn't result in tons of
> scary-looking error spew.

OK, I will exclude deadlocks and nil derefs. But then we still need to set
GOTRACEBACK=2 in run.bash, because a lot of runtime bugs (GC, the issue with
lock-free stack on 386) result in nil derefs.



> On Sat, Jan 12, 2013 at 6:29 AM, <mailto:dvyukov@google.com> wrote:
> 
> > Perhaps we can make it finer grained -- set the flag in signal handlers
> > only on SIGSEGV/SIGBUS. And I do not think it is useful for deadlocks,
> > deadlock is the only case that calls throw on user error.
> > What do you think?
> >
> >
> >
>
https://codereview.appspot.**com/7098050/%3Chttps://codereview.appspot.com/70...>
> >

[dvyukov, 2013-01-21 15:57:42]

PTAL

now it prints full stacks only on runtime.throw() except deadlocks

[minux1, 2013-01-21 16:03:10]

LGTM.

[rsc, 2013-01-22 22:20:36]

I wonder if the throwing variable should be per-M.
It is likely that only the M that received the signal/is throwing matters, and
then only when dumping that M's goroutine.

[dvyukov, 2013-01-23 07:13:19]

On 2013/01/22 22:20:36, rsc wrote:
> I wonder if the throwing variable should be per-M.
> It is likely that only the M that received the signal/is throwing matters, and
> then only when dumping that M's goroutine.

Agree.

You do not want to set GOTRACEBACK=2 on bots. But at the same time it would be
useful to see full stacks for nil derefs (previously they were caused by GC bugs
and non-atomic 64-bit atomics). But it's most likely unacceptable for end user
to see the following on nil deref in user code:

goroutine 1 [running]:
runtime.panic(0x4173d0, 0xc20000f200)
	/usr/local/google/home/dvyukov/go2/src/pkg/runtime/panic.c:245 +0xb1
runtime.panicstring(0x449bf3, 0x40bb3c)
	/usr/local/google/home/dvyukov/go2/src/pkg/runtime/panic.c:474 +0xa3
runtime.sigpanic()
	/usr/local/google/home/dvyukov/go2/src/pkg/runtime/thread_linux.c:199 +0x168
main.main()
	/tmp/test.go:5 +0x11
runtime.main()
	/usr/local/google/home/dvyukov/go2/src/pkg/runtime/proc.c:248 +0x92
runtime.goexit()
	/usr/local/google/home/dvyukov/go2/src/pkg/runtime/proc.c:277

So what is your plan?

[dvyukov, 2013-01-27 15:31:01]

PTAL

I've made throwing per-M and now it affects only the current goroutine.
+ hide "stack segment boundary" unless runtime frames are printed.
+ hide system goroutines (timer, scavenger) (I think finalizer goroutine must
not be hidden)

Now instead of:

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

goroutine 1 [running]:
main.foo(0x0, 0x1, 0x0, 0x0)
	/tmp/test2.go:21 +0x66
----- stack segment boundary -----
main.foo(0x1, 0x1, 0x0, 0x0)
	/tmp/test2.go:24 +0xbe
----- stack segment boundary -----
main.main()
	/tmp/test2.go:13 +0x98
----- stack segment boundary -----

goroutine 2 [syscall]:
created by runtime.main
	src/pkg/runtime/proc.c:232

goroutine 3 [timer goroutine (idle)]:
created by addtimer
	src/pkg/runtime/ztime_darwin_amd64.c:73

the trace looks as follows:

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

goroutine 1 [running]:
main.foo(0x0, 0x1, 0x0, 0x0)
	/tmp/test2.go:21 +0x66
main.foo(0x1, 0x1, 0x0, 0x0)
	/tmp/test2.go:24 +0xbe
main.main()
	/tmp/test2.go:13 +0x98

[minux1, 2013-01-27 15:46:18]

On 2013/01/27 15:31:01, dvyukov wrote:
> the trace looks as follows:
> 
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0x0 pc=0x2106]
> 
> goroutine 1 [running]:
> main.foo(0x0, 0x1, 0x0, 0x0)
> 	/tmp/test2.go:21 +0x66
> main.foo(0x1, 0x1, 0x0, 0x0)
> 	/tmp/test2.go:24 +0xbe
> main.main()
> 	/tmp/test2.go:13 +0x98
This is much clearer. LGTM.

[rsc, 2013-01-29 00:32:16]

LGTM

Thanks.

[dvyukov, 2013-01-29 10:59:25]

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

runtime: dump the full stack of a throwing goroutine
Useful for debugging of runtime bugs.
+ Do not print "stack segment boundary" unless GOTRACEBACK>1.
+ Do not traceback system goroutines unless GOTRACEBACK>1.

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