8253757: Add LLVM-based backend for hsdis #5920
Conversation
|
👋 Welcome back ihse! A progress list of the required criteria for merging this PR into |
|
/contributor add ihse |
|
@magicus |
|
@magicus |
openjdk
bot
added
build
Webrevs
|
I tried compiling the binutils-based hsdis earlier as well, but on WSL instead of cygwin (using the 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. |
|
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? |
|
@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 ( |
|
@magicus This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 10 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
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 doesn't seem to contain the needed header files): pr/5920...JornVernee:hsdis_llvm_windows (The only issue currently is that the code I used to filter out the incorrect I built llvm using something like this (according to my notes): This then uses MSVC to build me an llvm 'package' in The only other issue I had is that As you say, Windows support can be added later, so this doesn't need to be added to the current patch IMO. But, I might not have time to get back to this myself, so just leaving the steps I used that worked for me in case anyone else wants to pick it up as well. |
What is it for, then? hsdis builds on AArch64-MacOS-LLVM with |
|
@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. |
@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. |
OK, but how do you build it? I have applied this patch, and the instructions in |
|
So I figured out how to build it with Two problems: firstly, the library seems to be built with the wrong name, so the runtime doesn't find it. I had to use More severely, the disassembly is truncated, so lots of stuff doesn't work. The binutils-based hsdis prints but the llvm-based one stops here: 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 instead of 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 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. |
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 on Windows. |
|
The value add of this LLVM-based hsdis is two-fold:
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 |
|
Mailing list message from monica beckwith on hotspot-compiler-dev: Hi all, I am on vacation right now, but we can take a look next week and see how we Monica Sent from my Arm powered smart device. On Thu, Oct 14, 2021, 9:07 AM Andrew Haley <aph at openjdk.java.net> wrote: |
I don't think we should commit code that we know is broken. I don't believe that this view might be controversial. 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? |
Sorry, thinko. You don't need the .hotspot_compiler file |
|
The problem is 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);
} |
|
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. |
|
@magicus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Yeah bot, I'm still working on it. |
|
@magicus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@magicus @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. |
Turns out I just needed to add /opt/homebrew/opt/llvm/bin to my PATH. I can now use --with-hsdis=llvm. |
|
@magicus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Yes bot, thank you for the reminder. |
|
@magicus this pull request can not be integrated into git checkout hsdis-backend-llvm
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
merge-conflict
openjdk
bot
added
ready
|
/contributor add @nick-arm |
|
@magicus |
|
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. |
7 participants
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. With some additional flags I made it compile without complaints, but it caused hotspot to segfault in
LoadLibrary(!) inos::dll_loadwhen 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.
Progress
Issue
Reviewers
Contributors
<ihse@openjdk.org><luhenry@openjdk.org><ngasson@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5920/head:pull/5920$ git checkout pull/5920Update a local copy of the PR:
$ git checkout pull/5920$ git pull https://git.openjdk.java.net/jdk pull/5920/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5920View PR using the GUI difftool:
$ git pr show -t 5920Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5920.diff