diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go index c269c3a652..3e6012df57 100644 --- 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 eda2b003ad..f3456ffede 100644 --- 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 dc2a7a3693..944c8473d2 100644 --- 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 index 0000000000..f50cd02aac --- /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() +}