Skip to content

fix: display disassembly correctly on aarch64 #726

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

pcc
Copy link

@pcc pcc commented May 26, 2025

On aarch64 (and possibly other architectures) objdump splits the instruction mnemonic from the operands with a tab so the existing code would only show the mnemonic. Change the code so that it ignores tabs after the first two so that the whole instruction is displayed.

@GitMensch
Copy link
Contributor

Sounds good. Mind to share a screen before/after?

Note: there's ongoing work to use a disassembler library, so the fix may only be temporary...

@pcc
Copy link
Author

pcc commented May 26, 2025

Sure, here is before/after

Screenshot from 2025-05-26 15-36-47
Screenshot from 2025-05-26 15-39-52

Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

lgtm

@milianw
Copy link
Member

milianw commented May 27, 2025

hm the test failures are related, can you please have a look at those?

FAIL!  : TestDisassemblyOutput::testParse(objdump2.txt) 'line.branchVisualisation.isEmpty()' returned FALSE. ()

Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

see previous comment

@pcc
Copy link
Author

pcc commented May 27, 2025

Thanks. I was having some trouble figuring out how to run the tests yesterday, make test failed with missing binariers so it seemed like I would need to build the tests (and their dependencies) manually. I'm away from the machine that I was using for developing hotspot but from the CI output it looks like the dev-asan target will build all the tests? It may be worth documenting that somewhere, e.g. in CONTRIBUTING.md.

@milianw
Copy link
Member

milianw commented May 28, 2025

hm the normal build should also include tests, I run those via ctest --output-on-failure -j8 or similar myself. the CI clazy/clang-tidy jobs are sadly broken, please ignore those for now, but otherwise it should just work?

generally though, if you figure something out that you would have liked to see documented more clearly, by all means please create a PR to add the missing documentation after you figured it out!

thanks a lot for your contributions, I'll try to find some more time for your other changes the next days - please nag me if you don't hear in time, I'm notoriously busy with other projects these days

On aarch64 (and possibly other architectures) objdump splits the
instruction mnemonic from the operands with a tab so the existing code
would only show the mnemonic. Change the code so that it ignores tabs
after the first two so that the whole instruction is displayed.
@pcc
Copy link
Author

pcc commented Jun 1, 2025

hm the normal build should also include tests, I run those via ctest --output-on-failure -j8 or similar myself. the CI clazy/clang-tidy jobs are sadly broken, please ignore those for now, but otherwise it should just work?

Figured it out. Right, the tests are built by default, but I was building with Nix which was disabling them unbeknownst to me. I figured out why they were failing (sent #729), sent them a PR to enable tests (NixOS/nixpkgs#413058) and finally fixed the failing tests here.

generally though, if you figure something out that you would have liked to see documented more clearly, by all means please create a PR to add the missing documentation after you figured it out!

I guess the only thing worth documenting is that ctest should be used to run tests, since this might not be obvious to everyone (some CMake using projects like LLVM have their own testing tools). Sent #730 for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants