runtime: fix line number for faulting instructions

Unlike function calls, when processing instructions that directly
fault we must not subtract 1 from the pc before looking up the
file/line information.

Since the file/line lookup unconditionally subtracts 1, add 1 to
the faulting instruction PCs to compensate.

Fixes #34123

Change-Id: Ie7361e3d2f84a0d4f48d97e5a9e74f6291ba7a8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/196962
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>

diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go
index c269c3a65212c1f4fbf39f70e906a842518146a1..3e6012df575441a9b6a4d5be6e106279197737e1 100644 (file)
--- a/src/runtime/pprof/proto.go
+++ b/src/runtime/pprof/proto.go
@@ -359,13 +359,7 @@ func (b *profileBuilder) build() {
                        }
                }
 
-               // Addresses from stack traces point to the next instruction after each call,
-               // except for the leaf, which points to where the signal occurred.
-               // appendLocsForStack expects return PCs so increment the leaf address to
-               // look like a return PC.
-               e.stk[0] += 1
                locs = b.appendLocsForStack(locs[:0], e.stk)
-               e.stk[0] -= 1 // undo the adjustment on the leaf.
 
                b.pbSample(values, locs, labels)
        }

diff --git a/src/runtime/pprof/proto_test.go b/src/runtime/pprof/proto_test.go
index eda2b003ad44a466fba2177080a1aed19d201087..f3456ffede49906974e8c224cf9dafb8613954f0 100644 (file)
--- a/src/runtime/pprof/proto_test.go
+++ b/src/runtime/pprof/proto_test.go
@@ -116,9 +116,9 @@ func TestConvertCPUProfile(t *testing.T) {
 
        b := []uint64{
                3, 0, 500, // hz = 500
-               5, 0, 10, uint64(addr1), uint64(addr1 + 2), // 10 samples in addr1
-               5, 0, 40, uint64(addr2), uint64(addr2 + 2), // 40 samples in addr2
-               5, 0, 10, uint64(addr1), uint64(addr1 + 2), // 10 samples in addr1
+               5, 0, 10, uint64(addr1 + 1), uint64(addr1 + 2), // 10 samples in addr1
+               5, 0, 40, uint64(addr2 + 1), uint64(addr2 + 2), // 40 samples in addr2
+               5, 0, 10, uint64(addr1 + 1), uint64(addr1 + 2), // 10 samples in addr1
        }
        p, err := translateCPUProfile(b)
        if err != nil {

diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go
index dc2a7a36935aae06294ad8c80c1d03eb3c830450..944c8473d25992a2bdcbaf571fa248cd1ab28732 100644 (file)
--- a/src/runtime/traceback.go
+++ b/src/runtime/traceback.go
@@ -340,7 +340,20 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                        pc := frame.pc
                        // backup to CALL instruction to read inlining info (same logic as below)
                        tracepc := pc
-                       if (n > 0 || flags&_TraceTrap == 0) && frame.pc > f.entry && !waspanic {
+                       // Normally, pc is a return address. In that case, we want to look up
+                       // file/line information using pc-1, because that is the pc of the
+                       // call instruction (more precisely, the last byte of the call instruction).
+                       // Callers expect the pc buffer to contain return addresses and do the
+                       // same -1 themselves, so we keep pc unchanged.
+                       // When the pc is from a signal (e.g. profiler or segv) then we want
+                       // to look up file/line information using pc, and we store pc+1 in the
+                       // pc buffer so callers can unconditionally subtract 1 before looking up.
+                       // See issue 34123.
+                       // The pc can be at function entry when the frame is initialized without
+                       // actually running code, like runtime.mstart.
+                       if (n == 0 && flags&_TraceTrap != 0) || waspanic || pc == f.entry {
+                               pc++
+                       } else {
                                tracepc--
                        }
 

diff --git a/test/fixedbugs/issue34123.go b/test/fixedbugs/issue34123.go
new file mode 100644 (file)
index 0000000..f50cd02
--- /dev/null
+++ b/test/fixedbugs/issue34123.go
@@ -0,0 +1,43 @@
+// run
+
+// Copyright 2019 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.
+
+// Make sure that the line number is reported correctly
+// for faulting instructions.
+
+package main
+
+import (
+       "fmt"
+       "runtime"
+)
+
+var x byte
+var p *byte
+
+//go:noinline
+func f() {
+       q := p
+       x = 11  // line 23
+       *q = 12 // line 24
+}
+func main() {
+       defer func() {
+               recover()
+               var pcs [10]uintptr
+               n := runtime.Callers(1, pcs[:])
+               frames := runtime.CallersFrames(pcs[:n])
+               for {
+                       f, more := frames.Next()
+                       if f.Function == "main.f" && f.Line != 24 {
+                               panic(fmt.Errorf("expected line 24, got line %d", f.Line))
+                       }
+                       if !more {
+                               break
+                       }
+               }
+       }()
+       f()
+}