On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie <i...@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
