From b1d1ec9183590f43ea180e101a4f53ab29779e9c Mon Sep 17 00:00:00 2001 From: Elias Naur Date: Sun, 29 Apr 2018 16:29:43 +0200 Subject: [PATCH] runtime: perform crashes outside systemstack CL 93658 moved stack trace printing inside a systemstack call to sidestep complexity in case the runtime is in a inconsistent state. Unfortunately, debuggers generating backtraces for a Go panic will be confused and come up with a technical correct but useless stack. This CL moves just the crash performing - typically a SIGABRT signal - outside the systemstack call to improve backtraces. Unfortunately, the crash function now needs to be marked nosplit and that triggers the no split stackoverflow check. To work around that, split fatalpanic in two: fatalthrow for runtime.throw and fatalpanic for runtime.gopanic. Only Go panics really needs crashes on the right stack and there is enough stack for gopanic. Example program: package main import "runtime/debug" func main() { debug.SetTraceback("crash") crash() } func crash() { panic("panic!") } Before: (lldb) bt * thread #1, name = 'simple', stop reason = signal SIGABRT * frame #0: 0x000000000044ffe4 simple`runtime.raise at :1 frame #1: 0x0000000000438cfb simple`runtime.dieFromSignal(sig=) at signal_unix.go:424 frame #2: 0x0000000000438ec9 simple`runtime.crash at signal_unix.go:525 frame #3: 0x00000000004268f5 simple`runtime.dopanic_m(gp=, pc=, sp=) at panic.go:758 frame #4: 0x000000000044bead simple`runtime.fatalpanic.func1 at panic.go:657 frame #5: 0x000000000044d066 simple`runtime.systemstack at :1 frame #6: 0x000000000042a980 simple at proc.go:1094 frame #7: 0x0000000000438ec9 simple`runtime.crash at signal_unix.go:525 frame #8: 0x00000000004268f5 simple`runtime.dopanic_m(gp=, pc=, sp=) at panic.go:758 frame #9: 0x000000000044bead simple`runtime.fatalpanic.func1 at panic.go:657 frame #10: 0x000000000044d066 simple`runtime.systemstack at :1 frame #11: 0x000000000042a980 simple at proc.go:1094 frame #12: 0x00000000004268f5 simple`runtime.dopanic_m(gp=, pc=, sp=) at panic.go:758 frame #13: 0x000000000044bead simple`runtime.fatalpanic.func1 at panic.go:657 frame #14: 0x000000000044d066 simple`runtime.systemstack at :1 frame #15: 0x000000000042a980 simple at proc.go:1094 frame #16: 0x000000000044bead simple`runtime.fatalpanic.func1 at panic.go:657 frame #17: 0x000000000044d066 simple`runtime.systemstack at :1 After: (lldb) bt * thread #7, stop reason = signal SIGABRT * frame #0: 0x0000000000450024 simple`runtime.raise at :1 frame #1: 0x0000000000438d1b simple`runtime.dieFromSignal(sig=) at signal_unix.go:424 frame #2: 0x0000000000438ee9 simple`runtime.crash at signal_unix.go:525 frame #3: 0x00000000004264e3 simple`runtime.fatalpanic(msgs=) at panic.go:664 frame #4: 0x0000000000425f1b simple`runtime.gopanic(e=) at panic.go:537 frame #5: 0x0000000000470c62 simple`main.crash at simple.go:11 frame #6: 0x0000000000470c00 simple`main.main at simple.go:6 frame #7: 0x0000000000427be7 simple`runtime.main at proc.go:198 frame #8: 0x000000000044ef91 simple`runtime.goexit at :1 Updates #22716 Change-Id: Ib5fa35c13662c1dac2f1eac8b59c4a5824b98d92 Reviewed-on: https://go-review.googlesource.com/110065 Run-TryBot: Elias Naur TryBot-Result: Gobot Gobot Reviewed-by: Austin Clements --- src/runtime/os_nacl.go | 1 + src/runtime/os_plan9.go | 1 + src/runtime/panic.go | 59 ++++++++++++++++++++++------- src/runtime/runtime-gdb_test.go | 66 +++++++++++++++++++++++++++++++++ src/runtime/signal_unix.go | 1 + src/runtime/signal_windows.go | 1 + 6 files changed, 116 insertions(+), 13 deletions(-) diff --git a/src/runtime/os_nacl.go b/src/runtime/os_nacl.go index e2c3ef5e3dc..23ab03b953e 100644 --- a/src/runtime/os_nacl.go +++ b/src/runtime/os_nacl.go @@ -131,6 +131,7 @@ func signame(sig uint32) string { return sigtable[sig].name } +//go:nosplit func crash() { *(*int32)(nil) = 0 } diff --git a/src/runtime/os_plan9.go b/src/runtime/os_plan9.go index 7d557eb9434..9f41c5ac83a 100644 --- a/src/runtime/os_plan9.go +++ b/src/runtime/os_plan9.go @@ -296,6 +296,7 @@ func osinit() { notify(unsafe.Pointer(funcPC(sigtramp))) } +//go:nosplit func crash() { notify(nil) *(*int)(nil) = 0 diff --git a/src/runtime/panic.go b/src/runtime/panic.go index 3abcf9045be..9ba7e1063f3 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -586,7 +586,7 @@ func throw(s string) { if gp.m.throwing == 0 { gp.m.throwing = 1 } - fatalpanic(nil) + fatalthrow() *(*int)(nil) = 0 // not reached } @@ -627,18 +627,43 @@ func recovery(gp *g) { gogo(&gp.sched) } -// fatalpanic implements an unrecoverable panic. It freezes the -// system, prints panic messages if msgs != nil, prints stack traces -// starting from its caller, and terminates the process. +// fatalthrow implements an unrecoverable runtime throw. It freezes the +// system, prints stack traces starting from its caller, and terminates the +// process. // -// If msgs != nil, it also decrements runningPanicDefers once main is -// blocked from exiting. +//go:nosplit +func fatalthrow() { + pc := getcallerpc() + sp := getcallersp() + gp := getg() + // Switch to the system stack to avoid any stack growth, which + // may make things worse if the runtime is in a bad state. + systemstack(func() { + startpanic_m() + + if dopanic_m(gp, pc, sp) { + // crash uses a decent amount of nosplit stack and we're already + // low on stack in throw, so crash on the system stack (unlike + // fatalpanic). + crash() + } + + exit(2) + }) + + *(*int)(nil) = 0 // not reached +} + +// fatalpanic implements an unrecoverable panic. It is like fatalthrow, except +// that if msgs != nil, fatalpanic also prints panic messages and decrements +// runningPanicDefers once main is blocked from exiting. // //go:nosplit func fatalpanic(msgs *_panic) { pc := getcallerpc() sp := getcallersp() gp := getg() + var docrash bool // Switch to the system stack to avoid any stack growth, which // may make things worse if the runtime is in a bad state. systemstack(func() { @@ -654,8 +679,20 @@ func fatalpanic(msgs *_panic) { printpanics(msgs) } - dopanic_m(gp, pc, sp) // should never return + docrash = dopanic_m(gp, pc, sp) }) + + if docrash { + // By crashing outside the above systemstack call, debuggers + // will not be confused when generating a backtrace. + // Function crash is marked nosplit to avoid stack growth. + crash() + } + + systemstack(func() { + exit(2) + }) + *(*int)(nil) = 0 // not reached } @@ -713,7 +750,7 @@ func startpanic_m() bool { var didothers bool var deadlock mutex -func dopanic_m(gp *g, pc, sp uintptr) { +func dopanic_m(gp *g, pc, sp uintptr) bool { if gp.sig != 0 { signame := signame(gp.sig) if signame != "" { @@ -754,11 +791,7 @@ func dopanic_m(gp *g, pc, sp uintptr) { lock(&deadlock) } - if docrash { - crash() - } - - exit(2) + return docrash } // canpanic returns false if a signal should throw instead of diff --git a/src/runtime/runtime-gdb_test.go b/src/runtime/runtime-gdb_test.go index cae12e8af22..a6dfd64abc5 100644 --- a/src/runtime/runtime-gdb_test.go +++ b/src/runtime/runtime-gdb_test.go @@ -478,3 +478,69 @@ func TestGdbConst(t *testing.T) { t.Fatalf("output mismatch") } } + +const panicSource = ` +package main + +import "runtime/debug" + +func main() { + debug.SetTraceback("crash") + crash() +} + +func crash() { + panic("panic!") +} +` + +// TestGdbPanic tests that gdb can unwind the stack correctly +// from SIGABRTs from Go panics. +func TestGdbPanic(t *testing.T) { + checkGdbEnvironment(t) + t.Parallel() + checkGdbVersion(t) + + dir, err := ioutil.TempDir("", "go-build") + if err != nil { + t.Fatalf("failed to create temp directory: %v", err) + } + defer os.RemoveAll(dir) + + // Build the source code. + src := filepath.Join(dir, "main.go") + err = ioutil.WriteFile(src, []byte(panicSource), 0644) + if err != nil { + t.Fatalf("failed to create file: %v", err) + } + cmd := exec.Command(testenv.GoToolPath(t), "build", "-o", "a.exe") + cmd.Dir = dir + out, err := testenv.CleanCmdEnv(cmd).CombinedOutput() + if err != nil { + t.Fatalf("building source %v\n%s", err, out) + } + + // Execute gdb commands. + args := []string{"-nx", "-batch", + "-iex", "add-auto-load-safe-path " + filepath.Join(runtime.GOROOT(), "src", "runtime"), + "-ex", "set startup-with-shell off", + "-ex", "run", + "-ex", "backtrace", + filepath.Join(dir, "a.exe"), + } + got, _ := exec.Command("gdb", args...).CombinedOutput() + + // Check that the backtrace matches the source code. + bt := []string{ + `crash`, + `main`, + } + for _, name := range bt { + s := fmt.Sprintf("#.* .* in main\\.%v", name) + re := regexp.MustCompile(s) + if found := re.Find(got) != nil; !found { + t.Errorf("could not find '%v' in backtrace", s) + t.Fatalf("gdb output:\n%v", string(got)) + } + } +} diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go index 4981c1f6158..8bc73b7a234 100644 --- a/src/runtime/signal_unix.go +++ b/src/runtime/signal_unix.go @@ -509,6 +509,7 @@ func raisebadsignal(sig uint32, c *sigctxt) { setsig(sig, funcPC(sighandler)) } +//go:nosplit func crash() { if GOOS == "darwin" { // OS X core dumps are linear dumps of the mapped memory, diff --git a/src/runtime/signal_windows.go b/src/runtime/signal_windows.go index ad08019fc1b..500b02880d4 100644 --- a/src/runtime/signal_windows.go +++ b/src/runtime/signal_windows.go @@ -224,6 +224,7 @@ func signame(sig uint32) string { return "" } +//go:nosplit func crash() { // TODO: This routine should do whatever is needed // to make the Windows program abort/crash as it