runtime/pprof: increment fake overflow record PC

gentraceback generates PCs which are usually following the CALL
instruction. For those that aren't, it fixes up the PCs so that
functions processing the output can unconditionally decrement the PC.

runtime_expandInlineFrames does this unconditional decrement when
looking up the function. However, the fake stack frame generated for
overflow records fails to meet the contract, and decrementing the PC
results in a PC in the previous function. If that function contains
inlined call, runtime_expandInlineFrames will not short-circuit and will
panic trying to look up a PC that doesn't exist.

Note that the added test does not fail at HEAD. It will only fail (with
a panic) if the function preceeding lostProfileEvent contains inlined
function calls. At the moment (on linux/amd64), that is
runtime/pprof.addMaxRSS, which does not.

Fixes #38096

Change-Id: Iad0819f23c566011c920fd9a5b1254719228da0b
Reviewed-on: https://go-review.googlesource.com/c/go/+/225661
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/runtime/pprof/pprof_test.go b/src/runtime/pprof/pprof_test.go
index 5bfc3b6134131183a2cadf7a049086b4c1f8636f..83b3152d684756fbbc9e989964ac100a47e127c2 100644 (file)
--- a/src/runtime/pprof/pprof_test.go
+++ b/src/runtime/pprof/pprof_test.go
@@ -1171,6 +1171,18 @@ func TestTryAdd(t *testing.T) {
                        {Value: []int64{10, 10 * period}, Location: []*profile.Location{{ID: 1}, {ID: 1}}},
                        {Value: []int64{20, 20 * period}, Location: []*profile.Location{{ID: 1}}},
                },
+       }, {
+               name: "bug38096",
+               input: []uint64{
+                       3, 0, 500, // hz = 500. Must match the period.
+                       // count (data[2]) == 0 && len(stk) == 1 is an overflow
+                       // entry. The "stk" entry is actually the count.
+                       4, 0, 0, 4242,
+               },
+               wantLocs: [][]string{{"runtime/pprof.lostProfileEvent"}},
+               wantSamples: []*profile.Sample{
+                       {Value: []int64{4242, 4242 * period}, Location: []*profile.Location{{ID: 1}}},
+               },
        }, {
                // If a function is called recursively then it must not be
                // inlined in the caller.

diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go
index 416ace7ab2470f49de838e69957d34164d7a6342..bb63153a701b38f89abc87d1c8da38ecb469e872 100644 (file)
--- a/src/runtime/pprof/proto.go
+++ b/src/runtime/pprof/proto.go
@@ -322,7 +322,10 @@ func (b *profileBuilder) addCPUData(data []uint64, tags []unsafe.Pointer) error
                        // overflow record
                        count = uint64(stk[0])
                        stk = []uint64{
-                               uint64(funcPC(lostProfileEvent)),
+                               // gentraceback guarantees that PCs in the
+                               // stack can be unconditionally decremented and
+                               // still be valid, so we must do the same.
+                               uint64(funcPC(lostProfileEvent)+1),
                        }
                }
                b.m.lookup(stk, tag).count += int64(count)