[GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Srinath Parvathaneni]

[-- Attachment #1: Type: text/plain, Size: 3149 bytes --]

Hello,

This patch teaches the DWARF support in gcc about RA_AUTH_CODE pseudo hard-register and also 
updates the ".save", ".cfi_register", ".cfi_offset", ".cfi_restore" directives accordingly.
This patch also adds support to emit ".pacspval" directive when "pac ip, lr, sp" instruction
in generated in the assembly.

RA_AUTH_CODE register number is 107 and it's dwarf register number is 143.

Applying this patch on top of PACBTI series posted here
https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599658.html and when compiling the following
test.c with "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard
fasynchronous-unwind-tables -g -O0 -S" command line options, the assembly output after this patch
looks like below:

$cat test.c

void fun1(int a);
void fun(int a,...)
{
  fun1(a);
}

int main()
{
  fun (10);
  return 0;
}

$ arm-none-eabi-gcc -march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard
-fasynchronous-unwind-tables -g -O0 -S test.s

Assembly output:
...
fun:
...
        .pacspval
        pac     ip, lr, sp
        .cfi_register 143, 12
        push    {r3, r7, ip, lr}
        .save {r3, r7, ra_auth_code, lr}
...
        .cfi_offset 143, -24
...
        .cfi_restore 143
...
        aut     ip, lr, sp
        bx      lr
...
main:
...
        .pacspval
        pac     ip, lr, sp
        .cfi_register 143, 12
        push    {r3, r7, ip, lr}
        .save {r3, r7, ra_auth_code, lr}
...
        .cfi_offset 143, -8
...
        .cfi_restore 143
...
        aut     ip, lr, sp
        bx      lr
...

Regression tested on arm-none-eabi target and found no regressions.

Ok for master?

Regards,
Srinath.

2023-01-11  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>

        * config/arm/aout.h (ra_auth_code): Add entry in enum.
        (emit_multi_reg_push): Add RA_AUTH_CODE register to
        dwarf frame expression.
        (arm_emit_multi_reg_pop): Restore RA_AUTH_CODE register.
        (arm_expand_prologue): Update frame related information and reg notes
        for pac/pacbit insn.
        (arm_regno_class): Check for pac pseudo reigster.
        (arm_dbx_register_number): Assign ra_auth_code register number in dwarf.
        (arm_init_machine_status): Set pacspval_needed to zero.
        (arm_debugger_regno): Check for PAC register.
        (arm_unwind_emit_sequence): Print .save directive with ra_auth_code
        register.
        (arm_unwind_emit_set): Add entry for IP_REGNUM in switch case.
        (arm_unwind_emit): Update REG_CFA_REGISTER case._
        * config/arm/arm.h (FIRST_PSEUDO_REGISTER): Modify.
        (DWARF_PAC_REGNUM): Define.
        (IS_PAC_REGNUM): Likewise.
        (enum reg_class): Add PAC_REG entry.
        (machine_function): Add pacbti_needed state to structure.
        * config/arm/arm.md (RA_AUTH_CODE): Define.

gcc/testsuite/ChangeLog:

2023-01-11  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>

        * g++.target/arm/pac-1.C: New test.
        * gcc.target/arm/pac-15.c: Likewise.

[-- Attachment #2: dwarf_pacbti --]
[-- Type: application/octet-stream, Size: 15182 bytes --]

diff --git a/gcc/config/arm/aout.h b/gcc/config/arm/aout.h
index cfb8db52c59fb5178178a2bb419f0ffc51077ed3..92f844dcf9b87814c39c3662cbed9d316fb0228d 100644
--- a/gcc/config/arm/aout.h
+++ b/gcc/config/arm/aout.h
@@ -74,7 +74,8 @@
   "wr8",   "wr9",   "wr10",  "wr11",				\
   "wr12",  "wr13",  "wr14",  "wr15",				\
   "wcgr0", "wcgr1", "wcgr2", "wcgr3",				\
-  "cc", "vfpcc", "sfp", "afp", "apsrq", "apsrge", "p0"		\
+  "cc", "vfpcc", "sfp", "afp", "apsrq", "apsrge", "p0",		\
+  "ra_auth_code"						\
 }
 #endif
 
diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index 5f7c40805d3fe972b61a587f6bdd0c668c1c683d..c90cb0411e5c126cffdcd106e6af5a458de06217 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -816,7 +816,8 @@ extern const int arm_arch_cde_coproc_bits[];
 	s16-s31	      S	VFP variable (aka d8-d15).
 	vfpcc		Not a real register.  Represents the VFP condition
 			code flags.
-	vpr		Used to represent MVE VPR predication.  */
+	vpr		Used to represent MVE VPR predication.
+	ra_auth_code	Pseudo register to save PAC.  */
 
 /* The stack backtrace structure is as follows:
   fp points to here:  |  save code pointer  |      [fp]
@@ -857,7 +858,7 @@ extern const int arm_arch_cde_coproc_bits[];
   1,1,1,1,1,1,1,1,		\
   1,1,1,1,			\
   /* Specials.  */		\
-  1,1,1,1,1,1,1			\
+  1,1,1,1,1,1,1,1		\
 }
 
 /* 1 for registers not available across function calls.
@@ -887,7 +888,7 @@ extern const int arm_arch_cde_coproc_bits[];
   1,1,1,1,1,1,1,1,		\
   1,1,1,1,			\
   /* Specials.  */		\
-  1,1,1,1,1,1,1			\
+  1,1,1,1,1,1,1,1		\
 }
 
 #ifndef SUBTARGET_CONDITIONAL_REGISTER_USAGE
