Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cherrymui opened this issue Jan 28, 2019 · 3 comments
Open

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

cherrymui opened this issue Jan 28, 2019 · 3 comments
Labels
Priority: p3 Buganizer priority - P3 type: bug Buganizer type - Bug

Comments

@cherrymui
Copy link

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 (e84dfd6)

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
Copy link
Contributor

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

@kalyanac
Copy link
Contributor

kalyanac commented Feb 7, 2019

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

@aalexand
Copy link
Collaborator

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

@aalexand aalexand self-assigned this Nov 22, 2019
@aalexand aalexand removed their assignment Sep 28, 2020
@Louis-Ye Louis-Ye added type: bug Buganizer type - Bug Priority: p3 Buganizer priority - P3 labels May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: p3 Buganizer priority - P3 type: bug Buganizer type - Bug
Projects
None yet
Development

No branches or pull requests

5 participants