Matthieu Longo <matthieu.lo...@arm.com> writes:
> Architecture-specific CFI directives are currently declared an processed
> among others architecture-independent CFI directives in gcc/dwarf2* files.
> This approach creates confusion, specifically in the case of DWARF
> instructions in the vendor space and using the same instruction code.
>
> Such a clash currently happen between DW_CFA_GNU_window_save (used on
> SPARC) and DW_CFA_AARCH64_negate_ra_state (used on AArch64), and both
> having the same instruction code 0x2d.
> Then AArch64 compilers generates a SPARC CFI directive (.cfi_window_save)
> instead of .cfi_negate_ra_state, contrarilly to what is expected in
> [DWARF for the Arm 64-bit Architecture (AArch64)](https://github.com/
> ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst).
>
> This refactoring does not solve completely the problem, but improve the
> situation by moving some of the processing of those directives (more
> specifically their output in the assembly) to the backend via 2 target
> hooks:
> - DW_CFI_OPRND1_DESC: parse the first operand of the directive (if any).
> - OUTPUT_CFI_DIRECTIVE: output the CFI directive as a string.
>
> Additionally, this patch also contains a renaming of an enum used for
> return address mangling on AArch64.
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.cc
> (aarch64_output_cfi_directive): New hook for CFI directives.
> (aarch64_dw_cfi_oprnd1_desc): Same.
> (TARGET_OUTPUT_CFI_DIRECTIVE): Hook for output_cfi_directive.
> (TARGET_DW_CFI_OPRND1_DESC): Hook for dw_cfi_oprnd1_desc.
> * config/sparc/sparc.cc
> (sparc_output_cfi_directive): New hook for CFI directives.
> (sparc_dw_cfi_oprnd1_desc): Same.
> (TARGET_OUTPUT_CFI_DIRECTIVE): Hook for output_cfi_directive.
> (TARGET_DW_CFI_OPRND1_DESC): Hook for dw_cfi_oprnd1_desc.
> * coretypes.h
> (struct dw_cfi_node): Forward declaration of CFI type from
> gcc/dwarf2out.h.
> (enum dw_cfi_oprnd_type): Same.
> * doc/tm.texi: Regenerated from doc/tm.texi.in.
> * doc/tm.texi.in: Add doc for OUTPUT_CFI_DIRECTIVE and
> DW_CFI_OPRND1_DESC.
> * dwarf2cfi.cc
> (struct dw_cfi_row): Update the description for window_save
> and ra_mangled.
> (cfi_equal_p): Adapt parameter of dw_cfi_oprnd1_desc.
> (dwarf2out_frame_debug_cfa_negate_ra_state): Use AArch64 CFI
> directive instead of the SPARC one.
> (change_cfi_row): Use the right CFI directive's name for RA
> mangling.
> (output_cfi): Remove explicit architecture-specific CFI
> directive DW_CFA_GNU_window_save that falls into default case.
> (output_cfi_directive): Use target hook as default.
> * dwarf2out.cc (dw_cfi_oprnd1_desc): Use target hook as default.
> * dwarf2out.h (enum dw_cfi_oprnd_type): specify underlying type
> of enum to allow forward declaration.
> (dw_cfi_oprnd1_desc): Change type of parameter.
> (output_cfi_directive): Use dw_cfi_ref instead of struct
> dw_cfi_node *.
> * target.def: Documentation for new hooks.
>
> libffi/ChangeLog:
>
> * include/ffi_cfi.h (cfi_negate_ra_state): Declare AArch64 cfi
> directive.
>
> libgcc/ChangeLog:
>
> * config/aarch64/aarch64-asm.h (PACIASP): Replace SPARC CFI
> directive by AArch64 one.
> (AUTIASP): Same.
>
> libitm/ChangeLog:
>
> * config/aarch64/sjlj.S: Replace SPARC CFI directive by
> AArch64 one.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/aarch64/pr94515-1.C: Replace SPARC CFI directive by
> AArch64 one.
> * g++.target/aarch64/pr94515-2.C: Same.
> ---
> gcc/config/aarch64/aarch64.cc | 32 +++++++++++++++++
> gcc/config/sparc/sparc.cc | 36 ++++++++++++++++++++
> gcc/coretypes.h | 6 ++++
> gcc/doc/tm.texi | 28 +++++++++++++++
> gcc/doc/tm.texi.in | 17 +++++++++
> gcc/dwarf2cfi.cc | 29 ++++++----------
> gcc/dwarf2out.cc | 13 ++++---
> gcc/dwarf2out.h | 10 +++---
> gcc/target.def | 20 +++++++++++
> gcc/testsuite/g++.target/aarch64/pr94515-1.C | 6 ++--
> gcc/testsuite/g++.target/aarch64/pr94515-2.C | 2 +-
> libffi/include/ffi_cfi.h | 2 ++
> libgcc/config/aarch64/aarch64-asm.h | 4 +--
> libitm/config/aarch64/sjlj.S | 10 +++---
> 14 files changed, 176 insertions(+), 39 deletions(-)
Generally looks good to me. Most of the comments below are very minor,
but there's also a question about the hook interface.
>
> [...]
> @@ -12621,6 +12630,33 @@ sparc_output_dwarf_dtprel (FILE *file, int size, rtx
> x)
> fputs (")", file);
> }
>
> +/* This is called from gcc/dwarf2cfi.cc via TARGET_OUTPUT_CFI_DIRECTIVE.
> + It outputs SPARC-specific CFI directives (if any). */
I think we should use the same comments as for aarch64, i.e.:
/* Implement TARGET_OUTPUT_CFI_DIRECTIVE. */
...
/* Implement TARGET_DW_CFI_OPRND1_DESC. */
The idea is that the hook descriptions in target.def/tm.texi should be
self-contained.
> +static bool
> +sparc_output_cfi_directive (FILE *f, dw_cfi_ref cfi)
> +{
> + if (cfi->dw_cfi_opc == DW_CFA_GNU_window_save)
> + {
> + fprintf (f, "\t.cfi_window_save\n");
> + return true;
> + }
> + return false;
> +}
> +
> +/* This is called from gcc/dwarf2out.cc via TARGET_DW_CFI_OPRND1_DESC.
> + It describes for the GTY machinery what parts of the first operand
> + is used. */
> +static bool
> +sparc_dw_cfi_oprnd1_desc (dw_cfi_ref cfi, dw_cfi_oprnd_type_ref oprnd_type)
> +{
> + if (cfi->dw_cfi_opc == DW_CFA_GNU_window_save)
> + {
> + oprnd_type = dw_cfi_oprnd_unused;
> + return true;
> + }
> + return false;
> +}
> +
> /* Do whatever processing is required at the end of a file. */
>
> static void
> diff --git a/gcc/coretypes.h b/gcc/coretypes.h
> index 0544bb8fd97..3b622961b7a 100644
> --- a/gcc/coretypes.h
> +++ b/gcc/coretypes.h
> @@ -141,6 +141,12 @@ struct gomp_single;
> struct gomp_target;
> struct gomp_teams;
>
> +/* Forward declaration of CFI's and DWARF's types. */
> +struct dw_cfi_node;
> +using dw_cfi_ref = struct dw_cfi_node *;
It'd be good to remove the corresponding typedef in dwarf2out.h.
> +enum dw_cfi_oprnd_type: unsigned int;
> +using dw_cfi_oprnd_type_ref = enum dw_cfi_oprnd_type &;
IMO it'd better to avoid adding this, and instead just use
dw_cfi_oprnd_type &
directly.
I think the use of typedefs for pointers (as for dw_cfi_ref) was more
popular when the codebase was written in C, and where the typedef often
saved on a struct/union/enum tag. I don't think it's something we
should use for C++ references since it hides the fact that:
void foo (dw_cfi_ref x, dw_cfi_ref y) { ... x = y; ... }
is a normal copy whereas:
void foo (dw_cfi_oprnd_type_ref x, dw_cfi_oprnd_type_ref y) { ... x = y; ... }
isn't.
> +
> /* Subclasses of symtab_node, using indentation to show the class
> hierarchy. */
>
> [...]
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 64cea3b1eda..051217d68b1 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -3108,7 +3108,20 @@ used in .eh_frame or .debug_frame is different from
> that used in other
> debug info sections. Given a GCC hard register number, this macro
> should return the .eh_frame register number. The default is
> @code{DEBUGGER_REGNO (@var{regno})}.
> +@end defmac
> +
> +
> +@defmac OUTPUT_CFI_DIRECTIVE (@var{f}, @var{cfi})
> +
> +Define this macro if the target has additional CFI directives. Return
> +true if an architecture-specific directive was found, false otherwise.
> +@end defmac
> +
> +@defmac DW_CFI_OPRND1_DESC (@var{cfi}, @var{oprnd_type})
>
> +Define this macro if the target has additional CFI directives. Return
> +true if an architecture-specific directive was found and @var{oprnd_type}
> +is set, false otherwise and @var{oprnd_type} is not modified.
> @end defmac
The new hooks are proper target hooks rather than macros (which is good!)
so there's no need to have and document macros as well. (Macros are a
hold-over from before target hooks existed.)
> @defmac DWARF2_FRAME_REG_OUT (@var{regno}, @var{for_eh})
> [...]
> @@ -1547,17 +1549,13 @@ dwarf2out_frame_debug_cfa_window_save (void)
> cur_row->window_save = true;
> }
>
> -/* A subroutine of dwarf2out_frame_debug, process a REG_CFA_NEGATE_RA_STATE.
> - Note: DW_CFA_GNU_window_save dwarf opcode is reused for toggling RA mangle
> - state, this is a target specific operation on AArch64 and can only be used
> - on other targets if they don't use the window save operation otherwise.
> */
> +/*A subroutine of dwarf2out_frame_debug, process REG_CFA_NEGATE_RA_STATE. */
Nit: should be a space before "A subroutine ...".
>
> static void
> dwarf2out_frame_debug_cfa_negate_ra_state (void)
> {
> dw_cfi_ref cfi = new_cfi ();
> -
> - cfi->dw_cfi_opc = DW_CFA_GNU_window_save;
> + cfi->dw_cfi_opc = DW_CFA_AARCH64_negate_ra_state;
> add_cfi (cfi);
> cur_row->ra_mangled = !cur_row->ra_mangled;
> }
> [...]
> diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
> index 357efaa5990..260593421df 100644
> --- a/gcc/dwarf2out.cc
> +++ b/gcc/dwarf2out.cc
> @@ -516,12 +516,11 @@ switch_to_frame_table_section (int for_eh, bool back)
> /* Describe for the GTY machinery what parts of dw_cfi_oprnd1 are used. */
>
> enum dw_cfi_oprnd_type
> -dw_cfi_oprnd1_desc (enum dwarf_call_frame_info cfi)
> +dw_cfi_oprnd1_desc (dw_cfi_ref cfi)
> {
> - switch (cfi)
> + switch (cfi->dw_cfi_opc)
> {
> case DW_CFA_nop:
> - case DW_CFA_GNU_window_save:
> case DW_CFA_remember_state:
> case DW_CFA_restore_state:
> return dw_cfi_oprnd_unused;
> @@ -557,7 +556,13 @@ dw_cfi_oprnd1_desc (enum dwarf_call_frame_info cfi)
> return dw_cfi_oprnd_loc;
>
> default:
> - gcc_unreachable ();
> + {
> + dw_cfi_oprnd_type oprnd_type;
> + if (targetm.dw_cfi_oprnd1_desc (cfi, oprnd_type))
Could you say why passing opcode isn't enough? I guess this is to do
with some of the changes that are coming later?
I think the correspondong:
> - return (cfi_oprnd_equal_p (dw_cfi_oprnd1_desc (opc),
> + return (cfi_oprnd_equal_p (dw_cfi_oprnd1_desc (a),
> &a->dw_cfi_oprnd1, &b->dw_cfi_oprnd1)
> && cfi_oprnd_equal_p (dw_cfi_oprnd2_desc (opc),
> &a->dw_cfi_oprnd2, &b->dw_cfi_oprnd2));
makes the corretness of cfi_equal_p a bit fuzzier/less obvious,
since it seems that dw_cfi_oprnd1_desc (a) is checking something
about a that is not checked for b.
Thanks,
Richard
> + return oprnd_type;
> + else
> + gcc_unreachable ();
> + }
> }
> }
>
