On Thu, 8 Oct 2020 18:07:59 GMT, Ludovic Henry <[email protected]> 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
