google / pprof

pprof is a tool for visualization and analysis of profiling data
Apache License 2.0
7.99k stars 606 forks source link

Off-by-1 error in Disasm end address #450

Open cherrymui opened 5 years ago

cherrymui commented 5 years ago

What version of pprof are you using?

If you are using pprof via go tool pprof, what's your go env output? Go tip (8c10ce164f5b0244f3e08424c13666801b7c5973)

If you run pprof from GitHub, what's the Git revision? tip (e84dfd68c163c45ea47aa24b3dc7eaa93f6675b1)

What operating system and processor architecture are you using?

Linux/AMD64

What did you do?

Disasm is called with a Sym's End as the end address. Sym.End is the virtual address of last byte in sym (Start+size-1) (internal/plugin/plugin.go:176) whereas Disasm is stopping at (before) the end address (internal/plugin/plugin.go:124), i.e. end should be the address after the last instruction. I've seen the last instruction of a function not disassembled correctly, with both binutils and the Go objdump.

The following patch fixes this. Sorry I don't know how to add a test for this.

diff --git a/internal/report/report.go b/internal/report/report.go
index fb67a34..8208f89 100644
--- a/internal/report/report.go
+++ b/internal/report/report.go
@@ -421,7 +421,7 @@ func PrintAssembly(w io.Writer, rpt *Report, obj plugin.ObjTool, maxFuncs int) e
                flatSum, cumSum := sns.Sum()

                // Get the function assembly.
-               insts, err := obj.Disasm(s.sym.File, s.sym.Start, s.sym.End)
+               insts, err := obj.Disasm(s.sym.File, s.sym.Start, s.sym.End+1)
                if err != nil {
                        return err
                }
diff --git a/internal/report/source.go b/internal/report/source.go
index ab8b64c..5dbd173 100644
--- a/internal/report/source.go
+++ b/internal/report/source.go
@@ -248,7 +248,7 @@ func assemblyPerSourceLine(objSyms []*objSymbol, rs graph.Nodes, src string, obj
        }

        // Extract assembly for matched symbol
-       insts, err := obj.Disasm(o.sym.File, o.sym.Start, o.sym.End)
+       insts, err := obj.Disasm(o.sym.File, o.sym.Start, o.sym.End+1)
        if err != nil {
                return assembly
        }
nolanmar511 commented 5 years ago

+kalyanac@ -- I think you'd have the most context for the -disasm output right now.

kalyanac commented 5 years ago

Thank you for sending the patch. I will test it and report any concerns.

aalexand commented 5 years ago

@cherrymui Could you send a PR for this change? Thank you.