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 ();
+ }
}
}