@@ -1063,10 +1064,12 @@ extern const int arm_arch_cde_coproc_bits[];
    && (LAST_VFP_REGNUM - (REGNUM) >= 2 * (N) - 1))
 
 /* The number of hard registers is 16 ARM + 1 CC + 1 SFP + 1 AFP
-   + 1 APSRQ + 1 APSRGE + 1 VPR.  */
+   + 1 APSRQ + 1 APSRGE + 1 VPR + 1 Pseudo register to save PAC.  */
 /* Intel Wireless MMX Technology registers add 16 + 4 more.  */
 /* VFP (VFP3) adds 32 (64) + 1 VFPCC.  */
-#define FIRST_PSEUDO_REGISTER   107
+#define FIRST_PSEUDO_REGISTER   108
+
+#define DWARF_PAC_REGNUM 143
 
 #define DEBUGGER_REGNO(REGNO) arm_debugger_regno (REGNO)
 
@@ -1253,12 +1256,15 @@ extern int arm_regs_in_sequence[];
   CC_REGNUM, VFPCC_REGNUM,			\
   FRAME_POINTER_REGNUM, ARG_POINTER_REGNUM,	\
   SP_REGNUM, PC_REGNUM, APSRQ_REGNUM,		\
-  APSRGE_REGNUM, VPR_REGNUM			\
+  APSRGE_REGNUM, VPR_REGNUM, RA_AUTH_CODE	\
 }
 
 #define IS_VPR_REGNUM(REGNUM) \
   ((REGNUM) == VPR_REGNUM)
 
+#define IS_PAC_REGNUM(REGNUM) \
+  ((REGNUM) == RA_AUTH_CODE)
+
 /* Use different register alloc ordering for Thumb.  */
 #define ADJUST_REG_ALLOC_ORDER arm_order_regs_for_local_alloc ()
 
@@ -1297,6 +1303,7 @@ enum reg_class
   SFP_REG,
   AFP_REG,
   VPR_REG,
+  PAC_REG,
   GENERAL_AND_VPR_REGS,
   ALL_REGS,
   LIM_REG_CLASSES
@@ -1327,6 +1334,7 @@ enum reg_class
   "SFP_REG",		\
   "AFP_REG",		\
   "VPR_REG",		\
+  "PAC_REG",		\
   "GENERAL_AND_VPR_REGS", \
   "ALL_REGS"		\
 }
@@ -1356,6 +1364,7 @@ enum reg_class
   { 0x00000000, 0x00000000, 0x00000000, 0x00000040 }, /* SFP_REG */	\
   { 0x00000000, 0x00000000, 0x00000000, 0x00000080 }, /* AFP_REG */	\
   { 0x00000000, 0x00000000, 0x00000000, 0x00000400 }, /* VPR_REG.  */	\
+  { 0x00000000, 0x00000000, 0x00000000, 0x00000800 }, /* PAC_REG.  */	\
   { 0x00005FFF, 0x00000000, 0x00000000, 0x00000400 }, /* GENERAL_AND_VPR_REGS.  */ \
   { 0xFFFF7FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x0000040F }  /* ALL_REGS.  */	\
 }
@@ -1621,6 +1630,9 @@ typedef struct GTY(()) machine_function
   /* The number of bytes used to store the static chain register on the
      stack, above the stack frame.  */
   int static_chain_stack_bytes;
