runtime/pprof: clean up call/return PCs in memory profiles

Proto profile conversion is inconsistent about call vs return PCs in
profile locations. The proto defines locations to be call PCs. This is
what we do when proto-izing CPU profiles, but we fail to convert the
return PCs in memory and count profile stacks to call PCs when
converting them to proto locations.

Fix this in the heap and count profile conversion functions.
TestConvertMemProfile also hard-codes this failure to convert from
return PCs to call PCs, so fix up the addresses in the synthesized
profile to be return PCs while checking that we get call PCs out of
the conversion.

Change-Id: If1fc028b86fceac6d71a2d9fa6c41ff442c89296
Reviewed-on: https://go-review.googlesource.com/42951
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

diff --git a/src/runtime/pprof/pprof.go b/src/runtime/pprof/pprof.go
index 98c08654cf600550841082ff14a4e3e9a2d076be..74092d2565218ec90d50b5b6f53d25e84064b07d 100644 (file)
--- a/src/runtime/pprof/pprof.go
+++ b/src/runtime/pprof/pprof.go
@@ -398,10 +398,11 @@ func printCountProfile(w io.Writer, debug int, name string, p countProfile) erro
        for _, k := range keys {
                values[0] = int64(count[k])
                locs = locs[:0]
-               for i, addr := range p.Stack(index[k]) {
-                       if false && i > 0 { // TODO: why disabled?
-                               addr--
-                       }
+               for _, addr := range p.Stack(index[k]) {
+                       // For count profiles, all stack addresses are
+                       // return PCs. Adjust them to be call PCs for
+                       // locForPC.
+                       addr--
                        locs = append(locs, b.locForPC(addr))
                }
                b.pbSample(values, locs, nil)

diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go
index fd0b7c3e620f7fd5bc322134f4cc22bc8fc3450a..5210706f4e3c9c6817d76fe7bf958f178d77f8ea 100644 (file)
--- a/src/runtime/pprof/proto.go
+++ b/src/runtime/pprof/proto.go
@@ -183,6 +183,7 @@ func (b *profileBuilder) pbMapping(tag int, id, base, limit, offset uint64, file
 }
 
 // locForPC returns the location ID for addr.
+// addr must be a call address (not a return address).
 // It may emit to b.pb, so there must be no message encoding in progress.
 func (b *profileBuilder) locForPC(addr uintptr) uint64 {
        id := uint64(b.locs[addr])

diff --git a/src/runtime/pprof/protomem.go b/src/runtime/pprof/protomem.go
index a4851a72571b1e19a2ecbbf75a42314d295f6454..86c7dacfe67182bc3fd179eefa6b48f4118741d0 100644 (file)
--- a/src/runtime/pprof/protomem.go
+++ b/src/runtime/pprof/protomem.go
@@ -27,10 +27,11 @@ func writeHeapProto(w io.Writer, p []runtime.MemProfileRecord, rate int64) error
                locs = locs[:0]
                hideRuntime := true
                for tries := 0; tries < 2; tries++ {
-                       for i, addr := range r.Stack() {
-                               if false && i > 0 { // TODO: why disabled?
-                                       addr--
-                               }
+                       for _, addr := range r.Stack() {
+                               // For heap profiles, all stack
+                               // addresses are return PCs. Adjust
+                               // them to be call PCs for locForPC.
+                               addr--
                                if hideRuntime {
                                        if f := runtime.FuncForPC(addr); f != nil && strings.HasPrefix(f.Name(), "runtime.") {
                                                continue

diff --git a/src/runtime/pprof/protomem_test.go b/src/runtime/pprof/protomem_test.go
index e5aa69fc3775cef98f847a6e71371ca97ab38888..1e30ed93a36060d189e950069d7a3de5e3f460cd 100644 (file)
--- a/src/runtime/pprof/protomem_test.go
+++ b/src/runtime/pprof/protomem_test.go
@@ -15,7 +15,11 @@ func TestConvertMemProfile(t *testing.T) {
        addr1, addr2, map1, map2 := testPCs(t)
 
        var buf bytes.Buffer
-       a1, a2 := uintptr(addr1), uintptr(addr2)
+       // MemProfileRecord stacks are return PCs, so add one to the
+       // addresses recorded in the "profile". The proto profile
+       // locations are call PCs, so conversion will subtract one
+       // from these and get back to addr1 and addr2.
+       a1, a2 := uintptr(addr1)+1, uintptr(addr2)+1
        rate := int64(512 * 1024)
        rec := []runtime.MemProfileRecord{
                {AllocBytes: 4096, FreeBytes: 1024, AllocObjects: 4, FreeObjects: 1, Stack0: [32]uintptr{a1, a2}},