diff mbox series

[v1,4/4] dwarf2: store the RA state in CFI row

Message ID 20240806140744.1082602-5-matthieu.longo@arm.com
State New
Headers
Series dwarf2: add hooks for architecture-specific CFIs |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Build failed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Matthieu Longo Aug. 6, 2024, 2:07 p.m. UTC 
  On AArch64, the RA state informs the unwinder whether the return address
is mangled and how, or not. This information is encoded in a boolean in
the CFI row. This binary approach prevents from expressing more complex
configuration, as it is the case with PAuth_LR introduced in Armv9.5-A.

This patch addresses this limitation by replacing the boolean by an enum.

gcc/ChangeLog:

        * dwarf2cfi.cc
        (struct dw_cfi_row): Declare a new enum type to replace ra_mangled.
        (cfi_row_equal_p): Use ra_state instead of ra_mangled.
        (dwarf2out_frame_debug_cfa_negate_ra_state): Same.
        (change_cfi_row): Same.
---
 gcc/dwarf2cfi.cc | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Jakub Jelinek Aug. 6, 2024, 2:28 p.m. UTC | #1 
On Tue, Aug 06, 2024 at 03:07:44PM +0100, Matthieu Longo wrote:
> On AArch64, the RA state informs the unwinder whether the return address
> is mangled and how, or not. This information is encoded in a boolean in
> the CFI row. This binary approach prevents from expressing more complex
> configuration, as it is the case with PAuth_LR introduced in Armv9.5-A.
> 
> This patch addresses this limitation by replacing the boolean by an enum.

Formatting nits.
> 
> gcc/ChangeLog:
> 
>         * dwarf2cfi.cc
>         (struct dw_cfi_row): Declare a new enum type to replace ra_mangled.
>         (cfi_row_equal_p): Use ra_state instead of ra_mangled.
>         (dwarf2out_frame_debug_cfa_negate_ra_state): Same.
>         (change_cfi_row): Same.
> ---
>  gcc/dwarf2cfi.cc | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/dwarf2cfi.cc b/gcc/dwarf2cfi.cc
> index 6c80e0b17bd..023f61fb712 100644
> --- a/gcc/dwarf2cfi.cc
> +++ b/gcc/dwarf2cfi.cc
> @@ -57,6 +57,15 @@ along with GCC; see the file COPYING3.  If not see
>  #define DEFAULT_INCOMING_FRAME_SP_OFFSET INCOMING_FRAME_SP_OFFSET
>  #endif
>  
> +
> +/* Signing method used for return address authentication.
> +   (AArch64 extension)*/

Dot and two spaces missing before */

> +typedef enum
> +{
> +  RA_no_signing = 0x0,
> +  RA_signing_SP = 0x1,
> +} RA_signing_method_t;

I think it should be ra_* and *_sp
> @@ -1556,8 +1565,11 @@ dwarf2out_frame_debug_cfa_negate_ra_state (void)
>  {
>    dw_cfi_ref cfi = new_cfi ();
>    cfi->dw_cfi_opc = DW_CFA_AARCH64_negate_ra_state;
> +  cur_row->ra_state =
> +	  (cur_row->ra_state == RA_no_signing)
> +	  ? RA_signing_SP
> +	  : RA_no_signing;

This is wrongly formatted.  = shouldn't be at the end of line.
Better
  cur_row->ra_state
    = (cur_row->ra_state == ra_no_signing
       ? ra_signing_sp : ra_no_signing);

	Jakub
Richard Sandiford Aug. 13, 2024, 3:55 p.m. UTC | #2 
Matthieu Longo <matthieu.longo@arm.com> writes:
> On AArch64, the RA state informs the unwinder whether the return address
> is mangled and how, or not. This information is encoded in a boolean in
> the CFI row. This binary approach prevents from expressing more complex
> configuration, as it is the case with PAuth_LR introduced in Armv9.5-A.
>
> This patch addresses this limitation by replacing the boolean by an enum.
>
> gcc/ChangeLog:
>
>         * dwarf2cfi.cc
>         (struct dw_cfi_row): Declare a new enum type to replace ra_mangled.
>         (cfi_row_equal_p): Use ra_state instead of ra_mangled.
>         (dwarf2out_frame_debug_cfa_negate_ra_state): Same.
>         (change_cfi_row): Same.

OK with the changes that Jakub pointed out.

Thanks,
Richard

> ---
>  gcc/dwarf2cfi.cc | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/dwarf2cfi.cc b/gcc/dwarf2cfi.cc
> index 6c80e0b17bd..023f61fb712 100644
> --- a/gcc/dwarf2cfi.cc
> +++ b/gcc/dwarf2cfi.cc
> @@ -57,6 +57,15 @@ along with GCC; see the file COPYING3.  If not see
>  #define DEFAULT_INCOMING_FRAME_SP_OFFSET INCOMING_FRAME_SP_OFFSET
>  #endif
>  
> +
> +/* Signing method used for return address authentication.
> +   (AArch64 extension)*/
> +typedef enum
> +{
> +  RA_no_signing = 0x0,
> +  RA_signing_SP = 0x1,
> +} RA_signing_method_t;
> +
>  /* A collected description of an entire row of the abstract CFI table.  */
>  struct GTY(()) dw_cfi_row
>  {
> @@ -74,8 +83,8 @@ struct GTY(()) dw_cfi_row
>    bool window_save;
>  
>    /* Aarch64 extension for DW_CFA_AARCH64_negate_ra_state.
> -     True if the return address is in a mangled state.  */
> -  bool ra_mangled;
> +     Enum which stores the return address state.  */
> +  RA_signing_method_t ra_state;
>  };
>  
>  /* The caller's ORIG_REG is saved in SAVED_IN_REG.  */
> @@ -859,7 +868,7 @@ cfi_row_equal_p (dw_cfi_row *a, dw_cfi_row *b)
>    if (a->window_save != b->window_save)
>      return false;
>  
> -  if (a->ra_mangled != b->ra_mangled)
> +  if (a->ra_state != b->ra_state)
>      return false;
>  
>    return true;
> @@ -1556,8 +1565,11 @@ dwarf2out_frame_debug_cfa_negate_ra_state (void)
>  {
>    dw_cfi_ref cfi = new_cfi ();
>    cfi->dw_cfi_opc = DW_CFA_AARCH64_negate_ra_state;
> +  cur_row->ra_state =
> +	  (cur_row->ra_state == RA_no_signing)
> +	  ? RA_signing_SP
> +	  : RA_no_signing;
>    add_cfi (cfi);
> -  cur_row->ra_mangled = !cur_row->ra_mangled;
>  }
>  
>  /* Record call frame debugging information for an expression EXPR,
> @@ -2414,12 +2426,12 @@ change_cfi_row (dw_cfi_row *old_row, dw_cfi_row *new_row)
>      {
>        dw_cfi_ref cfi = new_cfi ();
>  
> -      gcc_assert (!old_row->ra_mangled && !new_row->ra_mangled);
> +      gcc_assert (!old_row->ra_state && !new_row->ra_state);
>        cfi->dw_cfi_opc = DW_CFA_GNU_window_save;
>        add_cfi (cfi);
>      }
>  
> -  if (old_row->ra_mangled != new_row->ra_mangled)
> +  if (old_row->ra_state != new_row->ra_state)
>      {
>        dw_cfi_ref cfi = new_cfi ();
Matthieu Longo Sept. 17, 2024, 1:39 p.m. UTC | #3 
On 2024-08-06 15:28, Jakub Jelinek wrote:
> On Tue, Aug 06, 2024 at 03:07:44PM +0100, Matthieu Longo wrote:
>> On AArch64, the RA state informs the unwinder whether the return address
>> is mangled and how, or not. This information is encoded in a boolean in
>> the CFI row. This binary approach prevents from expressing more complex
>> configuration, as it is the case with PAuth_LR introduced in Armv9.5-A.
>>
>> This patch addresses this limitation by replacing the boolean by an enum.
> 
> Formatting nits.
>>
>> gcc/ChangeLog:
>>
>>          * dwarf2cfi.cc
>>          (struct dw_cfi_row): Declare a new enum type to replace ra_mangled.
>>          (cfi_row_equal_p): Use ra_state instead of ra_mangled.
>>          (dwarf2out_frame_debug_cfa_negate_ra_state): Same.
>>          (change_cfi_row): Same.
>> ---
>>   gcc/dwarf2cfi.cc | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/dwarf2cfi.cc b/gcc/dwarf2cfi.cc
>> index 6c80e0b17bd..023f61fb712 100644
>> --- a/gcc/dwarf2cfi.cc
>> +++ b/gcc/dwarf2cfi.cc
>> @@ -57,6 +57,15 @@ along with GCC; see the file COPYING3.  If not see
>>   #define DEFAULT_INCOMING_FRAME_SP_OFFSET INCOMING_FRAME_SP_OFFSET
>>   #endif
>>   
>> +
>> +/* Signing method used for return address authentication.
>> +   (AArch64 extension)*/
> 
> Dot and two spaces missing before */
> 
>> +typedef enum
>> +{
>> +  RA_no_signing = 0x0,
>> +  RA_signing_SP = 0x1,
>> +} RA_signing_method_t;
> 
> I think it should be ra_* and *_sp
>> @@ -1556,8 +1565,11 @@ dwarf2out_frame_debug_cfa_negate_ra_state (void)
>>   {
>>     dw_cfi_ref cfi = new_cfi ();
>>     cfi->dw_cfi_opc = DW_CFA_AARCH64_negate_ra_state;
>> +  cur_row->ra_state =
>> +	  (cur_row->ra_state == RA_no_signing)
>> +	  ? RA_signing_SP
>> +	  : RA_no_signing;
> 
> This is wrongly formatted.  = shouldn't be at the end of line.
> Better
>    cur_row->ra_state
>      = (cur_row->ra_state == ra_no_signing
>         ? ra_signing_sp : ra_no_signing);
> 
> 	Jakub
> 

The comments are addressed in the next revision.

Thanks,
Matthieu.
diff mbox series

Patch

diff --git a/gcc/dwarf2cfi.cc b/gcc/dwarf2cfi.cc
index 6c80e0b17bd..023f61fb712 100644
--- a/gcc/dwarf2cfi.cc
+++ b/gcc/dwarf2cfi.cc
@@ -57,6 +57,15 @@  along with GCC; see the file COPYING3.  If not see
 #define DEFAULT_INCOMING_FRAME_SP_OFFSET INCOMING_FRAME_SP_OFFSET
 #endif
 
+
+/* Signing method used for return address authentication.
+   (AArch64 extension)*/
+typedef enum
+{
+  RA_no_signing = 0x0,
+  RA_signing_SP = 0x1,
+} RA_signing_method_t;
+
 /* A collected description of an entire row of the abstract CFI table.  */
 struct GTY(()) dw_cfi_row
 {
@@ -74,8 +83,8 @@  struct GTY(()) dw_cfi_row
   bool window_save;
 
   /* Aarch64 extension for DW_CFA_AARCH64_negate_ra_state.
-     True if the return address is in a mangled state.  */
-  bool ra_mangled;
+     Enum which stores the return address state.  */
+  RA_signing_method_t ra_state;
 };
 
 /* The caller's ORIG_REG is saved in SAVED_IN_REG.  */
@@ -859,7 +868,7 @@  cfi_row_equal_p (dw_cfi_row *a, dw_cfi_row *b)
   if (a->window_save != b->window_save)
     return false;
 
-  if (a->ra_mangled != b->ra_mangled)
+  if (a->ra_state != b->ra_state)
     return false;
 
   return true;
@@ -1556,8 +1565,11 @@  dwarf2out_frame_debug_cfa_negate_ra_state (void)
 {
   dw_cfi_ref cfi = new_cfi ();
   cfi->dw_cfi_opc = DW_CFA_AARCH64_negate_ra_state;
+  cur_row->ra_state =
+	  (cur_row->ra_state == RA_no_signing)
+	  ? RA_signing_SP
+	  : RA_no_signing;
   add_cfi (cfi);
-  cur_row->ra_mangled = !cur_row->ra_mangled;
 }
 
 /* Record call frame debugging information for an expression EXPR,
@@ -2414,12 +2426,12 @@  change_cfi_row (dw_cfi_row *old_row, dw_cfi_row *new_row)
     {
       dw_cfi_ref cfi = new_cfi ();
 
-      gcc_assert (!old_row->ra_mangled && !new_row->ra_mangled);
+      gcc_assert (!old_row->ra_state && !new_row->ra_state);
       cfi->dw_cfi_opc = DW_CFA_GNU_window_save;
       add_cfi (cfi);
     }
 
-  if (old_row->ra_mangled != new_row->ra_mangled)
+  if (old_row->ra_state != new_row->ra_state)
     {
       dw_cfi_ref cfi = new_cfi ();