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

Only run TestDisasm on amd64 #570

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Only run TestDisasm on amd64 #570

merged 1 commit into from
Oct 8, 2020

Conversation

zhsj
Copy link
Contributor

@zhsj zhsj commented Oct 7, 2020

It seems objdump is not able to dump other architecture object.

@google-cla google-cla bot added the cla: yes label Oct 7, 2020
It seems objdump is not able to dump other architecture object.
@zhsj
Copy link
Contributor Author

zhsj commented Oct 7, 2020

hmm, I'm not using master, it has been fixed.

@zhsj zhsj closed this Oct 7, 2020
@zhsj zhsj deleted the patch-1 branch October 7, 2020 19:53
@zhsj zhsj restored the patch-1 branch October 7, 2020 20:00
@zhsj
Copy link
Contributor Author

zhsj commented Oct 7, 2020

Not really fixed...

Still failing

=== RUN   TestDisasm
    binutils_test.go:205: Disasm: unexpected error [/usr/bin/objdump --disassemble-all --demangle --no-show-raw-insn --line-numbers --start-address=0x0 --stop-address=0xffffffffffffffff testdata/exe_linux_64]: exit status 1
--- FAIL: TestDisasm (0.04s)
=== RUN   TestDisasmIntelSyntax
    binutils_test.go:228: This test only works on x86_64 Linux or macOS as it tests Intel asm syntax
--- SKIP: TestDisasmIntelSyntax (0.00s)
/usr/bin/objdump: testdata/exe_linux_64: file format not recognized

@zhsj zhsj reopened this Oct 7, 2020
@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #570 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #570   +/-   ##
=======================================
  Coverage   67.14%   67.14%           
=======================================
  Files          78       78           
  Lines       14072    14072           
=======================================
  Hits         9449     9449           
  Misses       3788     3788           
  Partials      835      835           

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 1066cbb...924778f. Read the comment docs.

@kalyanac kalyanac self-requested a review October 7, 2020 20:36
@kalyanac
Copy link
Contributor

kalyanac commented Oct 7, 2020

From your error, it tooks like objdump is not able to recognize Linux binaries.
What architecture are you running the tests on? And what is the objdump version you are using?

@zhsj
Copy link
Contributor Author

zhsj commented Oct 7, 2020

I'm on arm.

$ uname -a
Linux abel 4.19.0-11-armmp-lpae #1 SMP Debian 4.19.146-1 (2020-09-17) armv7l GNU/Linux

$ objdump --version
GNU objdump (GNU Binutils for Debian) 2.35.1
Copyright (C) 2020 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) any later version.
This program has absolutely no warranty.

$ cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux bullseye/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

@zhsj
Copy link
Contributor Author

zhsj commented Oct 7, 2020

on x86 system, it seems objdump can't dump arm64 as well.

$ cat a.c 
void main(){}
$ aarch64-linux-gnu-gcc a.c 
$ file a.out 
a.out: ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=d65928b56beaf07019c6d04159c7e0ed28a7917f, for GNU/Linux 3.7.0, not stripped
$ /usr/bin/objdump --disassemble-all --demangle --no-show-raw-insn --line-numbers --start-address=0x0 --stop-address=0xffffffffffffffff a.out 

a.out:     file format elf64-little

/usr/bin/objdump: can't disassemble for architecture UNKNOWN!

$

$ objdump --version
GNU objdump (GNU Binutils for Debian) 2.35.1
Copyright (C) 2020 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) any later version.
This program has absolutely no warranty.

$ uname -a
Linux zhsj-debian 5.8.0-1-amd64 #1 SMP Debian 5.8.7-1 (2020-09-05) x86_64 GNU/Linux

@kalyanac
Copy link
Contributor

kalyanac commented Oct 7, 2020

Can you try with llvm-objdump? pprof will now prefer llvm-objdump if it is available. That should help the test pass.

@zhsj
Copy link
Contributor Author

zhsj commented Oct 8, 2020

llvm-objdump works.

@zhsj zhsj closed this Oct 8, 2020
@kalyanac
Copy link
Contributor

kalyanac commented Oct 8, 2020

Thank you for checking. I think it still makes sense to merge this PR. llvm-objdump installation may not always have all the backends available. Instead of trying to parse the support and run the test, we will limit the test to only x86-64.

If you reopen the PR, I can merge it. If not, I will submit a new PR.

@kalyanac kalyanac reopened this Oct 8, 2020
@kalyanac kalyanac merged commit e3cc412 into google:master Oct 8, 2020
@zhsj zhsj deleted the patch-1 branch October 9, 2020 14:33
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
Disable disassembly tests on non-amd64 architectures.
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.

3 participants