On Thu, 8 Oct 2020 12:30:13 GMT, Bernhard Urban-Forster <bur...@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://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