+  /* Set to 1 when pointer authentication operation uses value of SP other
+     than the incoming stack pointer value.  */
+  int pacspval_needed;
 }
 machine_function;
 #endif
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 0f105f60c083b7906d07056780be0bc3d0fde450..69bc29148958a825edb225310dbe6f7ff6d6e52f 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -22248,7 +22248,35 @@ emit_multi_reg_push (unsigned long mask, unsigned long dwarf_regs_mask)
     {
       if (mask & (1 << i))
 	{
-	  reg = gen_rtx_REG (SImode, i);
+	  /* NOTE: Dwarf code emitter handle reg-reg copies correctly and in the
+	     following example reg-reg copy of SP to IP register is handled
+	     through .cfi_def_cfa_register directive and the .cfi_offset
+	     directive for IP register is skipped by dwarf code emitter.
+	     Example:
+		mov     ip, sp
+		.cfi_def_cfa_register 12
+		push    {fp, ip, lr, pc}
+		.cfi_offset 11, -16
+		.cfi_offset 13, -12
+		.cfi_offset 14, -8
+
+	     Where as Arm-specific .save directive reg-reg copy handling is
+	     buggy.  After the reg-reg copy, the copied registers need to be
+	     populated in .save directive register list but with the current
+	     implementation of .save directive original registers are getting
+	     populated in the register list.  So to avoid this issue for IP
+	     register when PACBTI is enabled we manually updated the .save
+	     directive register list to use "ra_auth_code" (pseduo register 143)
+	     instead of IP register as shown in following example.
+	     Example:
+		pacbti  ip, lr, sp
+		.cfi_register 143, 12
+		push    {r3, r7, ip, lr}
+		.save {r3, r7, ra_auth_code, lr}
+	  */
+	  rtx dwarf_reg = reg = gen_rtx_REG (SImode, i);
+	  if (arm_current_function_pac_enabled_p () && i == IP_REGNUM)
+	    dwarf_reg = gen_rtx_REG (SImode, RA_AUTH_CODE);
 
 	  XVECEXP (par, 0, 0)
 	    = gen_rtx_SET (gen_frame_mem
@@ -22266,7 +22294,7 @@ emit_multi_reg_push (unsigned long mask, unsigned long dwarf_regs_mask)
 	  if (dwarf_regs_mask & (1 << i))
 	    {
 	      tmp = gen_rtx_SET (gen_frame_mem (SImode, stack_pointer_rtx),
-				 reg);
+				 dwarf_reg);
 	      RTX_FRAME_RELATED_P (tmp) = 1;
 	      XVECEXP (dwarf, 0, dwarf_par_index++) = tmp;
 	    }
@@ -22279,7 +22307,9 @@ emit_multi_reg_push (unsigned long mask, unsigned long dwarf_regs_mask)
     {
       if (mask & (1 << i))
 	{
-	  reg = gen_rtx_REG (SImode, i);
+	  rtx dwarf_reg = reg = gen_rtx_REG (SImode, i);
+	  if (arm_current_function_pac_enabled_p () && i == IP_REGNUM)
+	    dwarf_reg = gen_rtx_REG (SImode, RA_AUTH_CODE);
 
 	  XVECEXP (par, 0, j) = gen_rtx_USE (VOIDmode, reg);
 
@@ -22290,7 +22320,7 @@ emit_multi_reg_push (unsigned long mask, unsigned long dwarf_regs_mask)
 			       (SImode,
 				plus_constant (Pmode, stack_pointer_rtx,
 					       4 * j)),
-			       reg);
+			       dwarf_reg);
 	      RTX_FRAME_RELATED_P (tmp) = 1;
 	      XVECEXP (dwarf, 0, dwarf_par_index++) = tmp;
 	    }
@@ -22375,7 +22405,9 @@ arm_emit_multi_reg_pop (unsigned long saved_regs_mask)
   for (j = 0, i = 0; j < num_regs; i++)
     if (saved_regs_mask & (1 << i))
       {
-        reg = gen_rtx_REG (SImode, i);
+	rtx dwarf_reg = reg = gen_rtx_REG (SImode, i);
+	if (arm_current_function_pac_enabled_p () && i == IP_REGNUM)
+	  dwarf_reg = gen_rtx_REG (SImode, RA_AUTH_CODE);
         if ((num_regs == 1) && emit_update && !return_in_pc)
           {
             /* Emit single load with writeback.  */
@@ -22383,7 +22415,8 @@ arm_emit_multi_reg_pop (unsigned long saved_regs_mask)
                                  gen_rtx_POST_INC (Pmode,
                                                    stack_pointer_rtx));
             tmp = emit_insn (gen_rtx_SET (reg, tmp));
-            REG_NOTES (tmp) = alloc_reg_note (REG_CFA_RESTORE, reg, dwarf);
+	    REG_NOTES (tmp) = alloc_reg_note (REG_CFA_RESTORE, dwarf_reg,
+					      dwarf);
             return;
           }
 
@@ -22397,7 +22430,7 @@ arm_emit_multi_reg_pop (unsigned long saved_regs_mask)
         /* We need to maintain a sequence for DWARF info too.  As dwarf info
            should not have PC, skip PC.  */
         if (i != PC_REGNUM)
-          dwarf = alloc_reg_note (REG_CFA_RESTORE, reg, dwarf);
+	  dwarf = alloc_reg_note (REG_CFA_RESTORE, dwarf_reg, dwarf);
 
         j++;
       }
@@ -23589,6 +23622,8 @@ arm_expand_prologue (void)
 					      -fp_offset));
 	  RTX_FRAME_RELATED_P (insn) = 1;
 	  add_reg_note (insn, REG_FRAME_RELATED_EXPR, dwarf);
+	  if (arm_current_function_pac_enabled_p ())
+	    cfun->machine->pacspval_needed = 1;
 	}
       else
 	{
@@ -23624,6 +23659,8 @@ arm_expand_prologue (void)
 	  RTX_FRAME_RELATED_P (insn) = 1;
 	  fp_offset = args_to_push;
 	  args_to_push = 0;
+	  if (arm_current_function_pac_enabled_p ())
+	    cfun->machine->pacspval_needed = 1;
 	}
     }
 
@@ -23633,9 +23670,13 @@ arm_expand_prologue (void)
          one will be added before the push of the clobbered IP (if
          necessary) by the bti pass.  */
       if (aarch_bti_enabled () && !clobber_ip)
-	emit_insn (gen_pacbti_nop ());
+	insn = emit_insn (gen_pacbti_nop ());
       else
-	emit_insn (gen_pac_nop ());
+	insn = emit_insn (gen_pac_nop ());
+
+      rtx dwarf = gen_rtx_SET (ip_rtx, gen_rtx_REG (SImode, RA_AUTH_CODE));
+      RTX_FRAME_RELATED_P (insn) = 1;
+      add_reg_note (insn, REG_CFA_REGISTER, dwarf);
     }
 
   if (TARGET_APCS_FRAME && frame_pointer_needed && TARGET_ARM)
@@ -25717,6 +25758,9 @@ arm_regno_class (int regno)
   if (IS_VPR_REGNUM (regno))
     return VPR_REG;
 
+  if (IS_PAC_REGNUM (regno))
+    return PAC_REG;
+
   if (TARGET_THUMB1)
     {
       if (regno == STACK_POINTER_REGNUM)
@@ -26877,6 +26921,7 @@ arm_init_machine_status (void)
   machine->func_type = ARM_FT_UNKNOWN;
 #endif
   machine->static_chain_stack_bytes = -1;
+  machine->pacspval_needed = 0;
   return machine;
 }
 
@@ -29686,6 +29731,9 @@ arm_debugger_regno (unsigned int regno)
   if (IS_IWMMXT_REGNUM (regno))
     return 112 + regno - FIRST_IWMMXT_REGNUM;
 
