Support Intel syntax in the assembly reports #520

wyk9787 · 2020-04-10T17:42:02Z

Fixes: #400

Add an option to support Intel syntax in the assembly report, which is used in both disasm and weblist formats.

Tested on both Mac OS and Linux.

(pprof) disasm absl::*
   ...
         .          .    28aa620: movl  %ebx, %r11d
   ...
(pprof) intel_syntax
(pprof) disasm absl::*
   ...
         .          .    28aa620: mov   r11d, ebx
   ...

…ol option.

kalyanac · 2020-04-10T17:47:33Z

Can you include test outputs?

internal/binutils/binutils_test.go

internal/driver/commands.go

driver/driver.go

internal/binutils/binutils_test.go

aalexand · 2020-04-10T18:06:30Z

internal/binutils/binutils.go

+			cmd = exec.Command(b.objdump, "-x86-asm-syntax=intel", "-d", "-C", "--no-show-raw-insn", "-l",
+				fmt.Sprintf("--start-address=%#x", start),
+				fmt.Sprintf("--stop-address=%#x", end),
+				file)
+		} else {
+			cmd = exec.Command(b.objdump, "-M", "intel", "-d", "-C", "--no-show-raw-insn", "-l",
+				fmt.Sprintf("--start-address=%#x", start),
+				fmt.Sprintf("--stop-address=%#x", end),
+				file)
+		}
+	} else {
+		cmd = exec.Command(b.objdump, "-d", "-C", "--no-show-raw-insn", "-l",
+			fmt.Sprintf("--start-address=%#x", start),
+			fmt.Sprintf("--stop-address=%#x", end),
+			file)
+	}


Can we modify the arguments instead of duplicating the whole call? Make them an array, massage as needed, use ellipsis syntax to invoke exec.Command.

internal/driver/commands.go

internal/report/source.go

aalexand · 2020-04-10T18:08:27Z

This PR seems to contain unrelated commits.

…y non-bool option." This reverts commit a9ea908.

wyk9787 · 2020-04-10T19:04:38Z

This PR seems to contain unrelated commits.

Should now be reverted.

aalexand · 2020-04-10T19:06:28Z

internal/report/report.go

