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

Support Intel syntax in the assembly reports #520

Merged
merged 9 commits into from
Apr 13, 2020

Conversation

wyk9787
Copy link
Contributor

@wyk9787 wyk9787 commented Apr 10, 2020

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
   ...

@wyk9787 wyk9787 changed the title Assembly Support Intel syntax in assembly reports Apr 10, 2020
@kalyanac
Copy link
Contributor

Can you include test outputs?

@kalyanac kalyanac requested a review from nolanmar511 April 10, 2020 17:51
Comment on lines 165 to 180
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)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

internal/report/source.go Outdated Show resolved Hide resolved
@aalexand
Copy link
Collaborator

This PR seems to contain unrelated commits.

@wyk9787
Copy link
Contributor Author

wyk9787 commented Apr 10, 2020

This PR seems to contain unrelated commits.

Should now be reverted.

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing trailing full stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@wyk9787
Copy link
Contributor Author

wyk9787 commented Apr 10, 2020

to commands

Done.

@codecov-io
Copy link

codecov-io commented Apr 10, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Collaborator

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

wyk9787 commented Apr 11, 2020

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

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense. Now changed.

@wyk9787 wyk9787 changed the title Support Intel syntax in assembly reports Support Intel syntax in the assembly reports Apr 12, 2020
@aalexand aalexand merged commit b1a9688 into google:master Apr 13, 2020
@wyk9787 wyk9787 deleted the assembly branch April 15, 2020 01:15
gopherbot pushed a commit to golang/go that referenced this pull request Oct 7, 2020
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>
giordano added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Nov 13, 2020
* 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>
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
Add support for output assembly in Intel Syntax.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Intel syntax in the assembly reports
5 participants