+  if (IS_PAC_REGNUM (regno))
+    return DWARF_PAC_REGNUM;
+
   return DWARF_FRAME_REGISTERS;
 }
 
@@ -29779,7 +29827,7 @@ arm_unwind_emit_sequence (FILE * out_file, rtx p)
   gcc_assert (nregs);
 
   reg = REGNO (SET_SRC (XVECEXP (p, 0, 1)));
-  if (reg < 16)
+  if (reg < 16 || IS_PAC_REGNUM (reg))
     {
       /* For -Os dummy registers can be pushed at the beginning to
 	 avoid separate stack pointer adjustment.  */
@@ -29836,6 +29884,8 @@ arm_unwind_emit_sequence (FILE * out_file, rtx p)
 	 double precision register names.  */
       if (IS_VFP_REGNUM (reg))
 	asm_fprintf (out_file, "d%d", (reg - FIRST_VFP_REGNUM) / 2);
+      else if (IS_PAC_REGNUM (reg))
+	asm_fprintf (asm_out_file, "ra_auth_code");
       else
 	asm_fprintf (out_file, "%r", reg);
 
@@ -29930,7 +29980,7 @@ arm_unwind_emit_set (FILE * out_file, rtx p)
 	  /* Move from sp to reg.  */
 	  asm_fprintf (out_file, "\t.movsp %r\n", REGNO (e0));
 	}
-     else if (GET_CODE (e1) == PLUS
+      else if (GET_CODE (e1) == PLUS
 	      && REG_P (XEXP (e1, 0))
 	      && REGNO (XEXP (e1, 0)) == SP_REGNUM
 	      && CONST_INT_P (XEXP (e1, 1)))
@@ -29939,6 +29989,11 @@ arm_unwind_emit_set (FILE * out_file, rtx p)
 	  asm_fprintf (out_file, "\t.movsp %r, #%d\n",
 		       REGNO (e0), (int)INTVAL(XEXP (e1, 1)));
 	}
+      else if (REGNO (e0) == IP_REGNUM && arm_current_function_pac_enabled_p ())
+	{
+	  if (cfun->machine->pacspval_needed)
+	    asm_fprintf (out_file, "\t.pacspval\n");
+	}
       else
 	abort ();
       break;
@@ -29993,10 +30048,15 @@ arm_unwind_emit (FILE * out_file, rtx_insn *insn)
 	    src = SET_SRC (pat);
 	    dest = SET_DEST (pat);
 
-	    gcc_assert (src == stack_pointer_rtx);
+	    gcc_assert (src == stack_pointer_rtx
+			|| IS_PAC_REGNUM (REGNO (src)));
 	    reg = REGNO (dest);
-	    asm_fprintf (out_file, "\t.unwind_raw 0, 0x%x @ vsp = r%d\n",
-			 reg + 0x90, reg);
+
+	    if (IS_PAC_REGNUM (REGNO (src)))
+	      arm_unwind_emit_set (out_file, PATTERN (insn));
+	    else
+	      asm_fprintf (out_file, "\t.unwind_raw 0, 0x%x @ vsp = r%d\n",
+			   reg + 0x90, reg);
 	  }
 	  handled_one = true;
 	  break;
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 36062292b90c300f61ea678a37725f7e32080939..282ba19e3224259cd52294afa08c95fcaf89a60c 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -42,6 +42,7 @@
    (APSRQ_REGNUM    104)	; Q bit pseudo register
    (APSRGE_REGNUM   105)	; GE bits pseudo register
    (VPR_REGNUM      106)	; Vector Predication Register - MVE register.
+   (RA_AUTH_CODE    107)	; Pseudo register to save PAC.
   ]
 )
 ;; 3rd operand to select_dominance_cc_mode
diff --git a/gcc/testsuite/g++.target/arm/pac-1.C b/gcc/testsuite/g++.target/arm/pac-1.C
new file mode 100644
index 0000000000000000000000000000000000000000..f671a27b048c647eee973004d9aab6ec99769e38
--- /dev/null
+++ b/gcc/testsuite/g++.target/arm/pac-1.C
@@ -0,0 +1,35 @@
+/* Check that GCC does .save and .cfi_offset directives with RA_AUTH_CODE pseudo hard-register.  */
+/* { dg-do compile } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */
+/* { dg-options "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard -g -O0" } */
+
+__attribute__((noinline)) void
+fn1 (int a, int b, int c)
+{
+  if (a != b + c)
+    __builtin_abort ();
+  else
+    throw b+c;
+}
+
+int main ()
+{
+  int a = 120;
+  try
+    {
+      fn1 (a, 40, 80);
+    }
+  catch (int x)
+    {
+      if (x != a)
+        __builtin_abort ();
+      else
+	return 0;
+    }
+}
+
+/* { dg-final { scan-assembler-times "pac	ip, lr, sp" 2 } } */
+/* { dg-final { scan-assembler-times "\.cfi_register 143, 12" 2 } } */
+/* { dg-final { scan-assembler-times "\.save {r7, ra_auth_code, lr}" 1 } } */
+/* { dg-final { scan-assembler-times "\.cfi_offset 143, -8" 2 } } */
+/* { dg-final { scan-assembler-times "\.save {r4, r7, ra_auth_code, lr}" 1 } } */
diff --git a/gcc/testsuite/gcc.target/arm/pac-15.c b/gcc/testsuite/gcc.target/arm/pac-15.c
new file mode 100644
index 0000000000000000000000000000000000000000..e1054902955686a78536933580a603d5713794ef
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pac-15.c
@@ -0,0 +1,32 @@
+/* Check that GCC does .save and .cfi_offset directives with RA_AUTH_CODE pseudo hard-register.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target mbranch_protection_ok } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */
+/* { dg-options "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard -fasynchronous-unwind-tables -g -O0" } */
+
+#include "stdio.h"
+
+__attribute__((noinline)) int
+fn1 (int a)
+{
+  const char *fmt = "branch-protection";
+  int fun1(int x,const char *fmt,int c,int d)
+    {
+      printf("string = %s\n",fmt);
+      return x+c+d;
+    }
+  return fun1(a,fmt,10,10);
+}
+
+int main (void)
+{
+  return fn1 (40);
+}
+
+/* { dg-final { scan-assembler-times "\.pacspval" 1 } } */
+/* { dg-final { scan-assembler-times "pac	ip, lr, sp" 3 } } */
+/* { dg-final { scan-assembler-times "\.cfi_register 143, 12" 3 } } */
+/* { dg-final { scan-assembler-times "\.save {r7, ra_auth_code, lr}" 2 } } */
+/* { dg-final { scan-assembler-times "\.cfi_offset 143, -8" 2 } } */
+/* { dg-final { scan-assembler-times "\.save {r3, r7, ra_auth_code, lr}" 1 } } */
+/* { dg-final { scan-assembler-times "\.cfi_offset 143, -12" 1 } } */

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Jakub Jelinek]

