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

8253757: Add LLVM-based backend for hsdis #392

Closed
wants to merge 8 commits into from

Conversation

luhenry
Copy link
Member

@luhenry luhenry commented Sep 29, 2020

When bringing up Hotspot onto new platforms, it is not always possible to compile hsdis because gcc is not yet available. For example, for Windows-AArch64 and macOS-AArch64.

For some such platforms, it is possible to use LLVM as an alternative backend as it also supports a disassembler feature.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (3/3 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

Download

$ git fetch https://git.openjdk.java.net/jdk pull/392/head:pull/392
$ git checkout pull/392

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 29, 2020

👋 Welcome back luhenry! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 29, 2020
@openjdk
Copy link

openjdk bot commented Sep 29, 2020

@luhenry To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing

@luhenry
Copy link
Member Author

luhenry commented Sep 29, 2020

/label add hotspot-compiler

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 29, 2020
@openjdk
Copy link

openjdk bot commented Sep 29, 2020

@luhenry
The hotspot-compiler label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Sep 29, 2020

Webrevs

src/utils/hsdis-llvm/README Outdated Show resolved Hide resolved
@luhenry
Copy link
Member Author

luhenry commented Sep 30, 2020

@navyxliu I've merged the sources into src/utils/hsdis and added support to build it in the Makefile.

src/utils/hsdis/hsdis.cpp Outdated Show resolved Hide resolved
src/utils/hsdis/Makefile Outdated Show resolved Hide resolved
@luhenry
Copy link
Member Author

luhenry commented Oct 1, 2020

/label add hotspot

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 1, 2020
@openjdk
Copy link

openjdk bot commented Oct 1, 2020

@luhenry
The hotspot label was successfully added.

@magicus
Copy link
Member

magicus commented Oct 2, 2020

This is an interesting suggestion. There is a similar attempt at replacing binutils with capstone in https://bugs.openjdk.java.net/browse/JDK-8188073, which unfortunately has not seen much progress due to lack of resources; I don't know if you are aware of that? There is also a (extremely low priority) effort to rewrite the hsdis makefile to be part of the normal build system, see e.g. https://bugs.openjdk.java.net/browse/JDK-8208495. Neither of these should be any blocker for your change, but I think it might be good if you know about them.

I have couple of concerns with your patch. One is the method in which LLVM is selected instead of binutils; afaict this depends on having the LLVM variable set when executing the makefile. At the very least, this should be documented in the README. I don't think any more complicated configuration is really necessary at this point. With full integration with the build system, a more user-friendly way of selecting hsdis backend should be implemented, though.

Second, and I don't know if this is an artifact of git/github/the new skara tooling, but if you renamed hsdis.c to hsdis.cpp, this relationship does not show up, not even in the generated webrevs. Instead they are considered a new + a deleted file. This makes it hard to see what code changes you have done in that file.

And third; have you tested that your changes (both changing the main file from C to C++, and any code changes in it) does not break the old binutils functionality? Afaic there are no test suites for exercising hsdis :-( so manual ad-hoc testing is likely needed.

@magicus
Copy link
Member

magicus commented Oct 2, 2020

/label build

@openjdk openjdk bot added the build build-dev@openjdk.org label Oct 2, 2020
@openjdk
Copy link

openjdk bot commented Oct 2, 2020

@magicus
The build label was successfully added.

@YaSuenag
Copy link
Member

YaSuenag commented Oct 7, 2020

Can you separate LLVM and binutils from hsdis.cpp?

I guess you say that the problem is both GCC and binutils are not available on Windows AArch64. Is it right?
1 question: binutils seems to support Windows AArch64. Did you try recently binutils? If we can use binutils on Windows AArch64, you can fix makefile only. https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248

@navyxliu
Copy link
Member

navyxliu commented Oct 7, 2020

IMHO, it's great to have an alternative disassembler. I personally had better experience using llvm MC when I decoded aarch64 and AVX instructions than BFD. Another argument is that LLVM toolchain is supposed to provide the premium experience on non-gnu platforms such as FreeBSD.

@luhenry I tried to build it with LLVM10.0.1
on my x86_64, ubuntu, I ran into a small problem. here is how I build.
$make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ LLVM=/opt/llvm/

I can't meet this condition because Makefile defines LIBOS_linux.

#elif defined(LIBOS_Linux) && defined(LIBARCH_amd64)
    return "x86_64-pc-linux-gnu";

Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower case)and then
CPPFLAGS += -DLIBOS_$(OS) -DLIBOS=\"$(OS)\" -DLIBARCH_$(LIBARCH) -DLIBARCH=\"$(LIBARCH)\" -DLIB_EXT=\"$(LIB_EXT)\"

In hsdis.cpp, native_target_triple needs to match whatever Makefile defined. With that fix, I generate llvm version hsdis-amd64.so and it works flawlessly

@navyxliu
Copy link
Member

navyxliu commented Oct 7, 2020

/reviewer remove navyxliu

@openjdk
Copy link

openjdk bot commented Oct 7, 2020

@navyxliu Only the author (@luhenry) is allowed to issue the reviewer command.

@lewurm
Copy link
Member

lewurm commented Oct 8, 2020

1 question: binutils seems to support Windows AArch64. Did you try recently binutils? If we can use binutils on Windows AArch64, you can fix makefile only. https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248

This is armv7, I don't see any support for armv8/AArch64 in dlltool.c.

@luhenry
Copy link
Member Author

luhenry commented Oct 8, 2020

@magicus

This is an interesting suggestion. There is a similar attempt at replacing binutils with capstone in https://bugs.openjdk.java.net/browse/JDK-8188073, which unfortunately has not seen much progress due to lack of resources; I don't know if you are aware of that? There is also a (extremely low priority) effort to rewrite the hsdis makefile to be part of the normal build system, see e.g. https://bugs.openjdk.java.net/browse/JDK-8208495. Neither of these should be any blocker for your change, but I think it might be good if you know about them.

I was not aware of the effort to use capstone to replace/complement binutils in hsdis. I wonder how easy it is to port capstone to platforms in case it doesn't support them.

I have couple of concerns with your patch. One is the method in which LLVM is selected instead of binutils; afaict this depends on having the LLVM variable set when executing the makefile. At the very least, this should be documented in the README. I don't think any more complicated configuration is really necessary at this point. With full integration with the build system, a more user-friendly way of selecting hsdis backend should be implemented, though.

I'll add documentation to the Makefile. And I agree, I would prefer not to have to go through the whole build integration to integrate the support for LLVM.

Second, and I don't know if this is an artifact of git/github/the new skara tooling, but if you renamed hsdis.c to hsdis.cpp, this relationship does not show up, not even in the generated webrevs. Instead they are considered a new + a deleted file. This makes it hard to see what code changes you have done in that file.

That is Git not detecting enough similarities between the two files. I could probably hack my way around and find a way to reduce the code diff if that's something you want.

And third; have you tested that your changes (both changing the main file from C to C++, and any code changes in it) does not break the old binutils functionality? Afaic there are no test suites for exercising hsdis :-( so manual ad-hoc testing is likely needed.

I've tested on Linux-x86_64 and Linux-AArch64 on top of Windows-AArch64 and macOS-AArch64, and checked that both the binutils builds and works as previously and that the LLVM-based hsdis has an equivalent output.

@luhenry
Copy link
Member Author

luhenry commented Oct 8, 2020

@navyxliu

@luhenry I tried to build it with LLVM10.0.1
on my x86_64, ubuntu, I ran into a small problem. here is how I build.
$make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ LLVM=/opt/llvm/

I can't meet this condition because Makefile defines LIBOS_linux.

#elif defined(LIBOS_Linux) && defined(LIBARCH_amd64)
return "x86_64-pc-linux-gnu";

Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower case)and then
CPPFLAGS += -DLIBOS_$(OS) -DLIBOS="$(OS)" -DLIBARCH_$(LIBARCH) -DLIBARCH="$(LIBARCH)" -DLIB_EXT="$(LIB_EXT)"

Interestingly, I did it this way because on my machine LIBOS_Linux would get defined instead of LIBOS_linux. I tried on WSL which might explain the difference. Could you please share more details on what environment you are using?

In hsdis.cpp, native_target_triple needs to match whatever Makefile defined. With that fix, I generate llvm version hsdis-amd64.so and it works flawlessly

I'm not sure I understand what you mean. Are you saying we should define the native target triple based on the variables in the Makefile?

A difficulty I ran into is that there is not always a 1-to-1 mapping between the autoconf/gcc target triple and the LLVM one. For example. you pass x86_64-gnu-linux to the OpenJDK's configure script, but the equivalent target triple for LLVM is x86_64-pc-linux-gnu.

Since my plan isn't to use LLVM as the default for all platforms, and because there aren't that many combinations of target OS/ARCH, I am taking the approach of hardcoding the combinations we care about in hsdis.cpp.

@luhenry
Copy link
Member Author

luhenry commented Oct 8, 2020

/reviewer remove navyxliu

@openjdk
Copy link

openjdk bot commented Oct 8, 2020

@luhenry Could not parse navyxliu as a valid reviewer.
Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

  • /reviewer credit @openjdk-bot
  • /reviewer credit duke
  • /reviewer credit @user1 @user2

@navyxliu
Copy link
Member

navyxliu commented Oct 8, 2020

@navyxliu

@luhenry I tried to build it with LLVM10.0.1
on my x86_64, ubuntu, I ran into a small problem. here is how I build.
$make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ LLVM=/opt/llvm/
I can't meet this condition because Makefile defines LIBOS_linux.
#elif defined(LIBOS_Linux) && defined(LIBARCH_amd64)
return "x86_64-pc-linux-gnu";
Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower case)and then
CPPFLAGS += -DLIBOS_$(OS) -DLIBOS="$(OS)" -DLIBARCH_$(LIBARCH) -DLIBARCH="$(LIBARCH)" -DLIB_EXT="$(LIB_EXT)"

