Add support for LLVM objdump #534

kalyanac · 2020-05-06T18:14:03Z

Adds support for LLVM objdump

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

pull from remote master

googlebot · 2020-05-06T18:14:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-06T18:20:49Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-06T18:20:51Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

kalyanac · 2020-05-06T20:23:51Z

Failures unrelated. Restarted the jobs

kalyanac · 2020-05-15T22:39:37Z

gentle ping.

aalexand · 2020-05-16T02:27:31Z

It feels that the PR description doesn't tell the full story. There is no binutils version 9.0, that seems LLVM objdump. It seems that what we are trying to dispatch on here is binutils vs. LLVM objdump, so the PR description and the code change should be centered around that IMO.

internal/binutils/binutils.go

aalexand · 2020-05-16T02:28:32Z

internal/binutils/binutils.go

@@ -120,6 +121,29 @@ func (bu *Binutils) SetTools(config string) {
 	bu.update(func(r *binrep) { initTools(r, config) })
 }

+func ensureObjdumpVersion(objdump string, objdumpFound bool) (string, bool) {


Prefer top-down function order in Go - place this function after its use, i.e. after initTools.

internal/binutils/binutils.go

aalexand · 2020-05-16T02:30:39Z

internal/binutils/binutils_test.go

@@ -189,14 +191,39 @@ func skipUnlessDarwinAmd64(t *testing.T) {
 }

 func testDisasm(t *testing.T, intelSyntax bool) {
+	if runtime.GOOS == "darwin" {
+		// get objdump version and skip test if < 9.0


nit: get -> Get (proper sentences for comments).

MaskRay · 2020-05-17T22:18:46Z

Thanks for the patch! As Alexey noted, this patch with a few tweaks can work with llvm-objdump on other (non-Mac) platforms. Both rupprecht and I have made some UI improvements to llvm-objdump 10. llvm-objdump 10 may be closer to GNU objdump. (The function:line pattern just looks like GNU objdump's output)

Consider using --version (instead of Sun-style -version). GNU objdump doesn't accept -version. Latest llvm-objdump does not accept -version, either.

kalyanac · 2020-05-19T06:26:21Z

It feels that the PR description doesn't tell the full story. There is no binutils version 9.0, that seems LLVM objdump. It seems that what we are trying to dispatch on here is binutils vs. LLVM objdump, so the PR description and the code change should be centered around that IMO.

The PR description states objdump version should be at least 9.0, not binutils. This is from OS X core (pkgid: com.apple.pkg.Core) and not the homebrew binutils, which packages GNU tools.

Yes, this is (Apple) LLVM objdump. This PR is only to add support for the objdump installed with XCode.

objdump -version
Apple LLVM version 10.0.0 (clang-1000.11.45.5)

LLVM objdump support for Linux should be a separate, probably small, PR.

kalyanac · 2020-05-19T06:30:18Z

Thanks for the patch! As Alexey noted, this patch with a few tweaks can work with llvm-objdump on other (non-Mac) platforms. Both rupprecht and I have made some UI improvements to llvm-objdump 10. llvm-objdump 10 may be closer to GNU objdump. (The function:line pattern just looks like GNU objdump's output)

A separate PR will add support for LLVM objdump for non-Mac platforms.

Consider using --version (instead of Sun-style -version). GNU objdump doesn't accept -version. Latest llvm-objdump does not accept -version, either.

objdump on Mac seems to support the --version format but does not document it. --help only uses the single dash format.

I am not too familiar with how Apple changes their code from LLVM, so I figured it is safe to stay with documented flags.

codecov-commenter · 2020-05-19T07:03:44Z

Codecov Report

Merging #534 into master will increase coverage by 0.00%.
The diff coverage is 72.72%.

@@           Coverage Diff           @@
##           master     #534   +/-   ##
=======================================
  Coverage   68.33%   68.33%           
=======================================
  Files          39       39           
  Lines        8005     8044   +39     
=======================================
+ Hits         5470     5497   +27     
- Misses       2114     2120    +6     
- Partials      421      427    +6

Impacted Files	Coverage Δ
internal/binutils/binutils.go	`58.89% <70.00%> (+1.09%)`	⬆️
internal/binutils/disasm.go	`89.61% <100.00%> (+0.56%)`	⬆️

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 163a225...8119593. Read the comment docs.

aalexand · 2020-05-19T16:26:22Z

The PR description states objdump version should be at least 9.0, not binutils. This is from OS X core (pkgid: com.apple.pkg.Core) and not the homebrew binutils, which packages GNU tools.

objdump is part of binutils package on Linux. On a Mac machine objdump can come either from LLVM or from binutils. We should check for whether objdump being used is from LLVM or from binutils, not for the version.

kalyanac · 2020-05-27T23:48:17Z

Some Mac combinations which were not tested in CI

$ ./objdump --version
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
  Optimized build.
  Default target: x86_64-apple-darwin19.4.0
  Host CPU: skylake
....

$> PATH=./ /usr/local/go/bin/go test -v
...
=== RUN   TestDisasm
--- PASS: TestDisasm (0.03s)
=== RUN   TestObjFile
    TestObjFile: binutils_test.go:181: This test only works on x86-64 Linux
--- SKIP: TestObjFile (0.00s)

$ objdump --version
Apple LLVM version 11.0.3 (clang-1103.0.32.62)
  Optimized build.
  Default target: x86_64-apple-darwin19.4.0
  Host CPU: skylake
...

$ /usr/local/go/bin/go test -v
...
=== RUN   TestDisasm
--- PASS: TestDisasm (0.08s)
=== RUN   TestObjFile
    TestObjFile: binutils_test.go:181: This test only works on x86-64 Linux
--- SKIP: TestObjFile (0.00s)
....

kalyanac · 2020-05-28T04:56:51Z

Failures are due to timeouts. PR can be reviewed

internal/binutils/binutils.go

internal/binutils/disasm.go

internal/binutils/binutils_test.go

internal/binutils/binutils.go

internal/binutils/binutils_test.go

kalyanac · 2020-05-29T02:33:20Z

Failures due to timeout, restarted. Code ok to review.

kalyanac · 2020-06-01T16:53:03Z

gentle ping.

internal/binutils/binutils.go

internal/binutils/binutils_test.go

kalyanac · 2020-06-08T17:41:28Z

PTAL. All test passed before the latest merge. The latest merge is only a comment change, no functional changes. Code can be reviewed.

kalyanac · 2020-06-08T19:57:39Z

Restarted timedout tests.

aalexand

Looks good, probably last round of minor style nits. Thanks for bearing with the comments.

internal/binutils/binutils.go

internal/binutils/binutils_test.go

internal/binutils/disasm_test.go

kalyanac · 2020-06-10T22:34:49Z

Restarted timed out tests.

Code is ready for review, PTAL.

Thanks for bearing with the comments.

Thank you for patiently providing the thoughtful reviews.

kalyanac · 2020-06-15T23:52:29Z

Gentle ping

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

kalyanac and others added 5 commits February 4, 2019 21:12

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 pull request #3 from google/master

5987a5e

pull from remote master

googlebot added the cla: no label May 6, 2020

kalyanac requested review from aalexand and rauls5382 May 6, 2020 18:14

kalyanac force-pushed the mac_disasm branch 2 times, most recently from c470b1e to e072f4a Compare May 6, 2020 18:20

googlebot removed the cla: no label May 6, 2020

googlebot added the cla: yes label May 6, 2020

kalyanac mentioned this pull request May 6, 2020

use simplified demangled symbol names in disassembly report #449

Closed

aalexand reviewed May 16, 2020

View reviewed changes

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

aalexand reviewed May 16, 2020

View reviewed changes

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

aalexand reviewed May 16, 2020

View reviewed changes

kalyanac force-pushed the mac_disasm branch 2 times, most recently from cda73ae to 853e9d0 Compare May 19, 2020 06:09

aalexand reviewed May 28, 2020

View reviewed changes

kalyanac force-pushed the mac_disasm branch 2 times, most recently from cb2b584 to 478ecfd Compare May 28, 2020 23:09

kalyanac commented May 28, 2020

View reviewed changes

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

aalexand reviewed Jun 2, 2020

View reviewed changes

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

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

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

kalyanac force-pushed the mac_disasm branch 2 times, most recently from b0bc32b to ed96624 Compare June 3, 2020 02:52

aalexand reviewed Jun 5, 2020

View reviewed changes

kalyanac force-pushed the mac_disasm branch 2 times, most recently from 1edfed9 to ee01eb1 Compare June 8, 2020 17:30

aalexand reviewed Jun 9, 2020

View reviewed changes

kalyanac force-pushed the mac_disasm branch from 45b6460 to b31d543 Compare June 9, 2020 21:47

Add support for LLVM objudmp version string with turnk

e1143a5

kalyanac force-pushed the mac_disasm branch from 7170d28 to e1143a5 Compare June 9, 2020 21:55

Merge branch 'master' into mac_disasm

8119593

aalexand approved these changes Jun 15, 2020

View reviewed changes

aalexand merged commit 03e1cf3 into master Jun 15, 2020

kalyanac deleted the mac_disasm branch June 16, 2020 00:16

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

Add support for LLVM objdump (google#534)

a750844

Add support for LLVM objdump.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for LLVM objdump #534

Add support for LLVM objdump #534

kalyanac commented May 6, 2020 •

edited

Loading

googlebot commented May 6, 2020

googlebot commented May 6, 2020

googlebot commented May 6, 2020

kalyanac commented May 6, 2020

kalyanac commented May 15, 2020

aalexand commented May 16, 2020

aalexand May 16, 2020

kalyanac May 19, 2020

aalexand May 16, 2020

kalyanac May 19, 2020

MaskRay commented May 17, 2020 •

edited

Loading

kalyanac commented May 19, 2020

kalyanac commented May 19, 2020

codecov-commenter commented May 19, 2020 •

edited

Loading

aalexand commented May 19, 2020

kalyanac commented May 27, 2020

kalyanac commented May 28, 2020

kalyanac commented May 29, 2020

kalyanac commented Jun 1, 2020

kalyanac commented Jun 8, 2020

kalyanac commented Jun 8, 2020

aalexand left a comment

kalyanac commented Jun 10, 2020

kalyanac commented Jun 15, 2020

Add support for LLVM objdump #534

Add support for LLVM objdump #534

Conversation

kalyanac commented May 6, 2020 • edited Loading

googlebot commented May 6, 2020

googlebot commented May 6, 2020

googlebot commented May 6, 2020

kalyanac commented May 6, 2020

kalyanac commented May 15, 2020

aalexand commented May 16, 2020

aalexand May 16, 2020

Choose a reason for hiding this comment

kalyanac May 19, 2020

Choose a reason for hiding this comment

aalexand May 16, 2020

Choose a reason for hiding this comment

kalyanac May 19, 2020

Choose a reason for hiding this comment

MaskRay commented May 17, 2020 • edited Loading

kalyanac commented May 19, 2020

kalyanac commented May 19, 2020

codecov-commenter commented May 19, 2020 • edited Loading

Codecov Report

aalexand commented May 19, 2020

kalyanac commented May 27, 2020

kalyanac commented May 28, 2020

kalyanac commented May 29, 2020

kalyanac commented Jun 1, 2020

kalyanac commented Jun 8, 2020

kalyanac commented Jun 8, 2020

aalexand left a comment

Choose a reason for hiding this comment

kalyanac commented Jun 10, 2020

kalyanac commented Jun 15, 2020

kalyanac commented May 6, 2020 •

edited

Loading

MaskRay commented May 17, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading