On Fri, 22 Sep 2023 09:20:47 GMT, Robbin Ehn <r...@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