Interestingly, I did it this way because on my machine LIBOS_Linux would get defined instead of LIBOS_linux. I tried on WSL which might explain the difference. Could you please share more details on what environment you are using?

I am using ubuntu 18.04.

OS = $(shell uname) does initialize OS=Linux in the first place, but later OS is set to "linux" at line 88 of https://openjdk.github.io/cr/?repo=jdk&pr=392&range=05#new-0

At line 186, -DLIBOS_linux -DLIBOS="linux" ... It doesn't match line 564 of https://openjdk.github.io/cr/?repo=jdk&pr=392&range=05#new-2

in my understanding, C/C++ macros are all case sensitive. I got #error "unknown platform" because of Linux/linux discrepancy.

In hsdis.cpp, native_target_triple needs to match whatever Makefile defined. With that fix, I generate llvm version hsdis-amd64.so and it works flawlessly

I'm not sure I understand what you mean. Are you saying we should define the native target triple based on the variables in the Makefile?

A difficulty I ran into is that there is not always a 1-to-1 mapping between the autoconf/gcc target triple and the LLVM one. For example. you pass x86_64-gnu-linux to the OpenJDK's configure script, but the equivalent target triple for LLVM is x86_64-pc-linux-gnu.

Since my plan isn't to use LLVM as the default for all platforms, and because there aren't that many combinations of target OS/ARCH, I am taking the approach of hardcoding the combinations we care about in hsdis.cpp.