On Fri, Jan 13, 2023 at 05:44:15PM +0000, Srinath Parvathaneni via Gcc-patches wrote:
> Hello,
> 
> This patch teaches the DWARF support in gcc about RA_AUTH_CODE pseudo hard-register and also 
> updates the ".save", ".cfi_register", ".cfi_offset", ".cfi_restore" directives accordingly.
> This patch also adds support to emit ".pacspval" directive when "pac ip, lr, sp" instruction
> in generated in the assembly.
> 
> RA_AUTH_CODE register number is 107 and it's dwarf register number is 143.

I'm afraid increasing number of DWARF registers is ABI incompatible change.
E.g. libgcc __frame_state_for function fills in:
typedef struct frame_state
{
  void *cfa;
  void *eh_ptr;
  long cfa_offset;
  long args_size;
  long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
  unsigned short cfa_reg;
  unsigned short retaddr_column;
  char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
} frame_state;

structure, where PRE_GCC3_DWARF_FRAME_REGISTERS defaults to
__LIBGCC_DWARF_FRAME_REGISTERS__, which is defined to
DWARF_FRAME_REGISTERS, which defaults to FIRST_PSEUDO_REGISTER.
So, changing FIRST_PSEUDO_REGISTER is an ABI change unless you arrange for
PRE_GCC3_DWARF_FRAME_REGISTERS to be defined to the old value.

	Jakub

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Richard Earnshaw (lists)]

On 13/01/2023 18:02, Jakub Jelinek via Gcc-patches wrote:
> On Fri, Jan 13, 2023 at 05:44:15PM +0000, Srinath Parvathaneni via Gcc-patches wrote:
>> Hello,
>>
>> This patch teaches the DWARF support in gcc about RA_AUTH_CODE pseudo hard-register and also
>> updates the ".save", ".cfi_register", ".cfi_offset", ".cfi_restore" directives accordingly.
>> This patch also adds support to emit ".pacspval" directive when "pac ip, lr, sp" instruction
>> in generated in the assembly.
>>
>> RA_AUTH_CODE register number is 107 and it's dwarf register number is 143.
> 
> I'm afraid increasing number of DWARF registers is ABI incompatible change.
> E.g. libgcc __frame_state_for function fills in:
> typedef struct frame_state
> {
>    void *cfa;
>    void *eh_ptr;
>    long cfa_offset;
>    long args_size;
>    long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>    unsigned short cfa_reg;
>    unsigned short retaddr_column;
>    char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
> } frame_state;
> 
> structure, where PRE_GCC3_DWARF_FRAME_REGISTERS defaults to
> __LIBGCC_DWARF_FRAME_REGISTERS__, which is defined to
> DWARF_FRAME_REGISTERS, which defaults to FIRST_PSEUDO_REGISTER.
> So, changing FIRST_PSEUDO_REGISTER is an ABI change unless you arrange for
> PRE_GCC3_DWARF_FRAME_REGISTERS to be defined to the old value.
> 
> 	Jakub
> 

So where's the red flag that warns about this?

I also note that Richard Sandiford made a similar type of change for 
AArch64 in r10-4195 (183bfdafc6f1f98711c5400498a7268cc1441096) and 
nothing was said about that at the time.

It seems incredibly fragile to me to have some ABI based off the number 
of machine registers.

R.

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Richard Earnshaw]

