[v1,2/3] libgcc: hide CIE and FDE data for DWARF architecture extensions behind a handler.

  This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
architecture-specific structure containing CIE and FDE data related
to DWARF architecture extensions.

Hiding the architecture-specific attributes behind a handler has the
following benefits:
1. isolating those data from the generic ones in _Unwind_FrameState
2. avoiding casts to custom types.
3. preserving typing information when debugging with GDB, and so
   facilitating their printing.

This approach required to add a new header md-unwind-def.h included at
the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
architecture header via a symbolic link.

An obvious drawback is the increase in complexity with macros, and
headers. It also caused a split of architecture definitions between
md-unwind-def.h (types definitions used in unwind-dw2.h) and
md-unwind.h (local types definitions and handlers implementations).
The naming of md-unwind.h with .h extension is a bit misleading as
the file is only included in the middle of unwind-dw2.c. Changing
this naming would require modification of others backends, which I
prefered to abstain from. Overall the benefits are worth the added
complexity from my perspective.

libgcc/ChangeLog:

    * Makefile.in: New target for symbolic link to md-unwind-def.h
    * config.host: New parameter md_unwind_def_header. Set it to
    aarch64/aarch64-unwind-def.h for AArch64 targets, or no-unwind.h
    by default.
    * config/aarch64/aarch64-unwind.h
    (aarch64_pointer_auth_key): Move to aarch64-unwind-def.h
    (aarch64_cie_aug_handler): Update.
    (aarch64_arch_extension_frame_init): Update.
    (aarch64_demangle_return_addr): Update.
    * configure.ac: New substitute variable md_unwind_def_header.
    * unwind-dw2.h (defined): MD_ARCH_FRAME_STATE_T.
    * config/aarch64/aarch64-unwind-def.h: New file.
---
 libgcc/Makefile.in                         |  6 +++-
 libgcc/config.host                         | 13 +++++--
 libgcc/config/aarch64/aarch64-unwind-def.h | 41 ++++++++++++++++++++++
 libgcc/config/aarch64/aarch64-unwind.h     | 14 +++-----
 libgcc/configure.ac                        |  1 +
 libgcc/unwind-dw2.h                        |  6 ++--
 6 files changed, 67 insertions(+), 14 deletions(-)
 create mode 100644 libgcc/config/aarch64/aarch64-unwind-def.h
  

Matthieu Longo <matthieu.longo@arm.com> writes:
> This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
> architecture-specific structure containing CIE and FDE data related
> to DWARF architecture extensions.
>
> Hiding the architecture-specific attributes behind a handler has the
> following benefits:
> 1. isolating those data from the generic ones in _Unwind_FrameState
> 2. avoiding casts to custom types.
> 3. preserving typing information when debugging with GDB, and so
>    facilitating their printing.
>
> This approach required to add a new header md-unwind-def.h included at
> the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
> architecture header via a symbolic link.
>
> An obvious drawback is the increase in complexity with macros, and
> headers. It also caused a split of architecture definitions between
> md-unwind-def.h (types definitions used in unwind-dw2.h) and
> md-unwind.h (local types definitions and handlers implementations).
> The naming of md-unwind.h with .h extension is a bit misleading as
> the file is only included in the middle of unwind-dw2.c. Changing
> this naming would require modification of others backends, which I
> prefered to abstain from. Overall the benefits are worth the added
> complexity from my perspective.

Sorry, I should have read 2/3 before making the suggestion in the
previous review.  I agree that it makes sense to separate this change
out, given that it involves a new header file.

It'd be good to update the comment in no-unwind.h:

/* Dummy header for targets without a definition of
   MD_FALLBACK_FRAME_STATE_FOR.  */

LGTM otherwise, thanks.

On patch 3: IMO it's better to post the regenerated files as part
of the same patch, so that each patch is self-contained.

Richard