@magicus
Copy link
Member

magicus commented Oct 26, 2020

Since I found it close to impossible to review the changes when I could not get a diff with the changes done to hsdis.c/cpp, I created a webrev which shows these changes. I made this by renaming hsdis.cpp back to hsdis.c, and then webrev could match it up. It is available here:

http://cr.openjdk.java.net/~ihse/hsdis-llvm-backend-diff-webrev/

@magicus
Copy link
Member

magicus commented Oct 26, 2020

Some notes (perhaps most to myself) about how this ties into the existing hsdis implementation, and with JDK-8188073 (Capstone porting).

When printing disassembly from hotspot, the current solution tries to locate and load the hsdis library, which prints disassembly using bfd. The reason for using the separate library approach is, as far as I can understand, perhaps a mix of both incompatible licensing for bfd, and a wish to not burden the jvm library with additional bloat which is needed only for debugging.

The Capstone approach, in the prototype patch presented by Jorn in the issue, is to create a new capstonedis library, and dispatch to it instead of hsdis.

The approach used in this patch is to refactor the existing hsdis library into an abstract base class for hsdis backends, with two concrete implementations, one for bfd and one for llvm.

Unfortunately, I think the resulting code in hsdis.cpp in this patch is hard to read and understand.

@magicus
Copy link
Member

magicus commented Oct 26, 2020

I think a proper solution to both this and the Capstone implementation is to provide a proper framework for selecting the hsdis backend as a first step, and refactor the existing bfd implementation as the first such backend. After that, we can add llvm and capstone as alternative hsdis backend implementations.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2020

@luhenry This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@magicus
Copy link
Member

magicus commented Nov 23, 2020

FWIW, I started working on a framework which would add support for selectable backends for hsdis. Unfortunately it was not as simple as I initially thought, so I had to put it on hold while directing my time to working on the winenv patch instead.

I believe the proper way forward is to get the "selectable hsdis backend" framework in place, and then retrofit this patch to add LLVM support in that framework. If this means that this PR should be closed, or kept open until this is done, I don't know.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 21, 2020

@luhenry This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@magicus
Copy link
Member

magicus commented Dec 21, 2020

Nag nag bot...

I'm away for the holidays now, but I intend to pick up the work on the selectable hsdis backend when I return. So while this exact code is unlikely to be the one merged, it's good to have it open to allow me to double check the solutions picked here.

@luhenry
Copy link
Member Author

luhenry commented Dec 21, 2020

@magicus that PR fell off my plate, but I'm happy to pick it up based on your "selectable hsdis backend" change.

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 18, 2021

@luhenry This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 16, 2021

@luhenry This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org rfr Pull request is ready for review
5 participants