Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Fri, 18 Feb 2022 11:44:04 GMT, Magnus Ihse Bursie wrote: > Third time's a charm! After the two previous closed PRs for this issue, I > think this functionality is finally ready to enter mainline. :) > > This code is at it's core the same as the previous PR. The main C++ hsdis > implementation is still the one @luhenry wrote, with some changes. As in the > previous PR, I extracted the LLVM-specific part into a separate file. In > addition to the last PR, I have also fixed a warning, and added a patch > inspired by @nick-arm for getting past instructions unknown to LLVM. > > Thanks to the prototype written by @JornVernee (and his graciously providing > me with a working version of LLVM build for Windows), this PR now has full > support for LLVM on Windows (as well as Linux and macOS). > > Finally, I have cleaned up the integration in autoconf and Hsdis.gmk, and > written a thorough guide in the README on how to build witth LLVM, on Windows > and on saner platforms. :) > > I'm pretty sure this means that all comments and criticism in the previous PR > has been addressed. > > Huge thanks to everyone who has helped me with getting this PR in place. I > have a hard time remember a feature that has been so tricky to get in place, > for something to seemingly simple... Marked as reviewed by luhenry (Author). Thanks again for pushing that forward! It's always good to have an alternative especially for porting to new platforms and architectures. - PR: https://git.openjdk.java.net/jdk/pull/7531
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Fri, 18 Feb 2022 11:44:04 GMT, Magnus Ihse Bursie wrote: > Third time's a charm! After the two previous closed PRs for this issue, I > think this functionality is finally ready to enter mainline. :) > > This code is at it's core the same as the previous PR. The main C++ hsdis > implementation is still the one @luhenry wrote, with some changes. As in the > previous PR, I extracted the LLVM-specific part into a separate file. In > addition to the last PR, I have also fixed a warning, and added a patch > inspired by @nick-arm for getting past instructions unknown to LLVM. > > Thanks to the prototype written by @JornVernee (and his graciously providing > me with a working version of LLVM build for Windows), this PR now has full > support for LLVM on Windows (as well as Linux and macOS). > > Finally, I have cleaned up the integration in autoconf and Hsdis.gmk, and > written a thorough guide in the README on how to build witth LLVM, on Windows > and on saner platforms. :) > > I'm pretty sure this means that all comments and criticism in the previous PR > has been addressed. > > Huge thanks to everyone who has helped me with getting this PR in place. I > have a hard time remember a feature that has been so tricky to get in place, > for something to seemingly simple... Build changes look good. I can't comment on the hsdis implementation. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7531
RFR: 8253757: Add LLVM-based backend for hsdis
Third time's a charm! After the two previous closed PRs for this issue, I think this functionality is finally ready to enter mainline. :) This code is at it's core the same as the previous PR. The main C++ hsdis implementation is still the one @luhenry wrote, with some changes. As in the previous PR, I extracted the LLVM-specific part into a separate file. In addition to the last PR, I have also fixed a warning, and added a patch inspired by @nick-arm for getting past instructions unknown to LLVM. Thanks to the prototype written by @JornVernee (and his graciously providing me with a working version of LLVM build for Windows), this PR now has full support for LLVM on Windows (as well as Linux and macOS). Finally, I have cleaned up the integration in autoconf and Hsdis.gmk, and written a thorough guide in the README on how to build witth LLVM, on Windows and on saner platforms. :) I'm pretty sure this means that all comments and criticism in the previous PR has been addressed. Huge thanks to everyone who has helped me with getting this PR in place. I have a hard time remember a feature that has been so tricky to get in place, for something to seemingly simple... - Commit messages: - Update description on runtime requirements for LLVM - Fix warning on Windows (as opposed to hiding it) - Add LLVM backend to hsdis. Portions of this patch contributed by luhenry, jvernee and ngasson. Changes: https://git.openjdk.java.net/jdk/pull/7531/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7531&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253757 Stats: 495 lines in 4 files changed: 490 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7531.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7531/head:pull/7531 PR: https://git.openjdk.java.net/jdk/pull/7531
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v8]
On Wed, 16 Feb 2022 14:39:49 GMT, Magnus Ihse Bursie wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Add section on using LLVM > - Allow override of LLVM location using --with-llvm This PR has just gotten too messy. :-( I'll close this one for now, focus on getting the (much simpler) Capstone backend integrated, and then I can come back and revisit this functionality, but in a fresh new PR. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v8]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Add section on using LLVM - Allow override of LLVM location using --with-llvm - Changes: - all: https://git.openjdk.java.net/jdk/pull/5920/files - new: https://git.openjdk.java.net/jdk/pull/5920/files/467e1bb1..5ea9bc0d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=06-07 Stats: 37 lines in 2 files changed: 32 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v7]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Make sure install-hsdis also copies to image - Changes: - all: https://git.openjdk.java.net/jdk/pull/5920/files - new: https://git.openjdk.java.net/jdk/pull/5920/files/47cf9a1b..467e1bb1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=05-06 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v6]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Restructure README to prepare for section on LLVM - Changes: - all: https://git.openjdk.java.net/jdk/pull/5920/files - new: https://git.openjdk.java.net/jdk/pull/5920/files/a4d9896e..47cf9a1b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=04-05 Stats: 58 lines in 1 file changed: 30 ins; 22 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v5]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Fix install-hsdis warning - Changes: - all: https://git.openjdk.java.net/jdk/pull/5920/files - new: https://git.openjdk.java.net/jdk/pull/5920/files/7d866725..a4d9896e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=03-04 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v4]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Handle unknown instructions from LLVM. Solution suggested by ngasson. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5920/files - new: https://git.openjdk.java.net/jdk/pull/5920/files/9f2ab6d9..7d866725 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=02-03 Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v3]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Find llvm-config on mac-aarch64 homebrew as well - Changes: - all: https://git.openjdk.java.net/jdk/pull/5920/files - new: https://git.openjdk.java.net/jdk/pull/5920/files/c16dcb21..9f2ab6d9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis [v2]
> This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' into hsdis-backend-llvm - Create hsdis backend using LLVM - Changes: https://git.openjdk.java.net/jdk/pull/5920/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=01 Stats: 406 lines in 6 files changed: 398 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Yes bot, thank you for the reminder. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 17 Nov 2021 13:17:21 GMT, Magnus Ihse Bursie wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > > Yeah bot, I'm still working on it. > @magicus @JornVernee Thank you for starting this PR and for the notes on the > Windows changes. I'm trying them out on macOS and Windows. > > @theRealAph I'm not as familiar with the mac platform. Did you need to do > anything special to address these errors? I was adding > -L/opt/homebrew/opt/llvm/lib to the HSDIS_LDFLAGS. > > ``` > "_LLVMCreateDisasm", referenced from: > hsdis_backend::hsdis_backend(unsigned long, unsigned long, unsigned > char*, unsigned long, void* (*)(void*, char const*, void*), void*, int > (*)(void*, char const*, ...), void*, char const*, int) in hsdis-llvm.o > "_LLVMDisasmDispose", referenced from: > hsdis_backend::~hsdis_backend() in hsdis-llvm.o > ... > ld: symbol(s) not found for architecture arm64 > clang-13: error: linker command failed with exit code 1 (use -v to see > invocation) > ``` Turns out I just needed to add /opt/homebrew/opt/llvm/bin to my PATH. I can now use --with-hsdis=llvm. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 17 Nov 2021 13:17:21 GMT, Magnus Ihse Bursie wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > > Yeah bot, I'm still working on it. @magicus @JornVernee Thank you for starting this PR and for the notes on the Windows changes. I'm trying them out on macOS and Windows. @theRealAph I'm not as familiar with the mac platform. Did you need to do anything special to address these errors? I was adding -L/opt/homebrew/opt/llvm/lib to the HSDIS_LDFLAGS. ```Undefined symbols for architecture arm64: "_LLVMCreateDisasm", referenced from: hsdis_backend::hsdis_backend(unsigned long, unsigned long, unsigned char*, unsigned long, void* (*)(void*, char const*, void*), void*, int (*)(void*, char const*, ...), void*, char const*, int) in hsdis-llvm.o "_LLVMDisasmDispose", referenced from: hsdis_backend::~hsdis_backend() in hsdis-llvm.o ... ld: symbol(s) not found for architecture arm64 clang-13: error: linker command failed with exit code 1 (use -v to see invocation) - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Yeah bot, I'm still working on it. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Tue, 19 Oct 2021 04:47:52 GMT, Nick Gasson wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > > Rather than introduce a new dependency on all of LLVM you might like to take > a look at Capstone - https://www.capstone-engine.org/ . AFAIK the > disassemblers are generated from the same LLVM architecture description files > so the instruction coverage should be the same but the library is much more > lightweight. It's packaged in most Linux distributions and there's pre-built > Windows binaries available. @nick-arm That'd introduce a new dependency to Capstone. ;-) But your suggestion is excellent -- in fact, I have a branch in my personal fork that builds hsdis with Capstone as backend. I just scheduled for myself to submit this PR first. (Which maybe was a mistake; it was obviously more tricky to get right than I anticipated.) I might reconsider that choice and let this PR wait until I've pushed the Capstone backend first, instead. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Rather than introduce a new dependency on all of LLVM you might like to take a look at Capstone - https://www.capstone-engine.org/ . AFAIK the disassemblers are generated from the same LLVM architecture description files so the instruction coverage should be the same but the library is much more lightweight. It's packaged in most Linux distributions and there's pre-built Windows binaries available. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote:
> This patch expands the newly added system for hsdis backends to include LLVM.
>
> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry,
> as published in the never integrated PR
> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out
> the binutils-based part of it.)
>
> Unfortunately I have not been able to make this work properly on Windows.
> With some additional flags I made it compile without complaints, but it
> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I
> tried to load the library. This is somewhat ironic, since the initial
> implementation was created by Ludovic for the very purpose of using it on
> Windows.
>
> The lack of Windows support in this patch does not mean it is impossible to
> get it to work, just that I need to co-operate with someone who has more
> experience of compiling LLVM on Windows, and/or are more eager to get this
> combination to work.
The problem is `LLVMDisasmInstruction()` returns zero size as soon as it hits
an instruction it doesn't understand. Something kludgy like this works:
diff --git a/src/utils/hsdis/llvm/hsdis-llvm.cpp
b/src/utils/hsdis/llvm/hsdis-llvm.cpp
index a491082f14fa..3c50ee8e3b40 100644
--- a/src/utils/hsdis/llvm/hsdis-llvm.cpp
+++ b/src/utils/hsdis/llvm/hsdis-llvm.cpp
@@ -307,6 +307,10 @@ class hsdis_backend : public hsdis_backend_base {
virtual size_t decode_instruction(uintptr_t p, uintptr_t start, uintptr_t
end) {
char buf[128];
size_t size = LLVMDisasmInstruction(_dcontext, (uint8_t*)p, (uint64_t)(end
- start), (uint64_t)p, buf, sizeof(buf));
+if (size == 0 && end - start >= 4) {
+ snprintf(buf, sizeof(buf), "\t.word\t#0x%08x", *(uint32_t*)p);
+ size = 4;
+}
if (size > 0) {
(*_printf_callback)(_printf_stream, "%s", buf);
}
0x94685454: br x8
0x94685458: .word #0x93e3f4c0
0x9468545c: udf #0x
[Exception Handler]
0x94685460: adrpx8, #-0x795000 ; {runtime_call
handle_exception_from_callee Runtime1 stub}
-
PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Mon, 18 Oct 2021 09:10:32 GMT, Andrew Haley wrote: > >Can you describe a reproducer? > Sure. Create a .hotspot_compiler file containing print > java.lang.String::checkIndex then Sorry, thinko. You don't need the .hotspot_compiler file - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote:
> This patch expands the newly added system for hsdis backends to include LLVM.
>
> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry,
> as published in the never integrated PR
> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out
> the binutils-based part of it.)
>
> Unfortunately I have not been able to make this work properly on Windows.
> With some additional flags I made it compile without complaints, but it
> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I
> tried to load the library. This is somewhat ironic, since the initial
> implementation was created by Ludovic for the very purpose of using it on
> Windows.
>
> The lack of Windows support in this patch does not mean it is impossible to
> get it to work, just that I need to co-operate with someone who has more
> experience of compiling LLVM on Windows, and/or are more eager to get this
> combination to work.
On 10/16/21 09:13, Magnus Ihse Bursie wrote:
> @theRealAph We should not push broken code, and we should not break the
> existing build of hsdis. I fully agree with this. I will not push this patch
> until all reviewers are happy, so you don't need to worry about that. :)
>
> My initial plan was to get the unix platforms working in this push, and
> tackle Windows later on, but it seems now that it's better to keep this PR
> around for a bit longer instead, and fold Windows support into it as well.
> (Which means I'll wait for Monica to return and being able to test and help
> out.)
>
> I need to understand better why things are failing for you. Can you describe
> a reproducer?
Sure. Create a .hotspot_compiler file containing
print java.lang.String::checkIndex
then
./build/macosx-aarch64-server-fastdebug/jdk/bin/java -XX:+PrintAssembly
-version
GNU disassembly of java.lang.String::isLatin1 prints in full, ending thusly
[Exception Handler]
0x00010a72c340: mov x19, #0xdead// #57005
; {no_reloc}
0x00010a72c344: mov x2, #0xa// #10
0x00010a72c348: mov x4, #0xdead // #57005
0x00010a72c34c: mov x5, #0xdead // #57005
0x00010a72c350: adrpx8, 0x00010a1c3000 ; {runtime_call
handle_exception_from_callee Runtime1 stub}
0x00010a72c354: add x8, x8, #0x1c0
0x00010a72c358: blr x8
0x00010a72c35c: dcps1 #0xdeae
0x00010a72c360: .inst 0x0995f68e ; undefined
0x00010a72c364: udf #1
[Deopt Handler Code]
0x00010a72c368: adr x30, 0x00010a72c368
0x00010a72c36c: adrpx8, 0x00010a26c000 ; {runtime_call
DeoptimizationBlob}
0x00010a72c370: add x8, x8, #0xbc0
0x00010a72c374: br x8
LLVM disassembly dies at
[Exception Handler]
0x00010672c340: mov x19, #0xdead; {no_reloc}
0x00010672c344: mov x2, #0xa
0x00010672c348: mov x4, #0xdead
0x00010672c34c: mov x5, #0xdead
0x00010672c350: adrpx8, #-5672960 ; {runtime_call
handle_exception_from_callee Runtime1 stub}
0x00010672c354: add x8, x8, #0x1c0
0x00010672c358: blr x8
0x00010672c35c: dcps1 #0xdeae
0x00010672c360:
This is llvm 12.0.1 from homebrew.
-
PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Fri, 15 Oct 2021 15:36:14 GMT, Andrew Haley wrote: >> The value add of this LLVM-based hsdis is two-fold: >> - It supports platforms that aren't supported by binutils (Windows-AArch64 >> for example) >> - The license being more permissive would allow to build it as part of the >> OpenJDK build more easily (and even maybe ship it?) >> >> LLVM has a strong track record of supporting new platforms (Windows-AArch64 >> and macOS-AArch64 for example, mostly because of investment from Microsoft >> and Apple respectively), and `hsdis` is a necessary tool for porting the >> OpenJDK to any new platform. Since the maintenance is fairly low (small >> codebase, small and knowledgable user base), I would be biased towards >> including it with appropriate warnings. > >> Since the maintenance is fairly low (small codebase, small and knowledgable >> user base), I would be biased towards including it with appropriate warnings. > > I don't think we should commit code that we know is broken. I don't believe > that this view might be controversial. > Maybe someone should try to reproduce the failure I've seen, and then we > should look at fixing it. Maybe it's a local problem. > > Also, this patch breaks all current hsdis builds that follow the installation > instructions. Either we get revised instructions or the build should be fixed. @theRealAph We should not push broken code, and we should not break the existing build of hsdis. I fully agree with this. I will not push this patch until all reviewers are happy, so you don't need to worry about that. :) My initial plan was to get the unix platforms working in this push, and tackle Windows later on, but it seems now that it's better to keep this PR around for a bit longer instead, and fold Windows support into it as well. (Which means I'll wait for Monica to return and being able to test and help out.) I need to understand better why things are failing for you. Can you describe a reproducer? - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Fri, 15 Oct 2021 08:20:05 GMT, Ludovic Henry wrote: > Since the maintenance is fairly low (small codebase, small and knowledgable > user base), I would be biased towards including it with appropriate warnings. I don't think we should commit code that we know is broken. I don't believe that this view might be controversial. Maybe someone should try to reproduce the failure I've seen, and then we should look at fixing it. Maybe it's a local problem. Also, this patch breaks all current hsdis builds that follow the installation instructions. Either we get revised instructions or the build should be fixed. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
Hi all, I am on vacation right now, but we can take a look next week and see how we can support the LLVM backend for hsdis. It is much required by Windows-AArch64 and we would hate for all the good work to not make it upstream. Monica Sent from my Arm powered smart device. On Thu, Oct 14, 2021, 9:07 AM Andrew Haley wrote: > On Wed, 13 Oct 2021 07:26:21 GMT, Ludovic Henry > wrote: > > >> This patch expands the newly added system for hsdis backends to include > LLVM. > >> > >> The actual code in hsdis-llvm.cpp is based heavily on the work by > @luhenry, as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped > out the binutils-based part of it.) > >> > >> Unfortunately I have not been able to make this work properly on > Windows. With some additional flags I made it compile without complaints, > but it caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` > when I tried to load the library. This is somewhat ironic, since the > initial implementation was created by Ludovic for the very purpose of using > it on Windows. > >> > >> The lack of Windows support in this patch does not mean it is > impossible to get it to work, just that I need to co-operate with someone > who has more experience of compiling LLVM on Windows, and/or are more eager > to get this combination to work. > > > > Very happy to see it landing :) Thank you! > > > > I don't have access to a windows machine, and even less a > Windows-AArch64 machine. @lewurm would you be able to take a look? > > > As for LLVM not giving you a good user experience; I can't really tell > what's wrong. Apparently @luhenry (and @JornVernee I believe) has used > this. I'm not really the target audience myself; I'm only trying to make it > possible to use. If it is so severly limited as you say maybe this isn't > worth pursuing. Some feedback from those who have tested it would be > appeciated here, to help med understand if this patch should be dropped. > > I don't think it should be dropped, but I imagine that the bugs can be > fixed. If LLVM's disassembler always dies as soon as it sees something it > can't recognize, I'm astonished. Maybe the LLVM I'm using is bad. > > - > > PR: https://git.openjdk.java.net/jdk/pull/5920 >
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. The value add of this LLVM-based hsdis is two-fold: - It supports platforms that aren't supported by binutils (Windows-AArch64 for example) - The license being more permissive would allow to build it as part of the OpenJDK build more easily (and even maybe ship it?) LLVM has a strong track record of supporting new platforms (Windows-AArch64 and macOS-AArch64 for example, mostly because of investment from Microsoft and Apple respectively), and `hsdis` is a necessary tool for porting the OpenJDK to any new platform. Since the maintenance is fairly low (small codebase, small and knowledgable user base), I would be biased towards including it with appropriate warnings. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 14 Oct 2021 14:04:12 GMT, Andrew Haley wrote: > Some feedback from those who have tested it would be appeciated here I haven't really tested it beyond building the lib and seeing if assembly was output instead of just bytes, so I can't really comment on that I'm afraid. Since the binutils hsdis wasn't buildable on Windows for me in the past, I've always been using the [fcml based hsdis](https://github.com/swojtasiak/fcml-lib/tree/master/example/hsdis) on Windows. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 07:26:21 GMT, Ludovic Henry wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > > Very happy to see it landing :) Thank you! > > I don't have access to a windows machine, and even less a Windows-AArch64 > machine. @lewurm would you be able to take a look? > As for LLVM not giving you a good user experience; I can't really tell what's > wrong. Apparently @luhenry (and @JornVernee I believe) has used this. I'm not > really the target audience myself; I'm only trying to make it possible to > use. If it is so severly limited as you say maybe this isn't worth pursuing. > Some feedback from those who have tested it would be appeciated here, to help > med understand if this patch should be dropped. I don't think it should be dropped, but I imagine that the bugs can be fixed. If LLVM's disassembler always dies as soon as it sees something it can't recognize, I'm astonished. Maybe the LLVM I'm using is bad. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 14 Oct 2021 09:46:47 GMT, Andrew Haley wrote:
>> This patch expands the newly added system for hsdis backends to include LLVM.
>>
>> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry,
>> as published in the never integrated PR
>> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out
>> the binutils-based part of it.)
>>
>> Unfortunately I have not been able to make this work properly on Windows.
>> With some additional flags I made it compile without complaints, but it
>> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I
>> tried to load the library. This is somewhat ironic, since the initial
>> implementation was created by Ludovic for the very purpose of using it on
>> Windows.
>>
>> The lack of Windows support in this patch does not mean it is impossible to
>> get it to work, just that I need to co-operate with someone who has more
>> experience of compiling LLVM on Windows, and/or are more eager to get this
>> combination to work.
>
> So I figured out how to build it with `make build-hsdis`. The instructions
> need to be fixed.
>
> Two problems: firstly, the library seems to be built with the wrong name, so
> the runtime doesn't find it. I had to use
> `ln -sf ~/lib/libhsdis.dylib ~/lib/hsdis-aarch64.dylib`
> to get it to work.
>
> More severely, the disassembly is truncated, so lots of stuff doesn't work.
> The binutils-based hsdis prints
>
>
> 0x0001105665b4: stp wzr, wzr, [x20, #60];*putfield
> preCounterBlock {reexecute=0 rethrow=0 return_oop=0}
> ; -
> com.sun.crypto.provider.GaloisCounterMode$GCMEngine::@80 (line 673)
> ; -
> com.sun.crypto.provider.GaloisCounterMode$GCMDecrypt::@8 (line 1346)
> ; -
> com.sun.crypto.provider.GaloisCounterMode::checkInit@60 (line 329)
> ;; B6: # out( B242 B7 ) <- in( B192 B5 ) Freq: 0.99
> 0x0001105665b8: cbnzx21, 0x0001105665c8
> ;; null oop passed to encode_heap_oop_not_null2
> 0x0001105665bc: dcps1 #0xdeae
> 0x0001105665c0: .inst 0x082adca6 ; undefined
> 0x0001105665c4: udf #1
> 0x0001105665c8: lsr x10, x21, #3;*invokevirtual
> getLongUnaligned {reexecute=0 rethrow=0 return_oop=0}
> ; -
> java.lang.invoke.VarHandleByteArrayAsLongs$ArrayHandle::get@32 (line 115)
>... lots more
>
>
> but the llvm-based one stops here:
>
>
> 0x00010ed79034: stp wzr, wzr, [x20, #0x3c];*putfield
> preCounterBlock {reexecute=0 rethrow=0 return_oop=0}
> ; -
> com.sun.crypto.provider.GaloisCounterMode$GCMEngine::@80 (line 673)
> ; -
> com.sun.crypto.provider.GaloisCounterMode$GCMDecrypt::@8 (line 1346)
> ; -
> com.sun.crypto.provider.GaloisCounterMode::checkInit@60 (line 329)
> ;; B6: # out( B242 B7 ) <- in( B192 B5 ) Freq: 0.99
> 0x00010ed79038: cbnzx21, #0x10
> ;; null oop passed to encode_heap_oop_not_null2
> 0x00010ed7903c: dcps1 #0xdeae
> 0x00010ed79040:
>
> [/Disassembly]
>
>
>
> I think it's giving up as soon as it sees something it doesn't recognize, so
> it's pretty much useless.
>
> In addition, even when it does work the LLVM disassembly is pretty poor. For
> example, the unverified entry point looks like
>
>
> 0x00010ed78f80: ldr w8, [x1, #0x8]
> 0x00010ed78f84: cmp w9, w8
> 0x00010ed78f88: b.eq#0x10
> 0x00010ed78f8c: adrpx8, #-128524288 ; {runtime_call
> ic_miss_stub}
> 0x00010ed78f90: add x8, x8, #0x1c0
> 0x00010ed78f94: br x8
>
>
> instead of
>
>
> 0x000110566500: ldr w8, [x1, #8]
> 0x000110566504: cmp w9, w8
> 0x000110566508: b.eq0x000110566518 // b.none
> 0x00011056650c: adrpx8, 0x000108ae6000 ; {runtime_call
> ic_miss_stub}
> 0x000110566510: add x8, x8, #0x1c0
> 0x000110566514: br x8
>
>
> Sure, it's good to have a choice, but the LLVM-based hsdis doesn't look to me
> like a serious alternative for professional work.
@theRealAph Yes, the build instructions needs to be updated. I could have sworn
I did that, but maybe that fix is just lying around un-committed in one of my
repos on one of my test computers. I'll have to go around and check. Or write
it again if I can't fix it. That needs to go with this PR.
You can do `make install-hsdis` to av
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote:
> This patch expands the newly added system for hsdis backends to include LLVM.
>
> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry,
> as published in the never integrated PR
> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out
> the binutils-based part of it.)
>
> Unfortunately I have not been able to make this work properly on Windows.
> With some additional flags I made it compile without complaints, but it
> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I
> tried to load the library. This is somewhat ironic, since the initial
> implementation was created by Ludovic for the very purpose of using it on
> Windows.
>
> The lack of Windows support in this patch does not mean it is impossible to
> get it to work, just that I need to co-operate with someone who has more
> experience of compiling LLVM on Windows, and/or are more eager to get this
> combination to work.
So I figured out how to build it with `make build-hsdis`. The instructions need
to be fixed.
Two problems: firstly, the library seems to be built with the wrong name, so
the runtime doesn't find it. I had to use
`ln -sf ~/lib/libhsdis.dylib ~/lib/hsdis-aarch64.dylib`
to get it to work.
More severely, the disassembly is truncated, so lots of stuff doesn't work. The
binutils-based hsdis prints
0x0001105665b4: stp wzr, wzr, [x20, #60];*putfield
preCounterBlock {reexecute=0 rethrow=0 return_oop=0}
; -
com.sun.crypto.provider.GaloisCounterMode$GCMEngine::@80 (line 673)
; -
com.sun.crypto.provider.GaloisCounterMode$GCMDecrypt::@8 (line 1346)
; -
com.sun.crypto.provider.GaloisCounterMode::checkInit@60 (line 329)
;; B6: # out( B242 B7 ) <- in( B192 B5 ) Freq: 0.99
0x0001105665b8: cbnzx21, 0x0001105665c8
;; null oop passed to encode_heap_oop_not_null2
0x0001105665bc: dcps1 #0xdeae
0x0001105665c0: .inst 0x082adca6 ; undefined
0x0001105665c4: udf #1
0x0001105665c8: lsr x10, x21, #3;*invokevirtual
getLongUnaligned {reexecute=0 rethrow=0 return_oop=0}
; -
java.lang.invoke.VarHandleByteArrayAsLongs$ArrayHandle::get@32 (line 115)
... lots more
but the llvm-based one stops here:
0x00010ed79034: stp wzr, wzr, [x20, #0x3c];*putfield
preCounterBlock {reexecute=0 rethrow=0 return_oop=0}
; -
com.sun.crypto.provider.GaloisCounterMode$GCMEngine::@80 (line 673)
; -
com.sun.crypto.provider.GaloisCounterMode$GCMDecrypt::@8 (line 1346)
; -
com.sun.crypto.provider.GaloisCounterMode::checkInit@60 (line 329)
;; B6: # out( B242 B7 ) <- in( B192 B5 ) Freq: 0.99
0x00010ed79038: cbnzx21, #0x10
;; null oop passed to encode_heap_oop_not_null2
0x00010ed7903c: dcps1 #0xdeae
0x00010ed79040:
[/Disassembly]
I think it's giving up as soon as it sees something it doesn't recognize, so
it's pretty much useless.
In addition, even when it does work the LLVM disassembly is pretty poor. For
example, the unverified entry point looks like
0x00010ed78f80: ldr w8, [x1, #0x8]
0x00010ed78f84: cmp w9, w8
0x00010ed78f88: b.eq#0x10
0x00010ed78f8c: adrpx8, #-128524288 ; {runtime_call
ic_miss_stub}
0x00010ed78f90: add x8, x8, #0x1c0
0x00010ed78f94: br x8
instead of
0x000110566500: ldr w8, [x1, #8]
0x000110566504: cmp w9, w8
0x000110566508: b.eq0x000110566518 // b.none
0x00011056650c: adrpx8, 0x000108ae6000 ; {runtime_call
ic_miss_stub}
0x000110566510: add x8, x8, #0x1c0
0x000110566514: br x8
Sure, it's good to have a choice, but the LLVM-based hsdis doesn't look to me
like a serious alternative for professional work.
-
PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 13:55:04 GMT, Andrew Haley wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR #392. (I have basically just ripped >> out the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. > > What is it for, then? > > hsdis builds on AArch64-MacOS-LLVM with > > > cd src/utils/hsdis > mkdir build > cd build > git clone https://github.com/bminor/binutils-gdb > ln -s binutils-gdb binutils > cd .. > make > @theRealAph As you might be aware, the licensing criteria for binutils makes > it impossible to distribute a binutils-based hsdis with the JDK. While IANAL, > my understanding is that the LLVM license is less problematic in that way. > > Also, this is to allow a bit of freedom of choice. If you prefer the LLVM > backend (I've been told that it generates better disassembly in some case) > you should be able to select it. > > And finally, I do think that the LLVM backend should be able to work on > Windows, too. It's just that this is tricky enough to motivate doing this in > a separate, later, step. OK, but how do you build it? I have applied this patch, and the instructions in `hsdis/README` don't mention LLVM, just binutils. Shouldn't the instructions have been updated? - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Marked as reviewed by luhenry (Author). - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 13:04:45 GMT, Jorn Vernee wrote: > The only other issue I had is that `install-hsdis` only copies the library to > the exploded JDK, so I manually copy it to `images/jdk/bin/server` afterwards. @JornVernee Ah, good point! I need to make sure it gets copied to the image as well. And thank you for your patch, I'll have a look at it and see if I can incorporate it in this patch, or if it's better to make it a separate patch. It's worth noting that I used the llvm package from cygwin, neither the "official" package nor a self-built. I can imagine that the self-built works better, so it might be worth adding a --with-llvm-src option to help build it as well. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 13:55:04 GMT, Andrew Haley wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR #392. (I have basically just ripped >> out the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. > > What is it for, then? > > hsdis builds on AArch64-MacOS-LLVM with > > > cd src/utils/hsdis > mkdir build > cd build > git clone https://github.com/bminor/binutils-gdb > ln -s binutils-gdb binutils > cd .. > make @theRealAph As you might be aware, the licensing criteria for binutils makes it impossible to distribute a binutils-based hsdis with the JDK. While IANAL, my understanding is that the LLVM license is less problematic in that way. Also, this is to allow a bit of freedom of choice. If you prefer the LLVM backend (I've been told that it generates better disassembly in some case) you should be able to select it. And finally, I do think that the LLVM backend should be able to work on Windows, too. It's just that this is tricky enough to motivate doing this in a separate, later, step. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR #392. (I have basically just ripped > out the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. What is it for, then? hsdis builds on AArch64-MacOS-LLVM with cd src/utils/hsdis mkdir build cd build git clone https://github.com/bminor/binutils-gdb ln -s binutils-gdb binutils cd .. make - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. In my experience the output of llvm-config is also not usable. I think the output also depends on the toolchain you use to build llvm FWIW. The output of my locally built llvm-config does contain the MSVC flags, but the paths it points to are incorrect (all pointing to the build directory, instead of the package install location). I have a patch here that gets me a working hsdis based on the llvm package I built manually using MSVC (the [official package](https://github.com/llvm/llvm-project/releases/download/llvmorg-13.0.0/LLVM-13.0.0-win64.exe) doesn't seem to contain the needed header files): https://github.com/openjdk/jdk/compare/pr/5920...JornVernee:hsdis_llvm_windows (The only issue currently is that the code I used to filter out the incorrect `-I` flags from what llvm-config gives me doesn't seem to work, though the build still passes). I built llvm using something like this (according to my notes): git clone https://github.com/llvm/llvm-project.git cd llvm-project mkdir build_llvm cd build_llvm cmake ../llvm -D"LLVM_TARGETS_TO_BUILD:STRING=X86" -D"CMAKE_BUILD_TYPE:STRING=Release" -D"CMAKE_INSTALL_PREFIX=install_local" -A x64 -T host=x64 cmake --build . --config Release --target install This then uses MSVC to build me an llvm 'package' in `build_llvm/install_local`, which I then point to using `--with-llvm`. The only other issue I had is that `install-hsdis` only copies the library to the exploded JDK, so I manually copy it to `images/jdk/bin/server` afterwards. HTH - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:32:03 GMT, Jorn Vernee wrote: >> This patch expands the newly added system for hsdis backends to include LLVM. >> >> The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, >> as published in the never integrated PR >> https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out >> the binutils-based part of it.) >> >> Unfortunately I have not been able to make this work properly on Windows. >> With some additional flags I made it compile without complaints, but it >> caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I >> tried to load the library. This is somewhat ironic, since the initial >> implementation was created by Ludovic for the very purpose of using it on >> Windows. >> >> The lack of Windows support in this patch does not mean it is impossible to >> get it to work, just that I need to co-operate with someone who has more >> experience of compiling LLVM on Windows, and/or are more eager to get this >> combination to work. > >> but it caused hotspot to segfault in LoadLibrary (!) in os::dll_load when I >> tried to load the library. > > I tried compiling the binutils-based hsdis earlier as well, but on WSL > instead of cygwin (using the `mingw-w64` package), and ran into the same > issue. It kept segfaulting when loading the library. > > My guess was that it is a problem caused by mixing libraries that are > compiled with different toolchains, as the JDK itself is compiled with MSVC. > > AFAIK binutils can only be built with mingw (based on my earlier > experiments), but LLVM can be built with MSVC as well, so maybe the regular > MSVC toolchain could be used to build the llvm-based hsdis. @JornVernee It is likely that the LLVM-based backend can be build by MSVC, yes. I did not explore that further in this patch. I suggest that the way forward is to get this patch into mainline, and then experiment with how to get Windows support working properly. (The main problem with the MSVS approach is that the LLVM libraries, as returned by llvm-config, is in gcc format (`-lLLFoobarnicator`) which we can't send to CL. (It seems Ludovic tried to work around this by transforming the command line in his original PR.) - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. Very happy to see it landing :) Thank you! I don't have access to a windows machine, and even less a Windows-AArch64 machine. @lewurm would you be able to take a look? - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > but it caused hotspot to segfault in LoadLibrary (!) in os::dll_load when I > tried to load the library. I tried compiling the binutils-based hsdis earlier as well, but on WSL instead of cygwin (using the `mingw-w64` package), and ran into the same issue. It kept segfaulting when loading the library. My guess was that it is a problem caused by mixing libraries that are compiled with different toolchains, as the JDK itself is compiled with MSVC. AFAIK binutils can only be built with mingw (based on my earlier experiments), but LLVM can be built with MSVC as well, so maybe the regular MSVC toolchain could be used to build the llvm-based hsdis. - PR: https://git.openjdk.java.net/jdk/pull/5920
RFR: 8253757: Add LLVM-based backend for hsdis
This patch expands the newly added system for hsdis backends to include LLVM. The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, as published in the never integrated PR https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out the binutils-based part of it.) Unfortunately I have not been able to make this work properly on Windows. With some additional flags I made it compile without complaints, but it caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I tried to load the library. This is somewhat ironic, since the initial implementation was created by Ludovic for the very purpose of using it on Windows. The lack of Windows support in this patch does not mean it is impossible to get it to work, just that I need to co-operate with someone who has more experience of compiling LLVM on Windows, and/or are more eager to get this combination to work. - Commit messages: - Create hsdis backend using LLVM Changes: https://git.openjdk.java.net/jdk/pull/5920/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5920&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253757 Stats: 406 lines in 6 files changed: 398 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5920.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920 PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Mon, 21 Dec 2020 16:03:31 GMT, Magnus Ihse Bursie wrote: >> 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. > > 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. @magicus that PR fell off my plate, but I'm happy to pick it up based on your "selectable hsdis backend" change. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Mon, 23 Nov 2020 15:55:43 GMT, Magnus Ihse Bursie wrote: >> 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. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Mon, 26 Oct 2020 11:41:16 GMT, Magnus Ihse Bursie wrote: >> 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. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Mon, 26 Oct 2020 11:37:52 GMT, Magnus Ihse Bursie wrote: >> 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/ > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Mon, 26 Oct 2020 11:16:28 GMT, Magnus Ihse Bursie wrote: >>> @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`. > > 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/ 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. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 20:40:50 GMT, Xin Liu wrote: >> @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`. > >> @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`. 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/ - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 18:15:10 GMT, Ludovic Henry wrote: > @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`. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 18:07:59 GMT, Ludovic Henry wrote: >>> 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`. > > @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. @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`. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 12:30:13 GMT, Bernhard Urban-Forster wrote: >> 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 > >> 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`. @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. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 7 Oct 2020 08:02:59 GMT, Xin Liu wrote: >> 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 > > 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 > 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`. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 7 Oct 2020 00:48:24 GMT, Yasumasa Suenaga wrote: >> 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. > > 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 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 - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Fri, 2 Oct 2020 11:44:51 GMT, Magnus Ihse Bursie wrote: >> @navyxliu I've merged the sources into `src/utils/hsdis` and added support >> to build it in the Makefile. > > 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. 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 - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 30 Sep 2020 00:55:23 GMT, Ludovic Henry wrote: >> 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. > > @navyxliu I've merged the sources into `src/utils/hsdis` and added support to > build it in the Makefile. 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. - PR: https://git.openjdk.java.net/jdk/pull/392
