[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-swing-dev
Subject:    Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
From:       Julian Waters <jwaters () openjdk ! org>
Date:       2024-05-16 7:59:07
Message-ID: 4QTrXORVLmXOdBxVAdYyAE9M7HTfxOjcW0NVtitY-og=.aef997d7-96d6-4992-b308-d951046c920f () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]