Matthieu Longo <matthieu.lo...@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;
