runtime: fix line number in first stack frame in printed stack trace

Originally traceback was only used for printing the stack
when an unexpected signal came in. In that case, the
initial PC is taken from the signal and should be used
unaltered. For the callers, the PC is the return address,
which might be on the line after the call; we subtract 1
to get to the CALL instruction.

Traceback is now used for a variety of things, and for
almost all of those the initial PC is a return address,
whether from getcallerpc, or gp->sched.pc, or gp->syscallpc.
In those cases, we need to subtract 1 from this initial PC,
but the traceback code had a hard rule "never subtract 1
from the initial PC", left over from the signal handling days.

Change gentraceback to take a flag that specifies whether
we are tracing a trap.

Change traceback to default to "starting with a return PC",
which is the overwhelmingly common case.

Add tracebacktrap, like traceback but starting with a trap PC.

Use tracebacktrap in signal handlers.

Fixes #7690.

LGTM=iant, r
R=r, iant
CC=golang-codereviews
https://golang.org/cl/167810044

diff --git a/src/runtime/heapdump.c b/src/runtime/heapdump.c
index 71da419f150ec5511fe74c5ded597b81feeb3a5c..94a4bd2be5dac300bf4a00b67320a7286f500f50 100644 (file)
--- a/src/runtime/heapdump.c
+++ b/src/runtime/heapdump.c
@@ -413,7 +413,7 @@ dumpgoroutine(G *gp)
        child.sp = nil;
        child.depth = 0;
        fn = dumpframe;
-       runtime·gentraceback(pc, sp, lr, gp, 0, nil, 0x7fffffff, &fn, &child, false);
+       runtime·gentraceback(pc, sp, lr, gp, 0, nil, 0x7fffffff, &fn, &child, 0);
 
        // dump defer & panic records
        for(d = gp->defer; d != nil; d = d->link) {

diff --git a/src/runtime/mgc0.c b/src/runtime/mgc0.c
index 1b41bf9a79419627c791092102922c05e737f57b..7754bad89d437ea27f5881a6a7d6214419ebe846 100644 (file)
--- a/src/runtime/mgc0.c
+++ b/src/runtime/mgc0.c
@@ -774,7 +774,7 @@ scanstack(G *gp)
                runtime·throw("can't scan gchelper stack");
 
        fn = scanframe;
-       runtime·gentraceback(~(uintptr)0, ~(uintptr)0, 0, gp, 0, nil, 0x7fffffff, &fn, nil, false);
+       runtime·gentraceback(~(uintptr)0, ~(uintptr)0, 0, gp, 0, nil, 0x7fffffff, &fn, nil, 0);
        runtime·tracebackdefers(gp, &fn, nil);
 }
 
@@ -1964,7 +1964,7 @@ runtime·getgcmask(byte *p, Type *t, byte **mask, uintptr *len)
        frame.fn = nil;
        frame.sp = (uintptr)p;
        cb = getgcmaskcb;