> libgcc/ChangeLog:
>
>     * Makefile.in: New target for symbolic link to md-unwind-def.h
>     * config.host: New parameter md_unwind_def_header. Set it to
>     aarch64/aarch64-unwind-def.h for AArch64 targets, or no-unwind.h
>     by default.
>     * config/aarch64/aarch64-unwind.h
>     (aarch64_pointer_auth_key): Move to aarch64-unwind-def.h
>     (aarch64_cie_aug_handler): Update.
>     (aarch64_arch_extension_frame_init): Update.
>     (aarch64_demangle_return_addr): Update.
>     * configure.ac: New substitute variable md_unwind_def_header.
>     * unwind-dw2.h (defined): MD_ARCH_FRAME_STATE_T.
>     * config/aarch64/aarch64-unwind-def.h: New file.
> ---
>  libgcc/Makefile.in                         |  6 +++-
>  libgcc/config.host                         | 13 +++++--
>  libgcc/config/aarch64/aarch64-unwind-def.h | 41 ++++++++++++++++++++++
>  libgcc/config/aarch64/aarch64-unwind.h     | 14 +++-----
>  libgcc/configure.ac                        |  1 +
>  libgcc/unwind-dw2.h                        |  6 ++--
>  6 files changed, 67 insertions(+), 14 deletions(-)
>  create mode 100644 libgcc/config/aarch64/aarch64-unwind-def.h
>
> diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
> index 0e46e9ef768..ffc45f21267 100644
> --- a/libgcc/Makefile.in
> +++ b/libgcc/Makefile.in
> @@ -47,6 +47,7 @@ with_aix_soname = @with_aix_soname@
>  solaris_ld_v2_maps = @solaris_ld_v2_maps@
>  enable_execute_stack = @enable_execute_stack@
>  unwind_header = @unwind_header@
> +md_unwind_def_header = @md_unwind_def_header@
>  md_unwind_header = @md_unwind_header@
>  sfp_machine_header = @sfp_machine_header@
>  thread_header = @thread_header@
> @@ -358,13 +359,16 @@ SHLIBUNWIND_INSTALL =
>  
>  
>  # Create links to files specified in config.host.
> -LIBGCC_LINKS = enable-execute-stack.c unwind.h md-unwind-support.h \
> +LIBGCC_LINKS = enable-execute-stack.c \
> +               unwind.h md-unwind-def.h md-unwind-support.h \
>                 sfp-machine.h gthr-default.h
>  
>  enable-execute-stack.c: $(srcdir)/$(enable_execute_stack)
>  	-$(LN_S) $< $@
>  unwind.h: $(srcdir)/$(unwind_header)
>  	-$(LN_S) $< $@
> +md-unwind-def.h: $(srcdir)/config/$(md_unwind_def_header)
> +	-$(LN_S) $< $@
>  md-unwind-support.h: $(srcdir)/config/$(md_unwind_header)
>  	-$(LN_S) $< $@
>  sfp-machine.h: $(srcdir)/config/$(sfp_machine_header)
> diff --git a/libgcc/config.host b/libgcc/config.host
> index 9fae51d4ce7..61825e72fe4 100644
> --- a/libgcc/config.host
> +++ b/libgcc/config.host
> @@ -51,8 +51,10 @@
>  #			If either is set, EXTRA_PARTS and
>  #			EXTRA_MULTILIB_PARTS inherited from the GCC
>  #			subdirectory will be ignored.
> -#  md_unwind_header	The name of a header file defining
> -#			MD_FALLBACK_FRAME_STATE_FOR.
> +#  md_unwind_def_header The name of a header file defining architecture-specific
> +#			frame information types for unwinding.
> +#  md_unwind_header	The name of a header file defining architecture-specific
> +#			handlers used in the unwinder.
>  #  sfp_machine_header	The name of a sfp-machine.h header file for soft-fp.
>  #  			Defaults to "$cpu_type/sfp-machine.h" if it exists,
>  #			no-sfp-machine.h otherwise.
> @@ -72,6 +74,7 @@ extra_parts=
>  tmake_file=
>  tm_file=
>  tm_define=
> +md_unwind_def_header=no-unwind.h
>  md_unwind_header=no-unwind.h
>  unwind_header=unwind-generic.h
>  
> @@ -403,6 +406,7 @@ aarch64*-*-elf | aarch64*-*-rtems*)
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
>  	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
>  	tmake_file="${tmake_file} t-dfprules"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/aarch64-unwind.h
>  	;;
>  aarch64*-*-freebsd*)
> @@ -411,6 +415,7 @@ aarch64*-*-freebsd*)
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
>  	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
>  	tmake_file="${tmake_file} t-dfprules"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/freebsd-unwind.h
>  	;;
>  aarch64*-*-netbsd*)
> @@ -418,6 +423,7 @@ aarch64*-*-netbsd*)
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
>  	tmake_file="${tmake_file} t-dfprules"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/aarch64-unwind.h
>  	;;
>  aarch64*-*-fuchsia*)
> @@ -428,6 +434,7 @@ aarch64*-*-fuchsia*)
>  	;;
>  aarch64*-*-linux*)
>  	extra_parts="$extra_parts crtfastmath.o"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/linux-unwind.h
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
> @@ -437,6 +444,7 @@ aarch64*-*-linux*)
>  	;;
>  aarch64*-*-gnu*)
>  	extra_parts="$extra_parts crtfastmath.o"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/gnu-unwind.h
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
> @@ -446,6 +454,7 @@ aarch64*-*-gnu*)
>  	;;
>  aarch64*-*-vxworks7*)
>  	extra_parts="$extra_parts crtfastmath.o"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/aarch64-unwind.h
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse"
> diff --git a/libgcc/config/aarch64/aarch64-unwind-def.h b/libgcc/config/aarch64/aarch64-unwind-def.h
> new file mode 100644
> index 00000000000..d06b3222bed
> --- /dev/null
> +++ b/libgcc/config/aarch64/aarch64-unwind-def.h
> @@ -0,0 +1,41 @@
> +/* Copyright (C) 2024 Free Software Foundation, Inc.
> +   Contributed by Arm Ltd.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +for more details.
> +
> +Under Section 7 of GPL version 3, you are granted additional
> +permissions described in the GCC Runtime Library Exception, version
> +3.1, as published by the Free Software Foundation.
> +
> +You should have received a copy of the GNU General Public License and
> +a copy of the GCC Runtime Library Exception along with this program;
> +see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> +<http://www.gnu.org/licenses/>.  */
> +
> +#if !defined (AARCH64_UNWIND_DEF_H) && !defined (__ILP32__)
> +#define AARCH64_UNWIND_DEF_H
> +
> +/* The key used to sign a function's return address. */
> +typedef enum {
> +  AARCH64_PAUTH_KEY_A,
> +  AARCH64_PAUTH_KEY_B,
> +} __attribute__((packed)) aarch64_pointer_auth_key;
> +
> +typedef struct
> +{
> +  aarch64_pointer_auth_key signing_key;
> +} _AArch64Ext_Unwind_FrameState;
> +
> +#define MD_ARCH_FRAME_STATE_T _AArch64Ext_Unwind_FrameState
> +
> +#endif /* defined AARCH64_UNWIND_DEF_H && defined __ILP32__ */
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index cc225a7e207..e0da98d5e3b 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -25,6 +25,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
>  #define AARCH64_UNWIND_H
>  
> +#include "aarch64-unwind-def.h"
> +
>  #include "ansidecl.h"
>  #include <stdbool.h>
>  
> @@ -38,12 +40,6 @@ typedef enum
>    AARCH64_RA_signing_SP = 0x1,
>  } __attribute__((packed)) aarch64_RA_signing_method_t;
>  
> -/* The key used to sign a function's return address. */
> -typedef enum {
> -  AARCH64_PAUTH_KEY_A,
> -  AARCH64_PAUTH_KEY_B,
> -} __attribute__((packed)) aarch64_pointer_auth_key;
> -
>  #define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
>    aarch64_cie_aug_handler(fs, aug)
>  
> @@ -92,7 +88,7 @@ aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
>    // AArch64 B-key pointer authentication.
>    if (aug == 'B')
>      {
> -      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
> +      fs->regs.arch_fs.signing_key = AARCH64_PAUTH_KEY_B;
>        return true;
>      }
>    return false;
> @@ -109,7 +105,7 @@ aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNU
>  				   _Unwind_FrameState *fs)
>  {
>    // Reset previously cached CIE/FDE information.
> -  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
> +  fs->regs.arch_fs.signing_key = AARCH64_PAUTH_KEY_A;
>  
>    // Reset some registers.
>    // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
> @@ -169,7 +165,7 @@ aarch64_demangle_return_addr (struct _Unwind_Context *context,
>    if (signing_method == AARCH64_RA_signing_SP)
>      {
>        _Unwind_Word salt = (_Unwind_Word) context->cfa;
> -      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
> +      if (fs->regs.arch_fs.signing_key == AARCH64_PAUTH_KEY_B)
>  	return __builtin_aarch64_autib1716 (addr, salt);
>        return __builtin_aarch64_autia1716 (addr, salt);
>      }
> diff --git a/libgcc/configure.ac b/libgcc/configure.ac
> index c2749fe0958..ca341479177 100644
> --- a/libgcc/configure.ac
> +++ b/libgcc/configure.ac
> @@ -727,6 +727,7 @@ AC_SUBST(extra_parts)
>  AC_SUBST(asm_hidden_op)
>  AC_SUBST(enable_execute_stack)
>  AC_SUBST(unwind_header)
> +AC_SUBST(md_unwind_def_header)
>  AC_SUBST(md_unwind_header)
>  AC_SUBST(sfp_machine_header)
>  AC_SUBST(thread_header)
> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
> index b75f4c65f98..789150d3171 100644
> --- a/libgcc/unwind-dw2.h
> +++ b/libgcc/unwind-dw2.h
> @@ -22,6 +22,8 @@
>     see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include "md-unwind-def.h"
> +
>  enum register_rule
>  {
>    REG_UNSAVED,
> @@ -71,8 +73,8 @@ typedef struct
>       * Note: those information have to be saved in struct frame_state_reg_info
>       * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
>       * restore them.  */
> -#if defined(__aarch64__) && !defined (__ILP32__)
> -    unsigned char signing_key;
> +#if defined(MD_ARCH_FRAME_STATE_T)
> +    MD_ARCH_FRAME_STATE_T arch_fs;
>  #endif
>    } regs;
  

On 2024-08-06 11:21, Richard Sandiford wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
>> This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
>> architecture-specific structure containing CIE and FDE data related
>> to DWARF architecture extensions.
>>
>> Hiding the architecture-specific attributes behind a handler has the
>> following benefits:
>> 1. isolating those data from the generic ones in _Unwind_FrameState
>> 2. avoiding casts to custom types.
>> 3. preserving typing information when debugging with GDB, and so
>>     facilitating their printing.
>>
>> This approach required to add a new header md-unwind-def.h included at
>> the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
>> architecture header via a symbolic link.
>>
>> An obvious drawback is the increase in complexity with macros, and
>> headers. It also caused a split of architecture definitions between
>> md-unwind-def.h (types definitions used in unwind-dw2.h) and
>> md-unwind.h (local types definitions and handlers implementations).
>> The naming of md-unwind.h with .h extension is a bit misleading as
>> the file is only included in the middle of unwind-dw2.c. Changing
>> this naming would require modification of others backends, which I
>> prefered to abstain from. Overall the benefits are worth the added
>> complexity from my perspective.
> 
> Sorry, I should have read 2/3 before making the suggestion in the
> previous review.  I agree that it makes sense to separate this change
> out, given that it involves a new header file.
> 
> It'd be good to update the comment in no-unwind.h:
> 
> /* Dummy header for targets without a definition of
>     MD_FALLBACK_FRAME_STATE_FOR.  */

Done

> 
> LGTM otherwise, thanks.
> 
> On patch 3: IMO it's better to post the regenerated files as part
> of the same patch, so that each patch is self-contained.
 >
 > Richard
 >

I did it this way as it makes things easier to rebase, and regenerate 
the files if there is a conflict.
I agree, this patch can be squashed with the previous when merging those 
changes into master.

Matthieu