mbox series

[v1,0/4,RFC] Provide a target-specific definition to enable/disable section of code in others modules than gas

Message ID 20241211120553.1391850-1-matthieu.longo@arm.com
Headers
Series Provide a target-specific definition to enable/disable section of code in others modules than gas |

Message

Matthieu Longo Dec. 11, 2024, 12:05 p.m. UTC 
  **Disclaimer:** This patch series is related to a comment from another patch series (see [1]), and tries to propose a solution for the raised issue. Hence the code changes in patch 3/4 are restricted to architecture-specific code for DWARF and CFI directives.

DWARF 5 standard provides a space for vendor-specific DWARF directives. Binutils currently defines GNU-specific DWARF directives and vendor-specific ones for AArch64, SGI/MIPS, and Sparc architectures.
The architecture specific code is not guarded. This has caused confusion in the past, and .cfi_window_save (a Sparc-specific directive) was used on AArch64 in place of .cfi_negate_ra_state recommended by the document for DWARF extensions on AArch64 (see [2]). In [1], Jan Beulich rightly pointed out the issue and proposed to introduce an `#ifdef TC_AARCH64` around the AArch64 directives.
However, the current config header file generated by autotools does not provide everywhere a "switch mechanism" to enable/disable those extensions depending on the selected target similar to the one implemented in the gas module via `TC_<arch>`.

* Patch 1/4 adds the boilerplate to add the `TC_<arch>` definition to all modules of binutils interacting with architecture-specific DWARF
directives, i.e. bfd, binutils, gprofng, libbacktrace, libiberty.
Note: I added `CI-tag: skip` to the description of the commit so that the CIs or bisecting tools can skip it as it requires the autotools files to be regenerated.

* Patch 2/4 regenerates the build files with autoreconf for the previous commit.

* Patch 3/4 uses `TC_<arch>` and enables architecture-specific CFI directives and DWARF instructions only when they are required by the target.
  
* **Patch 4/4 should not be committed**. Its purpose is to explain how this patch series was tested on aarch64-none-linux-gnu. No regression were found.

**Note:**

I am not sure if this approach is correct as it duplicates the declaration of the `TC_<arch>` definition in several modules. I would advocate for centralizing this declaration at the root of the project since the target is not supposed to change from one module to another. However, it would clash with the definition of TC_AARCH64 in tc-aarch64.h

Additionally, among the modules impacted by this patch series, two of them, libiberty and libbacktrace, have their master version inside GCC's repository. Before committing any change inside binutils, the changes in libiberty and libbacktrace would need to be approved by GCC's maintainers.

Please let me know what you think about it.

Regards,
Matthieu.
  
[1]: https://inbox.sourceware.org/binutils/eaedfc6c-5666-4a05-a676-d25bf52b76c1@suse.com/
[2]: https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#id1