@@ -79,6 +79,8 @@ type Options struct {
 	Symbol     *regexp.Regexp // Symbols to include on disassembly report.
 	SourcePath string         // Search path for source files.
 	TrimPath   string         // Paths to trim from source file paths.
+
+	IntelSyntax bool // Whether or not to print assembly in Intel syntax


nit: missing trailing full stop.

wyk9787 · 2020-04-10T19:15:55Z

to commands

Done.

codecov-io · 2020-04-10T19:40:03Z

Codecov Report

Merging #520 into master will increase coverage by 0.01%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   67.06%   67.07%   +0.01%     
==========================================
  Files          37       37              
  Lines        7611     7623      +12     
==========================================
+ Hits         5104     5113       +9     
- Misses       2103     2104       +1     
- Partials      404      406       +2

Impacted Files	Coverage Δ
internal/driver/commands.go	`44.00% <ø> (ø)`
internal/report/report.go	`30.64% <0.00%> (ø)`
internal/binutils/binutils.go	`57.99% <70.00%> (-0.03%)`	⬇️
internal/driver/driver.go	`70.05% <100.00%> (+0.17%)`	⬆️
internal/report/source.go	`83.24% <100.00%> (ø)`
internal/driver/webhtml.go	`100.00% <0.00%> (ø)`
profile/profile.go	`71.23% <0.00%> (+0.15%)`	⬆️

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 1ebb73c...40bead3. Read the comment docs.

aalexand · 2020-04-10T22:26:42Z

Are you referring to "--start_address" and "--stop-address"? Do you mean they should be put after file?

I am referring to the file path argument (it's the only positional one: --start-address and --stop-address are not, these are options / flags).

wyk9787 · 2020-04-11T00:18:16Z

Are you referring to "--start_address" and "--stop-address"? Do you mean they should be put after file?

I am referring to the file path argument (it's the only positional one: --start-address and --stop-address are not, these are options / flags).

I see. Right now, I am actually inserting the "intel flags" at the beginning of the args, so the file path argument is indeed the last argument.

aalexand · 2020-04-11T02:38:58Z

I see. Right now, I am actually inserting the "intel flags" at the beginning of the args, so the file path argument is indeed the last argument.

Ah, I missed that, sorry.

aalexand · 2020-04-11T02:39:57Z

internal/binutils/binutils.go

 	b := bu.get()
-	cmd := exec.Command(b.objdump, "-d", "-C", "--no-show-raw-insn", "-l",
+	var cmd *exec.Cmd


Remove this var, and use "cmd :=" below, as prior to to this change.

aalexand · 2020-04-11T02:42:26Z

internal/binutils/binutils.go

+
+	if intelSyntax {
+		if runtime.GOOS == "darwin" {
+			args = append([]string{"-x86-asm-syntax=intel"}, args...)


nit: I personally would append this flag at the end, and move appending file to after this code. It doesn't really matter much in this case, but appending "args..." is O(N), unnecessarily. Again, this doesn't really matter here since the list is fixed len and tiny. So feel free to leave this as is.

It makes sense. Now changed.

Updated cmd/pprof.objTool.Disasm to accept an additional bool param introduced in google/pprof#520 to support intel syntax in the assembly report. Returns an error if the intelSyntax param is set. We use src/cmd/internal/objfile to disassemble and print assembly so I am not sure if it is relevant, and if so, how. Fixes #38802 Updates #36905 Change-Id: Iae2b4322404f232196705f05210f00e2495588d9 Reviewed-on: https://go-review.googlesource.com/c/go/+/248499 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>

Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504

* Update pprof to latest revision Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504 * Update P/pprof/build_tarballs.jl - use a real version number Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Remove now unused `timestamp` * [pprof] Use `GitSource` Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

Add support for output assembly in Intel Syntax.

wyk9787 added 2 commits April 8, 2020 14:53

Provide more helpful error message and require input for every non-bo…

a9ea908

…ol option.

Add support for output assembly in Intel Syntax

c08f654

googlebot added the cla: yes label Apr 10, 2020

wyk9787 changed the title ~~Assembly~~ Support Intel syntax in assembly reports Apr 10, 2020

kalyanac reviewed Apr 10, 2020

View reviewed changes

internal/binutils/binutils_test.go Outdated Show resolved Hide resolved

kalyanac reviewed Apr 10, 2020

View reviewed changes

internal/driver/commands.go Outdated Show resolved Hide resolved

kalyanac requested a review from nolanmar511 April 10, 2020 17:51

kalyanac reviewed Apr 10, 2020

View reviewed changes

driver/driver.go Show resolved Hide resolved

aalexand reviewed Apr 10, 2020

View reviewed changes

internal/binutils/binutils_test.go Outdated Show resolved Hide resolved

aalexand reviewed Apr 10, 2020

View reviewed changes

internal/driver/commands.go Outdated Show resolved Hide resolved

aalexand reviewed Apr 10, 2020

View reviewed changes

internal/report/source.go Outdated Show resolved Hide resolved

wyk9787 added 2 commits April 10, 2020 12:00

Refactor to use varargs

bb2dcc0

Revert "Provide more helpful error message and require input for ever…

3a9481b

…y non-bool option." This reverts commit a9ea908.

aalexand reviewed Apr 10, 2020

View reviewed changes

Fix nits

e4a7364

Fix testDisasm

40bead3

Merge branch 'master' into assembly

4f27ada

aalexand reviewed Apr 11, 2020

View reviewed changes

wyk9787 added 2 commits April 12, 2020 14:40

Fix a few nits

c3b8c71

Merge branch 'assembly' of github.com:wyk9787/pprof into assembly

1d913b5

wyk9787 changed the title ~~Support Intel syntax in assembly reports~~ Support Intel syntax in the assembly reports Apr 12, 2020

aalexand approved these changes Apr 13, 2020

View reviewed changes

aalexand merged commit b1a9688 into google:master Apr 13, 2020

wyk9787 deleted the assembly branch April 15, 2020 01:15

NHDaly mentioned this pull request Nov 13, 2020

Update pprof to latest revision JuliaPackaging/Yggdrasil#2096

Merged

gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020

Support Intel syntax in the assembly reports (google#520)

4da96a8

Add support for output assembly in Intel Syntax.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Intel syntax in the assembly reports #520

Support Intel syntax in the assembly reports #520

wyk9787 commented Apr 10, 2020 •

edited

Loading

kalyanac commented Apr 10, 2020

aalexand Apr 10, 2020

wyk9787 Apr 10, 2020

aalexand commented Apr 10, 2020

wyk9787 commented Apr 10, 2020

aalexand Apr 10, 2020

wyk9787 Apr 10, 2020

wyk9787 commented Apr 10, 2020

codecov-io commented Apr 10, 2020 •

edited

Loading

aalexand commented Apr 10, 2020

wyk9787 commented Apr 11, 2020

aalexand commented Apr 11, 2020

aalexand Apr 11, 2020

wyk9787 Apr 12, 2020

aalexand Apr 11, 2020

wyk9787 Apr 12, 2020

Support Intel syntax in the assembly reports #520

Support Intel syntax in the assembly reports #520

Conversation

wyk9787 commented Apr 10, 2020 • edited Loading

kalyanac commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Apr 10, 2020

wyk9787 commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyk9787 commented Apr 10, 2020

codecov-io commented Apr 10, 2020 • edited Loading

Codecov Report

aalexand commented Apr 10, 2020

wyk9787 commented Apr 11, 2020

aalexand commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyk9787 commented Apr 10, 2020 •

edited

Loading

codecov-io commented Apr 10, 2020 •

edited

Loading