runtime/pprof: fix allFrames cache

The compiler may choose to inline multiple layers of function call, such
that A calling B calling C may end up with all of the instructions for B
and C written as part of A's function body.

Within that function body, some PCs will represent code from function A.
Some will represent code from function B, and for each of those the
runtime will have an instruction attributable to A that it can report as
its caller. Others will represent code from function C, and for each of
those the runtime will have an instruction attributable to B and an
instruction attributable to A that it can report as callers.

When a profiling signal arrives at an instruction in B (as inlined in A)
that the runtime also uses to describe calls to C, the profileBuilder
ends up with an incorrect cache of allFrames results. That PC should
lead to a location record in the profile that represents the frames
B<-A, but the allFrames cache's view should expand the PC only to the B
frame.

Otherwise, when a profiling signal arrives at an instruction in C (as
inlined in B in A), the PC stack C,B,A can get expanded to the frames
C,B<-A,A as follows: The inlining deck starts empty. The first tryAdd
call proposes PC C and frames C, which the deck accepts. The second
tryAdd call proposes PC B and, due to the incorrect caching, frames B,A.
(A fresh call to allFrames with PC B would return the frame list B.) The
deck accepts that PC and frames. The third tryAdd call proposes PC A and
frames A. The deck rejects those because a call from A to A cannot
possibly have been inlined. This results in a new location record in the
profile representing the frames C<-B<-A (good), as called by A (bad).

The bug is the cached expansion of PC B to frames B<-A. That mapping is
only appropriate for the resulting protobuf-format profile. The cache
needs to reflect the results of a call to allFrames, which expands the
PC B to the single frame B.

For #50996
For #52693
Fixes #52764

Change-Id: I36d080f3c8a05650cdc13ced262189c33b0083b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/404995
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Rhys Hiltner <rhys@justin.tv>
Reviewed-by: Cherry Mui <cherryyz@google.com>

diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go
index f0769935ae801fcf17e0abfc33741670942f4cd1..085027cd9833f3d499334ba2b15ee626d1d05921 100644 (file)
--- a/src/runtime/pprof/proto.go
+++ b/src/runtime/pprof/proto.go
@@ -246,9 +246,10 @@ type locInfo struct {
        // https://github.com/golang/go/blob/d6f2f833c93a41ec1c68e49804b8387a06b131c5/src/runtime/traceback.go#L347-L368
        pcs []uintptr
 
-       // results of allFrames call for this PC
-       frames          []runtime.Frame
-       symbolizeResult symbolizeFlag
+       // firstPCFrames and firstPCSymbolizeResult hold the results of the
+       // allFrames call for the first (leaf-most) PC this locInfo represents
+       firstPCFrames          []runtime.Frame
+       firstPCSymbolizeResult symbolizeFlag
 }
 
 // newProfileBuilder returns a new profileBuilder.
@@ -416,7 +417,7 @@ func (b *profileBuilder) appendLocsForStack(locs []uint64, stk []uintptr) (newLo
                        // stack by trying to add it to the inlining deck before assuming
                        // that the deck is complete.
                        if len(b.deck.pcs) > 0 {
-                               if added := b.deck.tryAdd(addr, l.frames, l.symbolizeResult); added {
+                               if added := b.deck.tryAdd(addr, l.firstPCFrames, l.firstPCSymbolizeResult); added {
                                        stk = stk[1:]
                                        continue
                                }
@@ -520,12 +521,21 @@ type pcDeck struct {
        pcs             []uintptr
        frames          []runtime.Frame
        symbolizeResult symbolizeFlag
+
+       // firstPCFrames indicates the number of frames associated with the first
+       // (leaf-most) PC in the deck
+       firstPCFrames int
+       // firstPCSymbolizeResult holds the results of the allFrames call for the
+       // first (leaf-most) PC in the deck
+       firstPCSymbolizeResult symbolizeFlag
 }
 
 func (d *pcDeck) reset() {
        d.pcs = d.pcs[:0]
        d.frames = d.frames[:0]
        d.symbolizeResult = 0
+       d.firstPCFrames = 0
+       d.firstPCSymbolizeResult = 0
 }
 
 // tryAdd tries to add the pc and Frames expanded from it (most likely one,
@@ -554,6 +564,10 @@ func (d *pcDeck) tryAdd(pc uintptr, frames []runtime.Frame, symbolizeResult symb
        d.pcs = append(d.pcs, pc)
        d.frames = append(d.frames, frames...)
        d.symbolizeResult |= symbolizeResult
+       if len(d.pcs) == 1 {
+               d.firstPCFrames = len(d.frames)
+               d.firstPCSymbolizeResult = symbolizeResult
+       }
        return true
 }
 
@@ -581,10 +595,10 @@ func (b *profileBuilder) emitLocation() uint64 {
 
        id := uint64(len(b.locs)) + 1
        b.locs[addr] = locInfo{
-               id:              id,
-               pcs:             append([]uintptr{}, b.deck.pcs...),
-               symbolizeResult: b.deck.symbolizeResult,
-               frames:          append([]runtime.Frame{}, b.deck.frames...),
+               id:                     id,
+               pcs:                    append([]uintptr{}, b.deck.pcs...),
+               firstPCSymbolizeResult: b.deck.firstPCSymbolizeResult,
+               firstPCFrames:          append([]runtime.Frame{}, b.deck.frames[:b.deck.firstPCFrames]...),
        }
 
        start := b.pb.startMessage()