From 1dc0f9696b743b10bf0b7b49780bee7f4756499b Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Mon, 8 May 2017 22:31:41 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Michael Matloob --- src/runtime/pprof/pprof.go | 9 +++++---- src/runtime/pprof/proto.go | 1 + src/runtime/pprof/protomem.go | 9 +++++---- src/runtime/pprof/protomem_test.go | 6 +++++- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/runtime/pprof/pprof.go b/src/runtime/pprof/pprof.go index 98c08654cf..74092d2565 100644 --- 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 fd0b7c3e62..5210706f4e 100644 --- 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 a4851a7257..86c7dacfe6 100644 --- 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 e5aa69fc37..1e30ed93a3 100644 --- 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}}, -- 2.48.1