RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]

[Julian Waters]

On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:

> Hi Julian, sorry for not getting back to you.
> 
> The problem from my PoV is that this is a very large and intrusive patch in the build of the actual product, for a claimed problem in the tangential hsdis library. I have not understood a pressing business need to get a Windows/gcc port for hsdis, which means I can't really prioritize trying to understand this patch.
> 
> As you know, the build system does not currently really separate between "the OS is Windows" and "the toolchain is Microsoft". I understand that you want to fix that, and in theory I support it. However, you must also realize that making a complete fix of this requires a lot of work, for something that there is no clearly defined need. (After all, cl.exe works fine to create the build, has no apparent issues, and is as far as an "official" compiler for Windows as you can get.) That makes it fall squarely in the WIBNIs bucked ("wouldn't it be nice if...").
> 
> If you can fix just the hsdis build without changing anything outside the hsdis Makefiles, that would be a different story. Such a change would be limited in scope, easy to say it will not affect the product proper, and be easier to review.

Oh, no worries. Sorry for sounding a little impatient.

The problem is that there are areas in the build system that will need changing to support hsdis compilation, not just the hsdis Makefile and autoconf file itself. I can try to work on minimizing the amount of changes to non-hsdis files (I was hoping the current changes were small enough, but it seems they are not), but I believe it's impossible to achieve this by only touching the hsdis Makefile and lib-hsdis.m4. That is, there will necessarily be changes, no matter how minimal, to non-hsdis files.

As for why do this at all, I was mainly driven by seeing the hack in place for the binutils backend on Windows. It only supports Cygwin, of which the gcc installation is subpar compared to the newer gcc provided by some MSYS2 subsystems (MINGW64 gcc is primarily battle tested on MSYS2, on Cygwin it doesn't get as much attention and misses out further fixes and enhancements from the MSYS2 team, for instance it even links to the obsolete msvcrt library), and the hack itself has several flaws that don't seem apparent at first (For instance, -lpthread will link to libwinpthread.dll and result in an invisible dependency on that dll depending on the Windows platform, which will cause loading hsdis to silently fail depending on how it's loaded for seemingly random reasons!). I wanted to replace that (or I guess provide a better alternative) by adding semi proper support to the hsdis build for the more modern and better battle tested gcc that some MSYS2 subsystems provide, to streamline comp
 iling the binutils hsdis on Windows. So this is mainly about hsdis-binutils on Windows. hsdis-capstone I also decided to support because it's small and relatively easy to pile on top of the existing work, and MSYS2 also provides Capstone as well. The same unfortunately could not be said for hsdis-llvm, which was significantly more complex than Capstone is

I'm not sure where to go from here. I could support gcc for hsdis binutils by extending the hack that exists for Cygwin, but I _really_ don't want to do that. I could enhance the build system to semi properly support gcc for hsdis only, as I've done, but the changes will necessarily touch non hsdis files as well, no matter how minimal they are. I'll return this to draft for the time being I suppose, I'd be interested in hearing which way forward you prefer though

But while I work on making this change even smaller and easier to review, could I ask the above questions again? (Well, except for the FIXPATH one, you can ignore that)



> @magicus I think I might need some help here. Currently all the Cygwin stuff is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be behind isBuildOsEnv cygwin checks instead? I don't know how to add support for Cygwin gcc for most of hsdis, since it is quite different from the more modern gcc distributions that are found in places like MSYS2 and MINGW64. But most of the existing logic seems to accomodate more for the Microsoft compiler than it is concerned about the OS environment being used, and for this reason I can't tell which of the 2 checks to use for the existing hack that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but Microsoft does, but I don't want to check for microsoft inside TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get FIXPATH while Microsoft does?



> Also @magicus, what is the typical path passed to --with-binutils like on Windows? Something like --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work correctly, since the include path to dis-asm.h would then become `#include "/c/Users/vertig0/Downloads/binutils-2.42/include/dis-asm.h"` Which causes a configure check to fail on the compile stage since gcc cannot recognise the MINGW-esque /c/ as a drive, and then causes configure to erroneously report binutils as using the Old API when it's in fact using the New API. --with-binutils=C:/Users/vertig0/Downloads/binutils-2.42 on the other hand works as expected. Should there be a fixup for the path there so gcc can recognise it properly?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2114331612