On 13/01/2023 21:58, Richard Earnshaw (lists) via Gcc-patches wrote:
> On 13/01/2023 18:02, Jakub Jelinek via Gcc-patches wrote:
>> On Fri, Jan 13, 2023 at 05:44:15PM +0000, Srinath Parvathaneni via 
>> Gcc-patches wrote:
>>> Hello,
>>>
>>> This patch teaches the DWARF support in gcc about RA_AUTH_CODE pseudo 
>>> hard-register and also
>>> updates the ".save", ".cfi_register", ".cfi_offset", ".cfi_restore" 
>>> directives accordingly.
>>> This patch also adds support to emit ".pacspval" directive when "pac 
>>> ip, lr, sp" instruction
>>> in generated in the assembly.
>>>
>>> RA_AUTH_CODE register number is 107 and it's dwarf register number is 
>>> 143.
>>
>> I'm afraid increasing number of DWARF registers is ABI incompatible 
>> change.
>> E.g. libgcc __frame_state_for function fills in:
>> typedef struct frame_state
>> {
>>    void *cfa;
>>    void *eh_ptr;
>>    long cfa_offset;
>>    long args_size;
>>    long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>>    unsigned short cfa_reg;
>>    unsigned short retaddr_column;
>>    char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>> } frame_state;
>>
>> structure, where PRE_GCC3_DWARF_FRAME_REGISTERS defaults to
>> __LIBGCC_DWARF_FRAME_REGISTERS__, which is defined to
>> DWARF_FRAME_REGISTERS, which defaults to FIRST_PSEUDO_REGISTER.
>> So, changing FIRST_PSEUDO_REGISTER is an ABI change unless you arrange 
>> for
>> PRE_GCC3_DWARF_FRAME_REGISTERS to be defined to the old value.
>>
>>     Jakub
>>
> 
> So where's the red flag that warns about this?
> 
> I also note that Richard Sandiford made a similar type of change for 
> AArch64 in r10-4195 (183bfdafc6f1f98711c5400498a7268cc1441096) and 
> nothing was said about that at the time.
> 
> It seems incredibly fragile to me to have some ABI based off the number 
> of machine registers.
> 
> R.

Also, the Arm port does not use dwarf based unwinding, so is this really 
relevant?

R.

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Jakub Jelinek]

On Fri, Jan 13, 2023 at 09:58:26PM +0000, Richard Earnshaw (lists) wrote:
> > I'm afraid increasing number of DWARF registers is ABI incompatible change.
> > E.g. libgcc __frame_state_for function fills in:
> > typedef struct frame_state
> > {
> >    void *cfa;
> >    void *eh_ptr;
> >    long cfa_offset;
> >    long args_size;
> >    long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
> >    unsigned short cfa_reg;
> >    unsigned short retaddr_column;
> >    char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
> > } frame_state;
> > 
> > structure, where PRE_GCC3_DWARF_FRAME_REGISTERS defaults to
> > __LIBGCC_DWARF_FRAME_REGISTERS__, which is defined to
> > DWARF_FRAME_REGISTERS, which defaults to FIRST_PSEUDO_REGISTER.
> > So, changing FIRST_PSEUDO_REGISTER is an ABI change unless you arrange for
> > PRE_GCC3_DWARF_FRAME_REGISTERS to be defined to the old value.
> > 
> > 	Jakub
> > 
> 
> So where's the red flag that warns about this?
> 
> I also note that Richard Sandiford made a similar type of change for AArch64
> in r10-4195 (183bfdafc6f1f98711c5400498a7268cc1441096) and nothing was said
> about that at the time.
> 
> It seems incredibly fragile to me to have some ABI based off the number of
> machine registers.

It is.  The new unwinder fortunately doesn't suffer from this (at least I
think it doesn't), but in older gccs the unwinder could be split across different
objects, having e.g. parts of the unwinder in one shared library and another
part in another one, each built by different GCC version.

Guess targets which weren't supported in GCC 2.x are ok, while
__frame_state_for is in libgcc, nothing calls it, so while such changes
change the ABI, nothing likely cares.
But for older targets it is a problem.

And it is hard to catch this in the testsuite, one would either need to
hardcode the count for each target in the test, or test with mixing GCC 2.x
compiled code with current trunk.

Before the introduction of libgcc_eh.a etc., parts of the unwinder was e.g.
exported from glibc.
See e.g. https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472
for some details.

	Jakub

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Richard Earnshaw (lists)]

On 13/01/2023 22:12, Jakub Jelinek wrote:
> On Fri, Jan 13, 2023 at 09:58:26PM +0000, Richard Earnshaw (lists) wrote:
>> > I'm afraid increasing number of DWARF registers is ABI incompatible change.
>> > E.g. libgcc __frame_state_for function fills in:
>> > typedef struct frame_state
>> > {
>> >    void *cfa;
>> >    void *eh_ptr;
>> >    long cfa_offset;
>> >    long args_size;
>> >    long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>> >    unsigned short cfa_reg;
>> >    unsigned short retaddr_column;
>> >    char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>> > } frame_state;
>> > 
>> > structure, where PRE_GCC3_DWARF_FRAME_REGISTERS defaults to
>> > __LIBGCC_DWARF_FRAME_REGISTERS__, which is defined to
>> > DWARF_FRAME_REGISTERS, which defaults to FIRST_PSEUDO_REGISTER.
>> > So, changing FIRST_PSEUDO_REGISTER is an ABI change unless you arrange for
>> > PRE_GCC3_DWARF_FRAME_REGISTERS to be defined to the old value.
>> > 
>> >      Jakub
>> > 
>> 
>> So where's the red flag that warns about this?
>> 
>> I also note that Richard Sandiford made a similar type of change for AArch64
>> in r10-4195 (183bfdafc6f1f98711c5400498a7268cc1441096) and nothing was said
>> about that at the time.
>> 
>> It seems incredibly fragile to me to have some ABI based off the number of
>> machine registers.
> 
> It is.  The new unwinder fortunately doesn't suffer from this (at least I
> think it doesn't), but in older gccs the unwinder could be split across 
> different
> objects, having e.g. parts of the unwinder in one shared library and another
> part in another one, each built by different GCC version.
> 
> Guess targets which weren't supported in GCC 2.x are ok, while
> __frame_state_for is in libgcc, nothing calls it, so while such changes
> change the ABI, nothing likely cares.
> But for older targets it is a problem.
> 
> And it is hard to catch this in the testsuite, one would either need to
> hardcode the count for each target in the test, or test with mixing GCC 2.x
> compiled code with current trunk.
> 
> Before the introduction of libgcc_eh.a etc., parts of the unwinder was e.g.
> exported from glibc.
> See e.g. 
> https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472 
> <https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472>
> for some details.
> 
>          Jakub
> 

