RFR: 8295795: hsdis does not build with binutils 2.39+ [v3]

[Robbin Ehn]

On Fri, 22 Sep 2023 08:02:16 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:

>> I'm still not really happy about this. The old solution without .libs has worked before -- has anything changed in newer versions of binutils? 
>> 
>> Also, we support building binutils in place as a convenience, but it should also be possible to just set a hsdis path, and in that case we cannot presume that the .libs layout is kept.
>> 
>> I suggest you change this to both eat the cake and have it -- first check for the lib in the original location (without .libs), and if it is not found there, check in .libs. This is perhaps not necessary here in LIB_BUILD_BINUTILS, but it definitely is in LIB_SETUP_HSDIS_BINUTILS. It will make the code a few lines longer but more robust.
>
>> It may be possible to read some elf header about binutils version. I.e. do make install from our build system, then reading out version from elf section and pass that into our build. As we need to know the signature of init_disassemble_info() which is different between binutils versions.
> 
> Trying to understand what you are really saying here. If we accept this patch, it will no longer be possible to use a normally installed binutils, since the BFD_VERSION constant (via bfdver.h) is not available? 
> 
> That is not good; the idea of building binutils locally was supposed to be a convenience, not a requirement.

Both options --with-binutils= and --with-binutils-src= grab atrifact from temporary internal build layout.
If you do "make install" and point --with-binutils= to that directory the layout is diffrent and you get:
--with-binutils=/home/rehn/binutils-2.39 gives:
configure: error: /home/rehn/binutils-2.39 does not contain a proper binutils installation

We can't use my distro native or a installed binutils.

> I'm still not really happy about this. The old solution without .libs has worked before -- has anything changed in newer versions of binutils?

As the PR says: "libbfd.a is only present in .lib directory in newer binutils builds (older it is in both directories)"
We are depending on their build system artifact layout, this is not a stable interface.
You need todo make install to get the stable layout, then it would be "./lib/libsframe.a".

> 
> Also, we support building binutils in place as a convenience, but it should also be possible to just set a hsdis path, and in that case we cannot presume that the .libs layout is kept.

binutils do not support having source dependency on multiple versions (i asked about this).
That is why the binutils tools are co-hosted (gdb, gas, ...)
Which I assume is on of the reasons it is not possible form headers to check version.
But as I said we today do not support using the installation since we have wrong path for libs.

> 
> I suggest you change this to both eat the cake and have it -- first check for the lib in the original location (without .libs), and if it is not found there, check in .libs. This is perhaps not necessary here in LIB_BUILD_BINUTILS, but it definitely is in LIB_SETUP_HSDIS_BINUTILS. It will make the code a few lines longer but more robust.

As I said it have been under .lib in all version of binutils I checked. This is not a new location.
So it seem like it's much better to check the directory it is always in (so far), no?

> > It may be possible to read some elf header about binutils version. I.e. do make install from our build system, then reading out version from elf section and pass that into our build. As we need to know the signature of init_disassemble_info() which is different between binutils versions.
> 
> Trying to understand what you are really saying here. If we accept this patch, it will no longer be possible to use a normally installed binutils, since the BFD_VERSION constant (via bfdver.h) is not available?
> 
> That is not good; the idea of building binutils locally was supposed to be a convenience, not a requirement.

"--with-binutils" don't use the "normally installed binutils" it assume that the directory is the build directory, which as have a different layout than "make install". So that case do not work today.

Summarize, the layout we are using for both hsdis= and hsdis-src= are the internal temporary layout before you do make install. Therefore we have access to bfdver.h and can determine API version.

This do not handle the case if you built with dependency on zstd.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15138#discussion_r1334127071