use simplified demangled symbol names in disassembly report #449

kalyanac · 2019-01-10T05:30:18Z

In the current disassembly report, not all symbols are demangled. This results in readability challenges with examining the report.

This PR demangles all symbol names while generating the disassembly report. By default, demangling of function and template parameters is disabled to generate a simplified symbol name.

If a symbol cannot be demangled, it is printed as is.

codecov-io · 2019-01-10T05:43:07Z

Codecov Report

Merging #449 into master will increase coverage by 0.74%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
+ Coverage   67.05%   67.80%   +0.74%     
==========================================
  Files          37       37              
  Lines        7629     7637       +8     
==========================================
+ Hits         5116     5178      +62     
+ Misses       2105     2047      -58     
- Partials      408      412       +4

Impacted Files	Coverage Δ
internal/binutils/binutils.go	`56.75% <0.00%> (-1.05%)`	⬇️
internal/report/report.go	`39.22% <83.33%> (+8.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 160c429...9297b70. Read the comment docs.

aalexand · 2019-01-16T02:19:27Z

@kalyanac Add a test? The code change is small but I think we should still cover it.

sync to upstream

When a binary provided to pprof has no symbols, it is not recognized as a binary. This happens because the err value is only checked against 'nil'. This PR fixes this issue by checking the err value against elf.ErrNoSymbols in addition to nil.

Recognize an executable without symbols

kalyanac · 2019-03-09T03:31:36Z

sorry for the delay, added a test as requested

kalyanac · 2019-03-09T04:03:45Z

the failures are on OS X with objdump. I will take a look at modifying the test for OS X.

kalyanac · 2019-03-11T20:03:22Z

Mac tests using XCode 8.3 are all failing because objdump included in XCode 8.3 does not support -start-address and -stop-address

./objdump -version
Apple LLVM version 8.1.0 (clang-802.0.42)
  Optimized build.
  Default target: x86_64-apple-darwin18.2.0
  Host CPU: broadwell

./objdump -disassemble-all -no-show-raw-insn -line-numbers -start-address=0x100001cd0 -stop-address=0x100001cef ~/go/src/github.com/google/pprof/internal/report/testdata/disasm.mac.bin
objdump: Unknown command line argument '-start-address=0x100001cd0'.  Try: './objdump -help'
objdump: Did you mean '-headers=0x100001cd0'?
objdump: Unknown command line argument '-stop-address=0x100001cef'.  Try: './objdump -help'
objdump: Did you mean '-headers=0x100001cef'?

objdump -version
Apple LLVM version 10.0.0 (clang-1000.11.45.2)
  Optimized build.
  Default target: x86_64-apple-darwin18.2.0
  Host CPU: skylake

objdump -disassemble-all -no-show-raw-insn -line-numbers -start-address=0x100001cd0 -stop-address=0x100001cef ~/go/src/github.com/google/pprof/internal/report/testdata/disasm.mac.bin

/Users/user/go/src/github.com/google/pprof/internal/report/testdata/disasm.mac.bin:	file format Mach-O 64-bit x86-64

Disassembly of section __TEXT,__text:
__text:
100001cd0:	pushq	%rbp
100001cd1:	movq	%rsp, %rbp
100001cd4:	movl	%edi, -4(%rbp)
100001cd7:	movl	%esi, -8(%rbp)
100001cda:	movl	-4(%rbp), %esi
100001cdd:	cmpl	-8(%rbp), %esi
100001ce0:	sete	%al
100001ce3:	andb	$1, %al
100001ce5:	movzbl	%al, %eax
100001ce8:	popq	%rbp
100001ce9:	retq
100001cea:	nopw	(%rax,%rax)

___clang_call_terminate:

__ZNSt3__111char_traitsIcE11eq_int_typeEii:
100001cd0:	pushq	%rbp
100001cd1:	movq	%rsp, %rbp
100001cd4:	movl	%edi, -4(%rbp)
100001cd7:	movl	%esi, -8(%rbp)
100001cda:	movl	-4(%rbp), %esi
100001cdd:	cmpl	-8(%rbp), %esi
100001ce0:	sete	%al
100001ce3:	andb	$1, %al
100001ce5:	movzbl	%al, %eax
100001ce8:	popq	%rbp
100001ce9:	retq
100001cea:	nopw	(%rax,%rax)

Since XCode 8.3 is pretty old, can we drop support for it?

kalyanac · 2019-03-13T17:25:31Z

Added code to look for and determine objdump version before starting the test.
The test function will not have access to binutils initialization routine from within the test function. So the test function tries to determine the objdump version only if it is in the path.

$ objdump -version
Apple LLVM version 10.0.0 (clang-1000.11.45.2)
...

$ go test -v -run PrintAssembly
=== RUN   TestPrintAssembly
--- PASS: TestPrintAssembly (0.10s)
PASS
ok  	github.com/google/pprof/internal/report	0.218s

output with objdump version < 9.0

$ go test -v -run PrintAssembly
=== RUN   TestPrintAssembly
--- SKIP: TestPrintAssembly (0.01s)
    report_test.go:271: objdump version too old.  Apple LLVM version 8.1.0 (clang-802.0.42)
PASS
ok  	github.com/google/pprof/internal/report	0.378s

output when objdump not found or version cannot be determined

$ go test -v -run PrintAssembly
=== RUN   TestPrintAssembly
--- SKIP: TestPrintAssembly (0.01s)
    report_test.go:267: cannot determine objdump version. []
PASS
ok  	github.com/google/pprof/internal/report	0.130s

rauls5382 · 2019-04-11T22:07:15Z

internal/report/report.go

+			fmt.Fprintf(w, "ROUTINE ======================== %s\n", demangle.Filter(s.sym.Name[0][1:], options...))
+		} else {
+			fmt.Fprintf(w, "ROUTINE ======================== %s\n", demangle.Filter(s.sym.Name[0], options...))
+		}
 		for _, name := range s.sym.Name[1:] {
 			fmt.Fprintf(w, "    AKA ======================== %s\n", name)


Should we demangle these ones too?

rauls5382 · 2019-04-11T22:07:34Z

internal/report/report.go

-		fmt.Fprintf(w, "ROUTINE ======================== %s\n", s.sym.Name[0])
+		if strings.HasPrefix(s.sym.Name[0], "__Z") {
+			// Workaround to handle double leading underscores on Mac
+			// which are not properly handled by the demangle.Filter


Could we open an issue against the demangle package for this?

ianlancetaylor/demangle#12

rauls5382 · 2019-04-11T22:08:49Z

internal/report/report.go

@@ -413,6 +414,9 @@ func PrintAssembly(w io.Writer, rpt *Report, obj plugin.ObjTool, maxFuncs int) e
 		}
 	}

+	// demangle symbols by default for disassembly report
+	options := []demangle.Option{demangle.NoParams, demangle.NoTemplateParams}


I would've preferred to have a way to control this, (eg with symbolize=demangle=xxx). Sometimes having the mangled name is useful

rauls5382 · 2019-04-11T22:10:50Z

internal/binutils/binutils.go

@@ -163,6 +163,14 @@ func (bu *Binutils) Disasm(file string, start, end uint64) ([]plugin.Inst, error
 		fmt.Sprintf("--start-address=%#x", start),
 		fmt.Sprintf("--stop-address=%#x", end),
 		file)
+
+	if runtime.GOOS == "darwin" {


So without this disasm doesn't work on Mac at all? If that's the case perhaps we should put this on its own PR

pull from remote master

…nto disasm_demangle

googlebot · 2020-05-06T07:04:48Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

nolanmar511 · 2020-12-16T17:01:26Z

@kalyanac -- Does it make sense to try to get this PR merged in still?

kalyanac · 2020-12-16T17:33:59Z

No, it should be addressed by #534

use simplified demangled symbol names in disassembly report

722e882

googlebot added the cla: yes label Jan 10, 2019

aalexand and others added 6 commits January 15, 2019 18:19

Merge branch 'master' into disasm_demangle

4a33a9b

Merge pull request #1 from google/master

0de8ff2

sync to upstream

Recognize an executable without symbols

d681580

When a binary provided to pprof has no symbols, it is not recognized as a binary. This happens because the err value is only checked against 'nil'. This PR fixes this issue by checking the err value against elf.ErrNoSymbols in addition to nil.

Merge pull request #2 from kalyanac/pprof-exe-nosymbols

0153aa7

Recognize an executable without symbols

Merge branch 'master' into master

4d646c8

Merge branch 'master' into disasm_demangle

e71d09f

kalyanac force-pushed the disasm_demangle branch from da402b4 to a7e4c1a Compare March 9, 2019 03:38

kalyanac force-pushed the disasm_demangle branch 2 times, most recently from 334f31d to 316bbcc Compare March 11, 2019 18:51

kalyanac requested review from aalexand and rauls5382 March 12, 2019 03:35

kalyanac mentioned this pull request Mar 13, 2019

Drop XCode 8.3 #464

Closed

kalyanac force-pushed the disasm_demangle branch from 919d375 to f77c42f Compare March 13, 2019 17:21

test for printing demangled names in disassembly

8a1a78d

kalyanac force-pushed the disasm_demangle branch from c57d9b9 to 8a1a78d Compare March 13, 2019 17:26

Merge branch 'master' into disasm_demangle

4009e9c

rauls5382 reviewed Apr 11, 2019

View reviewed changes

kalyanac mentioned this pull request May 5, 2020

Fixed demangling fail on mac. #532

Closed

kalyanac and others added 4 commits May 5, 2020 23:35

use simplified demangled symbol names in disassembly report

f6a8201

test for printing demangled names in disassembly

15a7500

Add option to disable browser in HTTP mode (google#462)

6eaec37

Merge pull request #3 from google/master

5987a5e

pull from remote master

kalyanac added 2 commits May 5, 2020 23:52

Merge from master

1462b2f

Merge branch 'disasm_demangle' of https://github.com/kalyanac/pprof i…

cecb2a5

…nto disasm_demangle

googlebot added cla: no and removed cla: yes labels May 6, 2020

kalyanac and others added 2 commits May 6, 2020 00:09

Merge branch 'master' into disasm_demangle

9297b70

Merge branch 'master' into disasm_demangle

db747b2

kalyanac closed this Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use simplified demangled symbol names in disassembly report #449

use simplified demangled symbol names in disassembly report #449

kalyanac commented Jan 10, 2019

codecov-io commented Jan 10, 2019 •

edited

Loading

aalexand commented Jan 16, 2019

kalyanac commented Mar 9, 2019

kalyanac commented Mar 9, 2019

kalyanac commented Mar 11, 2019 •

edited

Loading

kalyanac commented Mar 13, 2019 •

edited

Loading

rauls5382 Apr 11, 2019

rauls5382 Apr 11, 2019

kalyanac May 6, 2020

rauls5382 Apr 11, 2019

rauls5382 Apr 11, 2019

kalyanac May 6, 2020

googlebot commented May 6, 2020

nolanmar511 commented Dec 16, 2020

kalyanac commented Dec 16, 2020

use simplified demangled symbol names in disassembly report #449

use simplified demangled symbol names in disassembly report #449

Conversation

kalyanac commented Jan 10, 2019

codecov-io commented Jan 10, 2019 • edited Loading

Codecov Report

aalexand commented Jan 16, 2019

kalyanac commented Mar 9, 2019

kalyanac commented Mar 9, 2019

kalyanac commented Mar 11, 2019 • edited Loading

kalyanac commented Mar 13, 2019 • edited Loading

rauls5382 Apr 11, 2019

Choose a reason for hiding this comment

rauls5382 Apr 11, 2019

Choose a reason for hiding this comment

kalyanac May 6, 2020

Choose a reason for hiding this comment

rauls5382 Apr 11, 2019

Choose a reason for hiding this comment

rauls5382 Apr 11, 2019

Choose a reason for hiding this comment

kalyanac May 6, 2020

Choose a reason for hiding this comment

googlebot commented May 6, 2020

nolanmar511 commented Dec 16, 2020

kalyanac commented Dec 16, 2020

codecov-io commented Jan 10, 2019 •

edited

Loading

kalyanac commented Mar 11, 2019 •

edited

Loading

kalyanac commented Mar 13, 2019 •

edited

Loading