Re: [PATCH v2][Binutils][GAS] arm: Use DWARF numbering convention for pseudo-register representation

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 28/07/2022 16:44, Victor Do Nascimento via Binutils wrote:
> Hi all,
> 
> This patch modifies the internal `struct reg_entry' numbering of DWARF
> pseudo-registers to match values assigned in DWARF standards (see "4.1
> DWARF register names" in [1])  and amends the unwinder .save
> directive-processing code to correctly handle mixed register-type save
> directives.
> 
> The mechanism for splitting the register list is re-written to comply
> with register ordering on push statements, being that registers are
> stored on the stack in numerical order, with the lowest numbered
> register at the lowest address [2].
> 
> Consequently, the parsing of the hypothetical directive
> 
>          .save{r4-r7, r10, ra_auth_core, lr}
> 
> has been changed such as rather than producing
> 
>          .save{r4-r7, r10}
>          .save{ra_auth_code}
>          .save{lr}
> 
> as was the case with previous implementation, now produces:
> 
>          .save{lr}
>          .save{ra_auth_code}
>          .save{r4-r7, r10}
> 
> Tested for arm-none-eabi on x86_64 and aarch64 hosts.
> 
> Best regards,
> Victor
> 
> [1] <https://github.com/ARM-software/abi-aa/blob/main/aadwarf32/aadwarf32.rst>
> [2] <https://developer.arm.com/documentation/dui0473/j/arm-and-thumb-instructions/push>
> 
> gas/Changelog:
> 
>    * config/tc-arm.c (REG_RA_AUTH_CODE): New.
>    (parse_dot_save): Likewise.
>    (parse_reg_list): Remove obsolete code.
>    (reg_names): set ra_auth_code to 143.

s/set/Set/

>    (s_arm_unwind_save): Handle core and pseudo-register lists via
>    parse_dot_save.
>    (s_arm_unwind_save_mixed): Deleted.
>    (s_arm_unwind_save_pseudo): Handle 1 register at a time.
s/1/one/
>    * testsuite/gas/arm/unwind-pacbti-m-readelf.d: Fix test.
>    * testsuite/gas/arm/unwind-pacbti-m.d: Likewise.
> ---
>   gas/config/tc-arm.c                           | 160 ++++++++++--------
>   .../gas/arm/unwind-pacbti-m-readelf.d         |   4 +-
>   gas/testsuite/gas/arm/unwind-pacbti-m.d       |   2 +-
>   3 files changed, 96 insertions(+), 70 deletions(-)
> 

Otherwise OK.

R.

> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index 2e6d175482e..f2e7f73cc2d 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -742,6 +742,7 @@ const char * const reg_expected_msgs[] =
>   #define REG_SP	13
>   #define REG_LR	14
>   #define REG_PC	15
> +#define REG_RA_AUTH_CODE 143
>   
>   /* ARM instructions take 4bytes in the object file, Thumb instructions
>      take 2:  */
> @@ -1943,21 +1944,6 @@ parse_reg_list (char ** strp, enum reg_list_els etype)
>   
>   	      reg = arm_reg_parse (&str, rt);
>   
> -	      /* Skip over allowed registers of alternative types in mixed-type
> -	         register lists.  */
> -	      if (reg == FAIL && rt == REG_TYPE_PSEUDO
> -		  && ((reg = arm_reg_parse (&str, REG_TYPE_RN)) != FAIL))
> -		{
> -		  cur_reg = reg;
> -		  continue;
> -		}
> -	      else if (reg == FAIL && rt == REG_TYPE_RN
> -		       && ((reg = arm_reg_parse (&str, REG_TYPE_PSEUDO)) != FAIL))
> -		{
> -		  cur_reg = reg;
> -		  continue;
> -		}
> -
>   	      if (etype == REGLIST_CLRM)
>   		{
>   		  if (reg == REG_SP || reg == REG_PC)
> @@ -4139,7 +4125,6 @@ s_arm_unwind_fnstart (int ignored ATTRIBUTE_UNUSED)
>     unwind.sp_restored = 0;
>   }
>   
> -
>   /* Parse a handlerdata directive.  Creates the exception handling table entry
>      for the function.  */
>   
> @@ -4297,15 +4282,19 @@ s_arm_unwind_personality (int ignored ATTRIBUTE_UNUSED)
>   /* Parse a directive saving pseudo registers.  */
>   
>   static void
> -s_arm_unwind_save_pseudo (long range)
> +s_arm_unwind_save_pseudo (int regno)
>   {
>     valueT op;
>   
> -  if (range & (1 << 12))
> +  switch (regno)
>       {
> +    case REG_RA_AUTH_CODE:
>         /* Opcode for restoring RA_AUTH_CODE.  */
>         op = 0xb4;
>         add_unwind_opcode (op, 1);
> +      break;
> +    default:
> +      as_bad (_("Unknown register no. encountered: %d\n"), regno);
>       }
>   }
>   
> @@ -4375,6 +4364,81 @@ s_arm_unwind_save_core (long range)
>       }
>   }
>   
> +/* Implement correct handling of .save lists enabling the split into
> +sublists where necessary, while preserving correct sublist ordering.  */
> +
> +static void
> +parse_dot_save (char **str_p, int prev_reg)
> +{
> +  long core_regs = 0;
> +  int reg;
> +  int in_range = 0;
> +
> +  if (**str_p == ',')
> +    *str_p += 1;
> +  if (**str_p == '}')
> +    {
> +      *str_p += 1;
> +      return;
> +    }
> +
> +  while ((reg = arm_reg_parse (str_p, REG_TYPE_RN)) != FAIL)
> +    {
> +      if (!in_range)
> +	{
> +	  if (core_regs & (1 << reg))
> +	    as_tsktsk (_("Warning: duplicated register (r%d) in register list"),
> +		       reg);
> +	  else if (reg <= prev_reg)
> +	    as_tsktsk (_("Warning: register list not in ascending order"));
> +
> +	  core_regs |= (1 << reg);
> +	  prev_reg = reg;
> +	  if (skip_past_char (str_p, '-') != FAIL)
> +	      in_range = 1;
> +	  else if (skip_past_comma (str_p) == FAIL)
> +	    first_error (_("bad register list"));
> +	}
> +      else
> +	{
> +	  int i;
> +	  if (reg <= prev_reg)
> +	    first_error (_("bad range in register list"));
> +	  for (i = prev_reg + 1; i <= reg; i++)
> +	    {
> +	      if (core_regs & (1 << i))
> +		as_tsktsk (_("Warning: duplicated register (r%d) in register list"),
> +			   i);
> +	      else
> +		core_regs |= 1 << i;
> +	    }
> +	  in_range = 0;
> +	}
> +    }
> +  if (core_regs)
> +    {
> +      /* Higher register numbers go in higher memory addresses.  When splitting
> +	 a list, right-most sublist should therefore be .saved first: use
> +	 recursion for this.  */
> +      parse_dot_save (str_p, reg);
> +      /* We're back from recursion, so emit .save insn for sublist.  */
> +      s_arm_unwind_save_core (core_regs);
> +      return;
> +    }
> +  /* Handle pseudo-regs, under assumption these are emitted singly.  */
> +  else if ((reg = arm_reg_parse (str_p, REG_TYPE_PSEUDO)) != FAIL)
> +    {
> +      /* Recurse for remainder of input.  Note: No assumption is made regarding
> +	 which register in core register set holds pseudo-register, thus it's
> +	 not considered in ordering check beyond ensuring it's not sandwiched
> +	 between 2 consecutive registers.  */
> +      parse_dot_save (str_p, prev_reg + 1);
> +      s_arm_unwind_save_pseudo (reg);
> +      return;
> +    }
> +  else
> +    as_bad (BAD_SYNTAX);
> +}
>   
>   /* Parse a directive saving FPA registers.  */
>   
> @@ -4716,39 +4780,13 @@ s_arm_unwind_save_mmxwcg (void)
>     ignore_rest_of_line ();
>   }
>   
> -/* Convert range and mask_range into a sequence of s_arm_unwind_core
> -   and s_arm_unwind_pseudo operations.  We assume that mask_range will
> -   not have consecutive bits set, or that one operation per bit is
> -   acceptable.  */
> -
> -static void
> -s_arm_unwind_save_mixed (long range, long mask_range)
> -{
> -  while (mask_range)
> -    {
> -      long mask_bit = mask_range & -mask_range;
> -      long subrange = range & (mask_bit - 1);
> -
> -      if (subrange)
> -	s_arm_unwind_save_core (subrange);
> -
> -      s_arm_unwind_save_pseudo (mask_bit);
> -      range &= ~subrange;
> -      mask_range &= ~mask_bit;
> -    }
> -
> -  if (range)
> -    s_arm_unwind_save_core (range);
> -}
> -
>   /* Parse an unwind_save directive.
>      If the argument is non-zero, this is a .vsave directive.  */
>   
>   static void
>   s_arm_unwind_save (int arch_v6)
>   {
> -  char *peek, *mask_peek;
> -  long range, mask_range;
> +  char *peek;
>     struct reg_entry *reg;
>     bool had_brace = false;
>   
> @@ -4756,7 +4794,7 @@ s_arm_unwind_save (int arch_v6)
>       as_bad (MISSING_FNSTART);
>   
>     /* Figure out what sort of save we have.  */
> -  peek = mask_peek = input_line_pointer;
> +  peek = input_line_pointer;
>   
>     if (*peek == '{')
>       {
> @@ -4788,20 +4826,13 @@ s_arm_unwind_save (int arch_v6)
>   
>       case REG_TYPE_PSEUDO:
>       case REG_TYPE_RN:
> -      mask_range = parse_reg_list (&mask_peek, REGLIST_PSEUDO);
> -      range = parse_reg_list (&input_line_pointer, REGLIST_RN);
> -
> -      if (range == FAIL || mask_range == FAIL)
> -	{
> -	  as_bad (_("expected register list"));
> -	  ignore_rest_of_line ();
> -	  return;
> -	}
> -
> -      demand_empty_rest_of_line ();
> -
> -      s_arm_unwind_save_mixed (range, mask_range);
> -      return;
> +      {
> +	if (had_brace)
> +	  input_line_pointer++;
> +	parse_dot_save (&input_line_pointer, -1);
> +	demand_empty_rest_of_line ();
> +	return;
> +      }
>   
>       case REG_TYPE_VFD:
>         if (arch_v6)
> @@ -23993,12 +24024,8 @@ static const struct reg_entry reg_names[] =
>     /* XScale accumulator registers.  */
>     REGNUM(acc,0,XSCALE), REGNUM(ACC,0,XSCALE),
>   
> -  /* DWARF ABI defines RA_AUTH_CODE to 143. It also reserves 134-142 for future
> -     expansion.  RA_AUTH_CODE here is given the value 143 % 134 to make it easy
> -     for tc_arm_regname_to_dw2regnum to translate to DWARF reg number using
> -     134 + reg_number should the range 134 to 142 be used for more pseudo regs
> -     in the future.  This also helps fit RA_AUTH_CODE into a bitmask.  */
> -  REGDEF(ra_auth_code,12,PSEUDO),
> +  /* DWARF ABI defines RA_AUTH_CODE to 143.  */
> +  REGDEF(ra_auth_code,143,PSEUDO),
>   };
>   #undef REGDEF
>   #undef REGNUM
> @@ -27905,7 +27932,6 @@ create_unwind_entry (int have_data)
>     return 0;
>   }
>   
> -
>   /* Initialize the DWARF-2 unwind information for this procedure.  */
>   
>   void
> diff --git a/gas/testsuite/gas/arm/unwind-pacbti-m-readelf.d b/gas/testsuite/gas/arm/unwind-pacbti-m-readelf.d
> index d8d647bb7f0..c40544a5a94 100644
> --- a/gas/testsuite/gas/arm/unwind-pacbti-m-readelf.d
> +++ b/gas/testsuite/gas/arm/unwind-pacbti-m-readelf.d
> @@ -10,11 +10,11 @@ Unwind section '.ARM.exidx' at offset 0x60 contains 1 entry:
>   
>   0x0 <foo>: @0x0
>     Compact model index: 1
> -  0x84 0x00 pop {r14}
>     0xb4      pop {ra_auth_code}
>     0x84 0x00 pop {r14}
> -  0xb4      pop {ra_auth_code}
>     0xa3      pop {r4, r5, r6, r7}
>     0xb4      pop {ra_auth_code}
> +  0x84 0x00 pop {r14}
> +  0xb4      pop {ra_auth_code}
>     0xa8      pop {r4, r14}
>     0xb0      finish
> diff --git a/gas/testsuite/gas/arm/unwind-pacbti-m.d b/gas/testsuite/gas/arm/unwind-pacbti-m.d
> index d178e4c0a44..b021c007b33 100644
> --- a/gas/testsuite/gas/arm/unwind-pacbti-m.d
> +++ b/gas/testsuite/gas/arm/unwind-pacbti-m.d
> @@ -8,4 +8,4 @@
>   .*:     file format.*
>   
>   Contents of section .ARM.extab:
> - 0000 (00840281 b40084b4 b0a8b4a3|81028400 b48400b4 a3b4a8b0) 00000000  .*
> + 0000 (84b40281 84b4a300 b0a8b400|8102b484 00a3b484 00b4a8b0) 00000000  .*