So:
1) GCC-2.* didn't support the EABI, which is all we support these days.
2) the Arm port updated FIRST_PSEUDO_REGISTER in 2019 in r10-4441 
(16155ccf588a403c033ccd7743329671bcfb27d5) and I didn't see any fallout 
from that.
3) The Arm port uses the unwinding mechanism defined by the ABI, not the 
dwarf2 based tables.

So I'm inclined to think this probably isn't going to be a problem in 
reality.

R.

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Richard Earnshaw]

On 13/01/2023 22:25, Richard Earnshaw (lists) via Gcc-patches wrote:
> On 13/01/2023 22:12, Jakub Jelinek wrote:
>> On Fri, Jan 13, 2023 at 09:58:26PM +0000, Richard Earnshaw (lists) wrote:
>>> > I'm afraid increasing number of DWARF registers is ABI incompatible 
>>> change.
>>> > E.g. libgcc __frame_state_for function fills in:
>>> > typedef struct frame_state
>>> > {
>>> >    void *cfa;
>>> >    void *eh_ptr;
>>> >    long cfa_offset;
>>> >    long args_size;
>>> >    long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>>> >    unsigned short cfa_reg;
>>> >    unsigned short retaddr_column;
>>> >    char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
>>> > } frame_state;
>>> > > structure, where PRE_GCC3_DWARF_FRAME_REGISTERS defaults to
>>> > __LIBGCC_DWARF_FRAME_REGISTERS__, which is defined to
>>> > DWARF_FRAME_REGISTERS, which defaults to FIRST_PSEUDO_REGISTER.
>>> > So, changing FIRST_PSEUDO_REGISTER is an ABI change unless you 
>>> arrange for
>>> > PRE_GCC3_DWARF_FRAME_REGISTERS to be defined to the old value.
>>> > >      Jakub
>>> >
>>> So where's the red flag that warns about this?
>>>
>>> I also note that Richard Sandiford made a similar type of change for 
>>> AArch64
>>> in r10-4195 (183bfdafc6f1f98711c5400498a7268cc1441096) and nothing 
>>> was said
>>> about that at the time.
>>>
>>> It seems incredibly fragile to me to have some ABI based off the 
>>> number of
>>> machine registers.
>>
>> It is.  The new unwinder fortunately doesn't suffer from this (at least I
>> think it doesn't), but in older gccs the unwinder could be split 
>> across different
>> objects, having e.g. parts of the unwinder in one shared library and 
>> another
>> part in another one, each built by different GCC version.
>>
>> Guess targets which weren't supported in GCC 2.x are ok, while
>> __frame_state_for is in libgcc, nothing calls it, so while such changes
>> change the ABI, nothing likely cares.
>> But for older targets it is a problem.
>>
>> And it is hard to catch this in the testsuite, one would either need to
>> hardcode the count for each target in the test, or test with mixing 
>> GCC 2.x
>> compiled code with current trunk.
>>
>> Before the introduction of libgcc_eh.a etc., parts of the unwinder was 
>> e.g.
>> exported from glibc.
>> See e.g. 
>> https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472 
>> <https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472>
>> for some details.
>>
>>          Jakub
>>
> 
> So:
> 1) GCC-2.* didn't support the EABI, which is all we support these days.
> 2) the Arm port updated FIRST_PSEUDO_REGISTER in 2019 in r10-4441 
> (16155ccf588a403c033ccd7743329671bcfb27d5) and I didn't see any fallout 
> from that.
In fact it's been changed in

  16155ccf588a
  cf16f980e527
  0be8bd1a1c89
  f1adb0a9f4d7
  9b66ebb1460d
  5a9335ef017c

All since 2003 (ie since gcc-3.0 was released).

> 3) The Arm port uses the unwinding mechanism defined by the ABI, not the 
> dwarf2 based tables.
> 
> So I'm inclined to think this probably isn't going to be a problem in 
> reality.
> 
> R.

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Jakub Jelinek]

On Fri, Jan 13, 2023 at 10:39:59PM +0000, Richard Earnshaw wrote:
> > > It is.  The new unwinder fortunately doesn't suffer from this (at least I
> > > think it doesn't), but in older gccs the unwinder could be split
> > > across different
> > > objects, having e.g. parts of the unwinder in one shared library and
> > > another
> > > part in another one, each built by different GCC version.
> > > 
> > > Guess targets which weren't supported in GCC 2.x are ok, while
> > > __frame_state_for is in libgcc, nothing calls it, so while such changes
> > > change the ABI, nothing likely cares.
> > > But for older targets it is a problem.
> > > 
> > > And it is hard to catch this in the testsuite, one would either need to
> > > hardcode the count for each target in the test, or test with mixing
> > > GCC 2.x
> > > compiled code with current trunk.
> > > 
> > > Before the introduction of libgcc_eh.a etc., parts of the unwinder
> > > was e.g.
> > > exported from glibc.
> > > See e.g.
> > > https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472
> > > <https://gcc.gnu.org/legacy-ml/gcc-patches/2001-07/threads.html#00472>
> > > for some details.
> > 
> > So:
> > 1) GCC-2.* didn't support the EABI, which is all we support these days.
> > 2) the Arm port updated FIRST_PSEUDO_REGISTER in 2019 in r10-4441
> > (16155ccf588a403c033ccd7743329671bcfb27d5) and I didn't see any fallout
> > from that.
> In fact it's been changed in
> 
>  16155ccf588a
>  cf16f980e527
>  0be8bd1a1c89
>  f1adb0a9f4d7
>  9b66ebb1460d
>  5a9335ef017c
> 
> All since 2003 (ie since gcc-3.0 was released).

