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

[Robbin Ehn]

On Fri, 22 Sep 2023 09:20:47 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>>> 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...

I forgot about --with-binutils=system:
This one is broken:
/home/rehn/source/jdk/open/src/utils/hsdis/binutils/hsdis-binutils.c:60:10: fatal error: libiberty.h: No such file or directory
   60 | #include <libiberty.h>

This include is under "libiberty/libiberty.h".

And yes, if we fix that bfdver.h would be an issue, you are correct.

Maybe we should just test what API version in make system by testing the init_disassemble_info_from_bfd signature.
Thus removing bfdver.h.

For a system build we would have deps on libzstd.so, if it was not compiled without it.

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

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