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

[Magnus Ihse Bursie]

On Mon, 7 Aug 2023 10:57:57 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi please consider.
>> 
>> This works with 2.30, 2.34, 2.38, 2.39, 2.40, 2.41 and current master head. (tested x64 and some RV)
>> 
>> There are 4 changes in binutils we work around.
>> - zstd compressed debug sections
>> - libsframe added
>> - init_disassemble_info() change
>> - libbfd.a is only present in .lib directory in newer binutils builds (older it is in both directories) (I think the issue is that we never do make install, thus have dependency on internal artifact placement)
>> 
>> Specific to RV, there is a bug in binutils causing the standard extensions not being added to disassembler if we pass in NULL.
>> 
>> This no way near perfect, but at least we can build hsdis with any contemporary binutils.
>> 
>> Todo better I think we need to build and install binutils to check the version and then use that version to figure out what options to use when re-building and re-installing binutils for hsdis.
>> 
>> I asked tool-chain people about our issues, they said, you can't do that. I.e. have source dependencies on many binutils versions.
>> 
>> As RV is new and have new instructions added to it frequently we really need to be able to build with bleeding-edge binutils. (capstone RV is not actively worked on, llvm have many more dependencies)
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reverted bad change

This discussion branched away a bit. Let me try to summarize:

About hsdis backends:
* Capstone is easier for us to interact with, and has no license issues blocking a distributable hsdis. In that way it is definitely preferable, just as @theRealAph says. Ideally, it should be our first hand choice of hsdis backend.
* However, it is also the case that the backend must be useful for the developers. Robin's point is that for RISC-V, only binutils is really usable as of now, not Capstone. Hence we must keep our binutils support alive.

About this patch:
* Our support for `--with-binutils=system` is currently broken. 
* Currently, the `--with-binutils=` argument works well if pointed to a binutils build location, but not if pointed to an installed location. 
* The above issues is relatively easy to fix as the code looks right now, but if this PR is accepted, it will be much harder (or impossible) to fix, thus cementing that only a binutils build location will be acceptable.
* If there were some other way to determine the version of BFD used for building binutils, then we could have this patch, as well as being able to support system binutils and/or a installed location.

Correct me if I'm wrong.

The upshot of all this is that I am in favor of keeping binutils support alive, but I am a bit hesitant about locking in on the need for a installation directory. Instead, I'd rather see that we try to fix the current bugs with `system` and an installation directory. 

It would be really great if we could find a way to deduce the bfd version. If this is not possible, a less ideal solution could be to e.g. assume a certain bfd version if using a system or installed location binutils, and/or possibly allow the user to set this value from configure options. (And if we do have a binutils build location, we can of course do as in this patch and get it from bfdver.h -- but maybe extract the value in configure instead).

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

PR Comment: https://git.openjdk.org/jdk/pull/15138#issuecomment-1739604044