Matthieu Longo (4):
  Add target compile definitions in libraries interacting with DWARF arch-extensions
  Regenerate build files with autoreconf for previous commit
  Enable arch-specific CFI directives and DWARF instructions only when required by the target
  [DON'T COMMIT] error if TC_AARCH64 is not defined on AArch64 target

 bfd/Makefile.am            |  3 ++-
 bfd/Makefile.in            |  3 ++-
 bfd/config.in              |  9 ++++++++
 bfd/configure              | 15 ++++++++++++++
 bfd/configure.ac           |  9 ++++++++
 bfd/configure.tgt          | 42 ++++++++++++++++++++++++++++++++++++++
 bfd/elf-eh-frame.c         | 11 +++++++++-
 binutils/config.in         |  9 ++++++++
 binutils/configure         | 18 ++++++++++++++--
 binutils/configure.ac      | 12 +++++++++--
 binutils/configure.tgt     | 21 +++++++++++++++++--
 binutils/dwarf.c           | 23 ++++++++++++++-------
 gas/dw2gencfi.c            | 33 ++++++++++++++++++++++++++----
 gas/gen-sframe.c           | 34 ++++++++++++++++++------------
 gprofng/Makefile.am        |  4 +++-
 gprofng/Makefile.in        |  5 ++++-
 gprofng/common/config.h.in |  9 ++++++++
 gprofng/configure          | 15 ++++++++++++++
 gprofng/configure.ac       |  9 ++++++++
 gprofng/configure.tgt      | 42 ++++++++++++++++++++++++++++++++++++++
 gprofng/doc/version.texi   |  4 ++--
 include/dwarf2.def         | 27 ++++++++++++++++++------
 include/dwarf2.h           |  1 +
 libbacktrace/Makefile.am   |  3 +++
 libbacktrace/Makefile.in   |  3 +++
 libbacktrace/config.h.in   |  9 ++++++++
 libbacktrace/configure     | 16 +++++++++++++++
 libbacktrace/configure.ac  | 10 +++++++++
 libbacktrace/configure.tgt | 42 ++++++++++++++++++++++++++++++++++++++
 libiberty/Makefile.in      |  3 +++
 libiberty/config.in        |  9 ++++++++
 libiberty/configure        | 16 +++++++++++++++
 libiberty/configure.ac     | 10 +++++++++
 libiberty/configure.tgt    | 42 ++++++++++++++++++++++++++++++++++++++
 34 files changed, 478 insertions(+), 43 deletions(-)
 create mode 100644 bfd/configure.tgt
 create mode 100644 gprofng/configure.tgt
 create mode 100644 libbacktrace/configure.tgt
 create mode 100644 libiberty/configure.tgt

Comments

Alan Modra Dec. 11, 2024, 10:02 p.m. UTC | #1 
On Wed, Dec 11, 2024 at 12:05:49PM +0000, Matthieu Longo wrote:
> I am not sure if this approach is correct as it duplicates the
> declaration of the `TC_<arch>` definition in several modules.

It can't be correct in any file like bfd/elf-eh-frame.c or
binutils/dwarf.c that is compiled once but needs to support multiple
architectures.  Instead you should handle architecture specific dwarf
code at runtime using bfd_get_arch (and if necessary, bfd_get_mach).
See for example dwarf.c:init_dwarf_regnames_by_bfd_arch_and_mach or
the switch in elf-eh-frame.c:_bfd_elf_write_section_eh_frame on
abfd->arch_info->arch.
Jan Beulich Dec. 12, 2024, 10:25 a.m. UTC | #2 
On 11.12.2024 13:05, Matthieu Longo wrote:
> **Disclaimer:** This patch series is related to a comment from another patch series (see [1]), and tries to propose a solution for the raised issue. Hence the code changes in patch 3/4 are restricted to architecture-specific code for DWARF and CFI directives.
> 
> DWARF 5 standard provides a space for vendor-specific DWARF directives. Binutils currently defines GNU-specific DWARF directives and vendor-specific ones for AArch64, SGI/MIPS, and Sparc architectures.
> The architecture specific code is not guarded. This has caused confusion in the past, and .cfi_window_save (a Sparc-specific directive) was used on AArch64 in place of .cfi_negate_ra_state recommended by the document for DWARF extensions on AArch64 (see [2]). In [1], Jan Beulich rightly pointed out the issue and proposed to introduce an `#ifdef TC_AARCH64` around the AArch64 directives.
> However, the current config header file generated by autotools does not provide everywhere a "switch mechanism" to enable/disable those extensions depending on the selected target similar to the one implemented in the gas module via `TC_<arch>`.
> 
> * Patch 1/4 adds the boilerplate to add the `TC_<arch>` definition to all modules of binutils interacting with architecture-specific DWARF
> directives, i.e. bfd, binutils, gprofng, libbacktrace, libiberty.
> Note: I added `CI-tag: skip` to the description of the commit so that the CIs or bisecting tools can skip it as it requires the autotools files to be regenerated.

As Alan has already pointed out, what is doable in gas cannot easily be extended
in the same shape to libraries potentially serving many targets. My suggestion
was never meant to go beyond gas.

Jan