-       runtime·gentraceback(g->m->curg->sched.pc, g->m->curg->sched.sp, 0, g->m->curg, 0, nil, 1000, &cb, &frame, false);
+       runtime·gentraceback(g->m->curg->sched.pc, g->m->curg->sched.sp, 0, g->m->curg, 0, nil, 1000, &cb, &frame, 0);
        if(frame.fn != nil) {
                Func *f;
                StackMap *stackmap;

diff --git a/src/runtime/mprof.go b/src/runtime/mprof.go
index 803da56670bd9eb1215d92ef7d7894db1cd2656b..d64e3be6952dbf86c6fd3dad4912a811d7c945c5 100644 (file)
--- a/src/runtime/mprof.go
+++ b/src/runtime/mprof.go
@@ -562,7 +562,7 @@ func GoroutineProfile(p []StackRecord) (n int, ok bool) {
 }
 
 func saveg(pc, sp uintptr, gp *g, r *StackRecord) {
-       n := gentraceback(pc, sp, 0, gp, 0, &r.Stack0[0], len(r.Stack0), nil, nil, false)
+       n := gentraceback(pc, sp, 0, gp, 0, &r.Stack0[0], len(r.Stack0), nil, nil, 0)
        if n < len(r.Stack0) {
                r.Stack0[n] = 0
        }

diff --git a/src/runtime/os_plan9_386.c b/src/runtime/os_plan9_386.c
index 3490862244485d011608e67a07b6f8b223afccb3..42c6d161c7dfc2a6ef92869effe742a7a85b5ec2 100644 (file)
--- a/src/runtime/os_plan9_386.c
+++ b/src/runtime/os_plan9_386.c
@@ -114,7 +114,7 @@ Throw:
 
        if(runtime·gotraceback(&crash)) {
                runtime·goroutineheader(gp);
-               runtime·traceback(ureg->pc, ureg->sp, 0, gp);
+               runtime·tracebacktrap(ureg->pc, ureg->sp, 0, gp);
                runtime·tracebackothers(gp);
                runtime·printf("\n");
                runtime·dumpregs(ureg);

diff --git a/src/runtime/os_plan9_amd64.c b/src/runtime/os_plan9_amd64.c
index 6b0f8ae3a217f3fd4b4137225cd1acac30501071..a9dc0eb966b67339567cdd4bb1f2fbea63a3a61d 100644 (file)
--- a/src/runtime/os_plan9_amd64.c
+++ b/src/runtime/os_plan9_amd64.c
@@ -122,7 +122,7 @@ Throw:
 
        if(runtime·gotraceback(&crash)) {
                runtime·goroutineheader(gp);
-               runtime·traceback(ureg->ip, ureg->sp, 0, gp);
+               runtime·tracebacktrap(ureg->ip, ureg->sp, 0, gp);
                runtime·tracebackothers(gp);
                runtime·printf("\n");
                runtime·dumpregs(ureg);

diff --git a/src/runtime/os_windows_386.c b/src/runtime/os_windows_386.c
index 213582799b13604fc36a08cadfce9a9d603bbd89..9962f0dc2e4dde5ee816a11b360fbe14a515f088 100644 (file)
--- a/src/runtime/os_windows_386.c
+++ b/src/runtime/os_windows_386.c
@@ -97,7 +97,7 @@ runtime·lastcontinuehandler(ExceptionRecord *info, Context *r, G *gp)
        runtime·printf("\n");
 
        if(runtime·gotraceback(&crash)){
-               runtime·traceback(r->Eip, r->Esp, 0, gp);
+               runtime·tracebacktrap(r->Eip, r->Esp, 0, gp);
                runtime·tracebackothers(gp);
                runtime·dumpregs(r);
        }

diff --git a/src/runtime/os_windows_amd64.c b/src/runtime/os_windows_amd64.c
index b96cf70d1ec0726ee4930e87e2043c722d0a4bf7..e4617e4cefa8fc7761c29a6d0bab992dad18a9c9 100644 (file)
--- a/src/runtime/os_windows_amd64.c
+++ b/src/runtime/os_windows_amd64.c
@@ -119,7 +119,7 @@ runtime·lastcontinuehandler(ExceptionRecord *info, Context *r, G *gp)
        runtime·printf("\n");
 
        if(runtime·gotraceback(&crash)){
-               runtime·traceback(r->Rip, r->Rsp, 0, gp);
+               runtime·tracebacktrap(r->Rip, r->Rsp, 0, gp);
                runtime·tracebackothers(gp);
                runtime·dumpregs(r);
        }

diff --git a/src/runtime/proc.c b/src/runtime/proc.c
index 52f7ef3a5bfbca96f63904fab25e35e4e3d62207..b46f67065a528b92182b5ef29565250eb4fa3a8f 100644 (file)
--- a/src/runtime/proc.c
+++ b/src/runtime/proc.c
@@ -2532,7 +2532,7 @@ runtime·sigprof(uint8 *pc, uint8 *sp, uint8 *lr, G *gp, M *mp)
 
        n = 0;
        if(traceback)
-               n = runtime·gentraceback((uintptr)pc, (uintptr)sp, (uintptr)lr, gp, 0, stk, nelem(stk), nil, nil, false);
+               n = runtime·gentraceback((uintptr)pc, (uintptr)sp, (uintptr)lr, gp, 0, stk, nelem(stk), nil, nil, TraceTrap);
        if(!traceback || n <= 0) {
                // Normal traceback is impossible or has failed.
                // See if it falls into several common cases.
@@ -2542,13 +2542,13 @@ runtime·sigprof(uint8 *pc, uint8 *sp, uint8 *lr, G *gp, M *mp)
                        // Cgo, we can't unwind and symbolize arbitrary C code,
                        // so instead collect Go stack that leads to the cgo call.
                        // This is especially important on windows, since all syscalls are cgo calls.
-                       n = runtime·gentraceback(mp->curg->syscallpc, mp->curg->syscallsp, 0, mp->curg, 0, stk, nelem(stk), nil, nil, false);
+                       n = runtime·gentraceback(mp->curg->syscallpc, mp->curg->syscallsp, 0, mp->curg, 0, stk, nelem(stk), nil, nil, 0);
                }
 #ifdef GOOS_windows
                if(n == 0 && mp->libcallg != nil && mp->libcallpc != 0 && mp->libcallsp != 0) {
                        // Libcall, i.e. runtime syscall on windows.
                        // Collect Go stack that leads to the call.
-                       n = runtime·gentraceback(mp->libcallpc, mp->libcallsp, 0, mp->libcallg, 0, stk, nelem(stk), nil, nil, false);
+                       n = runtime·gentraceback(mp->libcallpc, mp->libcallsp, 0, mp->libcallg, 0, stk, nelem(stk), nil, nil, 0);
                }
 #endif
                if(n == 0) {

diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h
index 2a607400636ad72d80c5bfd95bba92e8ca2980f8..977c4547dfbacf55f2627968e9c4d3fd39c72016 100644 (file)
--- a/src/runtime/runtime.h
+++ b/src/runtime/runtime.h
@@ -719,9 +719,15 @@ struct Stkframe
        BitVector*      argmap; // force use of this argmap
 };
 
-intgo  runtime·gentraceback(uintptr, uintptr, uintptr, G*, intgo, uintptr*, intgo, bool(**)(Stkframe*, void*), void*, bool);
+enum
+{
+       TraceRuntimeFrames = 1<<0, // include frames for internal runtime functions.
+       TraceTrap = 1<<1, // the initial PC, SP are from a trap, not a return PC from a call
+};
+intgo  runtime·gentraceback(uintptr, uintptr, uintptr, G*, intgo, uintptr*, intgo, bool(**)(Stkframe*, void*), void*, uintgo);
 void   runtime·tracebackdefers(G*, bool(**)(Stkframe*, void*), void*);
 void   runtime·traceback(uintptr pc, uintptr sp, uintptr lr, G* gp);
+void   runtime·tracebacktrap(uintptr pc, uintptr sp, uintptr lr, G* gp);
 void   runtime·tracebackothers(G*);
 bool   runtime·haszeroargs(uintptr pc);
 bool   runtime·topofstack(Func*);

diff --git a/src/runtime/signal_386.c b/src/runtime/signal_386.c
index d55e3045288f673f2c1dc3115f3e878b7f17179d..30a7488bd786f5dd2ba4b2144e9a805393bdb433 100644 (file)
--- a/src/runtime/signal_386.c
+++ b/src/runtime/signal_386.c
@@ -109,7 +109,7 @@ runtime·sighandler(int32 sig, Siginfo *info, void *ctxt, G *gp)
 
        if(runtime·gotraceback(&crash)){
                runtime·goroutineheader(gp);
-               runtime·traceback(SIG_EIP(info, ctxt), SIG_ESP(info, ctxt), 0, gp);
+               runtime·tracebacktrap(SIG_EIP(info, ctxt), SIG_ESP(info, ctxt), 0, gp);
                runtime·tracebackothers(gp);
                runtime·printf("\n");
                runtime·dumpregs(info, ctxt);

diff --git a/src/runtime/signal_amd64x.c b/src/runtime/signal_amd64x.c
index 44e68cecfccf8e8717bc3c12ac998e36f2c8ce8e..feb4afcce3481a64e1a459d45a50c059d38ef000 100644 (file)
--- a/src/runtime/signal_amd64x.c
+++ b/src/runtime/signal_amd64x.c
@@ -143,7 +143,7 @@ runtime·sighandler(int32 sig, Siginfo *info, void *ctxt, G *gp)
 
        if(runtime·gotraceback(&crash)){
                runtime·goroutineheader(gp);
-               runtime·traceback(SIG_RIP(info, ctxt), SIG_RSP(info, ctxt), 0, gp);
+               runtime·tracebacktrap(SIG_RIP(info, ctxt), SIG_RSP(info, ctxt), 0, gp);
                runtime·tracebackothers(gp);
                runtime·printf("\n");
                runtime·dumpregs(info, ctxt);

diff --git a/src/runtime/signal_arm.c b/src/runtime/signal_arm.c
index 3571cf3ac6715a38782a9bbc0e549dfb84ab46cf..afad5e7d166820093ede00e35072f92b01e7f18c 100644 (file)
--- a/src/runtime/signal_arm.c
+++ b/src/runtime/signal_arm.c
@@ -108,7 +108,7 @@ runtime·sighandler(int32 sig, Siginfo *info, void *ctxt, G *gp)
 
        if(runtime·gotraceback(&crash)){
                runtime·goroutineheader(gp);
-               runtime·traceback(SIG_PC(info, ctxt), SIG_SP(info, ctxt), SIG_LR(info, ctxt), gp);
+               runtime·tracebacktrap(SIG_PC(info, ctxt), SIG_SP(info, ctxt), SIG_LR(info, ctxt), gp);
                runtime·tracebackothers(gp);
                runtime·printf("\n");
                runtime·dumpregs(info, ctxt);

diff --git a/src/runtime/stack.c b/src/runtime/stack.c
index ed8f4f872799be664a90c501de68094a8af7b07d..072bc242bc2a5d3cad6bd004a2d2011b17fd5f92 100644 (file)
--- a/src/runtime/stack.c
+++ b/src/runtime/stack.c
@@ -620,7 +620,7 @@ copystack(G *gp, uintptr newsize)
        adjinfo.old = old;
        adjinfo.delta = new.hi - old.hi;
        cb = adjustframe;
-       runtime·gentraceback(~(uintptr)0, ~(uintptr)0, 0, gp, 0, nil, 0x7fffffff, &cb, &adjinfo, false);
+       runtime·gentraceback(~(uintptr)0, ~(uintptr)0, 0, gp, 0, nil, 0x7fffffff, &cb, &adjinfo, 0);
        
        // adjust other miscellaneous things that have pointers into stacks.
        adjustctxt(gp, &adjinfo);

diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go
index 24dc3eea951d6831e94425ef57d48a59c5ac12f1..834435b400d3d0be0bff6f4f499cd7090bd63445 100644 (file)
--- a/src/runtime/traceback.go
+++ b/src/runtime/traceback.go
@@ -96,7 +96,7 @@ func tracebackdefers(gp *g, callback func(*stkframe, unsafe.Pointer) bool, v uns
 // the runtime.Callers function (pcbuf != nil), as well as the garbage
 // collector (callback != nil).  A little clunky to merge these, but avoids
 // duplicating the code and all its subtlety.
-func gentraceback(pc0 uintptr, sp0 uintptr, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max int, callback func(*stkframe, unsafe.Pointer) bool, v unsafe.Pointer, printall bool) int {
+func gentraceback(pc0 uintptr, sp0 uintptr, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max int, callback func(*stkframe, unsafe.Pointer) bool, v unsafe.Pointer, flags uint) int {
        if goexitPC == 0 {
                gothrow("gentraceback before goexitPC initialization")
        }
@@ -297,13 +297,13 @@ func gentraceback(pc0 uintptr, sp0 uintptr, lr0 uintptr, gp *g, skip int, pcbuf
                        }
                }
                if printing {
-                       if printall || showframe(f, gp) {
+                       if (flags&_TraceRuntimeFrames) != 0 || showframe(f, gp) {
                                // Print during crash.
                                //      main(0x1, 0x2, 0x3)
                                //              /home/rsc/go/src/runtime/x.go:23 +0xf
                                //
                                tracepc := frame.pc // back up to CALL instruction for funcline.
-                               if n > 0 && frame.pc > f.entry && !waspanic {
+                               if (n > 0 || flags&_TraceTrap == 0) && frame.pc > f.entry && !waspanic {
                                        tracepc--
                                }
                                print(gofuncname(f), "(")
@@ -475,17 +475,32 @@ func printcreatedby(gp *g) {
 }
 
 func traceback(pc uintptr, sp uintptr, lr uintptr, gp *g) {
+       traceback1(pc, sp, lr, gp, 0)
+}
+
+// tracebacktrap is like traceback but expects that the PC and SP were obtained
+// from a trap, not from gp->sched or gp->syscallpc/gp->syscallsp or getcallerpc/getcallersp.
+// Because they are from a trap instead of from a saved pair,
+// the initial PC must not be rewound to the previous instruction.
+// (All the saved pairs record a PC that is a return address, so we
+// rewind it into the CALL instruction.)
+func tracebacktrap(pc uintptr, sp uintptr, lr uintptr, gp *g) {
+       traceback1(pc, sp, lr, gp, _TraceTrap)
+}
+
+func traceback1(pc uintptr, sp uintptr, lr uintptr, gp *g, flags uint) {
        var n int
        if readgstatus(gp)&^_Gscan == _Gsyscall {
-               // Override signal registers if blocked in system call.
+               // Override registers if blocked in system call.
                pc = gp.syscallpc
                sp = gp.syscallsp
+               flags &^= _TraceTrap
        }
        // Print traceback. By default, omits runtime frames.
        // If that means we print nothing at all, repeat forcing all frames printed.
-       n = gentraceback(pc, sp, lr, gp, 0, nil, _TracebackMaxFrames, nil, nil, false)
-       if n == 0 {
-               n = gentraceback(pc, sp, lr, gp, 0, nil, _TracebackMaxFrames, nil, nil, true)
+       n = gentraceback(pc, sp, lr, gp, 0, nil, _TracebackMaxFrames, nil, nil, flags)
+       if n == 0 && (flags&_TraceRuntimeFrames) == 0 {
+               n = gentraceback(pc, sp, lr, gp, 0, nil, _TracebackMaxFrames, nil, nil, flags|_TraceRuntimeFrames)
        }
        if n == _TracebackMaxFrames {
                print("...additional frames elided...\n")
@@ -496,11 +511,11 @@ func traceback(pc uintptr, sp uintptr, lr uintptr, gp *g) {
 func callers(skip int, pcbuf *uintptr, m int) int {
        sp := getcallersp(unsafe.Pointer(&skip))
        pc := uintptr(getcallerpc(unsafe.Pointer(&skip)))
-       return gentraceback(pc, sp, 0, getg(), skip, pcbuf, m, nil, nil, false)
+       return gentraceback(pc, sp, 0, getg(), skip, pcbuf, m, nil, nil, 0)
 }
 
 func gcallers(gp *g, skip int, pcbuf *uintptr, m int) int {
-       return gentraceback(^uintptr(0), ^uintptr(0), 0, gp, skip, pcbuf, m, nil, nil, false)
+       return gentraceback(^uintptr(0), ^uintptr(0), 0, gp, skip, pcbuf, m, nil, nil, 0)
 }
 
 func showframe(f *_func, gp *g) bool {

diff --git a/test/fixedbugs/issue7690.go b/test/fixedbugs/issue7690.go
new file mode 100644 (file)
index 0000000..4ad9e86
--- /dev/null
+++ b/test/fixedbugs/issue7690.go
@@ -0,0 +1,49 @@
+// run
+
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// issue 7690 - Stack and other routines did not back up initial PC
+// into CALL instruction, instead reporting line number of next instruction,
+// which might be on a different line.
+
+package main
+
+import (
+       "bytes"
+       "regexp"
+       "runtime"
+       "strconv"
+)
+
+func main() {
+       buf1 := make([]byte, 1000)
+       buf2 := make([]byte, 1000)
+
+       runtime.Stack(buf1, false)      // CALL is last instruction on this line
+       n := runtime.Stack(buf2, false) // CALL is followed by load of result from stack
+
+       buf1 = buf1[:bytes.IndexByte(buf1, 0)]
+       buf2 = buf2[:n]
+
+       re := regexp.MustCompile(`(?m)^main\.main\(\)\n.*/issue7690.go:([0-9]+)`)
+       m1 := re.FindStringSubmatch(string(buf1))
+       if m1 == nil {
+               println("BUG: cannot find main.main in first trace")
+               return
+       }
+       m2 := re.FindStringSubmatch(string(buf2))
+       if m2 == nil {
+               println("BUG: cannot find main.main in second trace")
+               return
+       }
+
+       n1, _ := strconv.Atoi(m1[1])
+       n2, _ := strconv.Atoi(m2[1])
+       if n1+1 != n2 {
+               println("BUG: expect runtime.Stack on back to back lines, have", n1, n2)
+               println(string(buf1))
+               println(string(buf2))
+       }
+}