You're right, t-bpabi uses unwind-arm.c rather than unwind-dw2.c.
Sorry for the false alarm.

	Jakub

Re: [GCC][PATCH 13/15, v5] arm: Add support for dwarf debug directives and pseudo hard-register for PAC feature.

[Richard Earnshaw]

On 13/01/2023 17:44, Srinath Parvathaneni via Gcc-patches wrote:
> Hello,
> 
> This patch teaches the DWARF support in gcc about RA_AUTH_CODE pseudo hard-register and also
> updates the ".save", ".cfi_register", ".cfi_offset", ".cfi_restore" directives accordingly.
> This patch also adds support to emit ".pacspval" directive when "pac ip, lr, sp" instruction
> in generated in the assembly.
> 
> RA_AUTH_CODE register number is 107 and it's dwarf register number is 143.
> 
> Applying this patch on top of PACBTI series posted here
> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599658.html and when compiling the following
> test.c with "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard
> fasynchronous-unwind-tables -g -O0 -S" command line options, the assembly output after this patch
> looks like below:
> 
> $cat test.c
> 
> void fun1(int a);
> void fun(int a,...)
> {
>    fun1(a);
> }
> 
> int main()
> {
>    fun (10);
>    return 0;
> }
> 
> $ arm-none-eabi-gcc -march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard
> -fasynchronous-unwind-tables -g -O0 -S test.s
> 
> Assembly output:
> ...
> fun:
> ...
>          .pacspval
>          pac     ip, lr, sp
>          .cfi_register 143, 12
>          push    {r3, r7, ip, lr}
>          .save {r3, r7, ra_auth_code, lr}
> ...
>          .cfi_offset 143, -24
> ...
>          .cfi_restore 143
> ...
>          aut     ip, lr, sp
>          bx      lr
> ...
> main:
> ...
>          .pacspval
>          pac     ip, lr, sp
>          .cfi_register 143, 12
>          push    {r3, r7, ip, lr}
>          .save {r3, r7, ra_auth_code, lr}
> ...
>          .cfi_offset 143, -8
> ...
>          .cfi_restore 143
> ...
>          aut     ip, lr, sp
>          bx      lr
> ...
> 
> Regression tested on arm-none-eabi target and found no regressions.
> 
> Ok for master?
> 
> Regards,
> Srinath.
> 
> 2023-01-11  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
> 
>          * config/arm/aout.h (ra_auth_code): Add entry in enum.
>          (emit_multi_reg_push): Add RA_AUTH_CODE register to
>          dwarf frame expression.
>          (arm_emit_multi_reg_pop): Restore RA_AUTH_CODE register.
>          (arm_expand_prologue): Update frame related information and reg notes
>          for pac/pacbit insn.
>          (arm_regno_class): Check for pac pseudo reigster.
>          (arm_dbx_register_number): Assign ra_auth_code register number in dwarf.
>          (arm_init_machine_status): Set pacspval_needed to zero.
>          (arm_debugger_regno): Check for PAC register.
>          (arm_unwind_emit_sequence): Print .save directive with ra_auth_code
>          register.
>          (arm_unwind_emit_set): Add entry for IP_REGNUM in switch case.
>          (arm_unwind_emit): Update REG_CFA_REGISTER case._
>          * config/arm/arm.h (FIRST_PSEUDO_REGISTER): Modify.
>          (DWARF_PAC_REGNUM): Define.
>          (IS_PAC_REGNUM): Likewise.
>          (enum reg_class): Add PAC_REG entry.
>          (machine_function): Add pacbti_needed state to structure.
>          * config/arm/arm.md (RA_AUTH_CODE): Define.
> 
> gcc/testsuite/ChangeLog:
> 
> 2023-01-11  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
> 
>          * g++.target/arm/pac-1.C: New test.
>          * gcc.target/arm/pac-15.c: Likewise.

Your attachments are still not being correctly detected.  Perhaps this 
is because of the filename you've chosen, which has no recognizable 
extension.  If you name your files <xxx>.patch (or <xxx>.diff, or even 
<xxx>.txt) then the system should automatically pick the right mime type 
for encoding.

+	  /* NOTE: Dwarf code emitter handle reg-reg copies correctly and in the
+	     following example reg-reg copy of SP to IP register is handled
+	     through .cfi_def_cfa_register directive and the .cfi_offset
+	     directive for IP register is skipped by dwarf code emitter.
+	     Example:
+		mov     ip, sp
+		.cfi_def_cfa_register 12
+		push    {fp, ip, lr, pc}
+		.cfi_offset 11, -16
+		.cfi_offset 13, -12
+		.cfi_offset 14, -8
+
+	     Where as Arm-specific .save directive reg-reg copy handling is
+	     buggy.  After the reg-reg copy, the copied registers need to be

It's not buggy (if it were you'd need to fix it :).  It just works in a 
different way to the dwarf tracker and doesn't need to handle reg->reg 
copies.  So please rephrase this.

+	     populated in .save directive register list but with the current
+	     implementation of .save directive original registers are getting
+	     populated in the register list.  So to avoid this issue for IP
+	     register when PACBTI is enabled we manually updated the .save
+	     directive register list to use "ra_auth_code" (pseduo register 143)
+	     instead of IP register as shown in following example.
+	     Example:
+		pacbti  ip, lr, sp
+		.cfi_register 143, 12
+		push    {r3, r7, ip, lr}
+		.save {r3, r7, ra_auth_code, lr}
+	  */

R.