Details
P4 Description
I've just left a comment on a PR and the "author" of the comment is wrong in the "On .., XXX wrote:" headline. In the following email, it should say "On Thu, 8 Oct 2020 12:30:13 GMT, Ludovic Henry <luhenry@openjdk.org> wrote:". The comment itself can be found at https://github.com/openjdk/jdk/pull/392#issuecomment-705736041.
```
From: hotspot-dev <hotspot-dev-retn@openjdk.java.net> On Behalf Of Ludovic Henry
Sent: Thursday, October 8, 2020 11:10 AM
To: build-dev@openjdk.java.net; hotspot-dev@openjdk.java.net; hotspot-compiler-dev@openjdk.java.net
Subject: Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 12:30:13 GMT, Bernhard Urban-Forster <burban@openjdk.org> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsourceware.org%2Fgit%2F%3Fp%3Dbinutils-gdb.git%3Ba%3Dblob%3Bf%3Dbinutils%2Fdlltool.c%3Bh%3Ded016b97dc38cdb1b85d2f6df676b9c9750f0d41%3Bhb%3DHEAD%23l248&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619338275%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CFw%2FKlZOpereGWdS8tcdjzwj0Qml5WeTFfl%2Bvng%2Fy9E%3D&reserved=0
>
> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8188073&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619338275%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=97l%2B2uPWlyHu2wA1G69o4k6%2B2EgwMFUiY%2BII6PD7caQ%3D&reserved=0, 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8208495&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619338275%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=EVnHzpyIpDDP8VeBqayvlqDqfJlkVRBYcG0HpHLEhLk%3D&reserved=0. 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.openjdk.java.net%2Fjdk%2Fpull%2F392&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619348231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eFzOrb6sf4N%2FPkmfxAc90JXZJa1XXbppVZBVy5tHpzQ%3D&reserved=0
```
Thank you for all the hard work! :)
```
From: hotspot-dev <hotspot-dev-retn@openjdk.java.net> On Behalf Of Ludovic Henry
Sent: Thursday, October 8, 2020 11:10 AM
To: build-dev@openjdk.java.net; hotspot-dev@openjdk.java.net; hotspot-compiler-dev@openjdk.java.net
Subject: Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 12:30:13 GMT, Bernhard Urban-Forster <burban@openjdk.org> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsourceware.org%2Fgit%2F%3Fp%3Dbinutils-gdb.git%3Ba%3Dblob%3Bf%3Dbinutils%2Fdlltool.c%3Bh%3Ded016b97dc38cdb1b85d2f6df676b9c9750f0d41%3Bhb%3DHEAD%23l248&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619338275%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CFw%2FKlZOpereGWdS8tcdjzwj0Qml5WeTFfl%2Bvng%2Fy9E%3D&reserved=0
>
> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8188073&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619338275%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=97l%2B2uPWlyHu2wA1G69o4k6%2B2EgwMFUiY%2BII6PD7caQ%3D&reserved=0, 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8208495&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619338275%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=EVnHzpyIpDDP8VeBqayvlqDqfJlkVRBYcG0HpHLEhLk%3D&reserved=0. 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.openjdk.java.net%2Fjdk%2Fpull%2F392&data=04%7C01%7Cluhenry%40microsoft.com%7C1642ec4bcd804cd7917608d86bb5832c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637377774619348231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eFzOrb6sf4N%2FPkmfxAc90JXZJa1XXbppVZBVy5tHpzQ%3D&reserved=0
```
Thank you for all the hard work! :)
