[PATCH][Binutils][GAS] arm: Fix DWARF pseudo-register numbering convention

[PATCH][Binutils][GAS] arm: Fix DWARF pseudo-register numbering convention

[Victor Do Nascimento]

Hi all,

This patch restores the assembler internal numbering of DWARF pseudo-
registers to the preferred modulo 134 convention (see "4.1 DWARF register
names" in [1]) and amends the unwinder .save directive processing code to
handle this.

Issue:
1.  DWARF pseudo-registers to be assigned their own reg_entry numbering
    system based on modulo 134. Pseudo-reg no. 143 thus gets assigned
    to (143 % 134) = 9.
    Not orthogonal to core-register numbering.
2.  We must have a way to to map pseudo-register numbers to
    corresponding physical register numbers and vice-versa. More
    concretely, issues first arise when invoking s_arm_unwind_save_mixed.

    Consider parsing the hypothetical directive:

        .save{r4-r7, r10, ra_auth_core, lr}

    As `.save`-ing ra_auth_code requires a special DWARF opcode, this
    directive must be split into several sub-statements, i.e.

        .save{r4-r7, r10}
        .save{ra_auth_code}
        .save{lr}

    we pass s_arm_unwind_save_mixed 2 bitmaps - one of physical register
    numbers and one of pseudo-register numbers.

        core-register bitmap:   000011110010001
        pseudo-register bitmap: 000000000100000

    Under proposed modulo 134 numbering system, this would erroneously
    produce:

        .save{r4-r7}
        .save{ra_auth_code}
        .save{r10, lr}

    the pseudo-reg bitmap must be cast to physical register space to
    know how to split .save directives correctly. So something like:

        core-register bitmap:           000011110010001
        recast pseudo-register bitmap:  000000000000100

    would give us the correct split of the .save statements.

3.  so our bitmap of pseudo-registers cast in physical register space
    tells us that bit 12 came from a pseudo-register, but with no
    guarantee (or indeed any reason to expect) that there is
    preservation of register ordering in mapping between DWARF register
    space and core-register space, in the presence of multiple
    pseudo-registers, there is no immediate way of inferring which bit
    in the DWARF bitmap mapped onto bit 12. This therefore creates the
    need for a function to convert DWARF bitmaps into core-register
    bitmaps for splitting the register list and then another for
    translating core-register numbers back to DWARF register numbers to
    correctly handle the pseudo-registers.
    This is the least invasive implementation for handling the proposed
    DWARF modulo 134 numbering system, but also seems to involve a great
    deal of mapping to and fro numbering conventions.

I've not yet thought of an alternative that is better and doesn't
require too many invasive alterations to the existing codebase (though
I'm sure it may well be out there).

Therefore, any feedback/advice/suggestions would be most welcome.

Proposed changes:
1.  Implement a function to cast a bitmap of pseudo-registers to
    corresponding physical register numbers: tc_dw2range_to_arm_range.
2.  When emitting an unwinder save instruction for a pseudo-register
    have a function that maps back physical register ranges back to the
    correct pseudo-register numbers:  tc_arm_range_to_dw2range

[1] <https://github.com/ARM-software/abi-aa/blob/main/aadwarf32/aadwarf32.rst>

Many thanks,
Victor

gas/Changelog:

    * config/tc-arm.c:
    (reg_names): Renumber RA_AUTH_CODE from 12 to 9.
    (parse_reg_list): Ensure correct range-handling.
    (s_arm_unwind_save_pseudo): Fix register number.
    (s_arm_unwind_save_mixed): Add number system interconversion.
    (tc_dw2range_to_arm_range): New.
    (tc_arm_range_to_dw2range): New.
    * config/tc-arm.h: Add new function prototypes.
---
 gas/config/tc-arm.c | 64 ++++++++++++++++++++++++++++++++++++++++-----
 gas/config/tc-arm.h |  2 ++
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
index 2e6d175482e..f66d2acca0e 100644
--- a/gas/config/tc-arm.c
+++ b/gas/config/tc-arm.c
@@ -1948,13 +1948,13 @@ parse_reg_list (char ** strp, enum reg_list_els etype)
 	      if (reg == FAIL && rt == REG_TYPE_PSEUDO
 		  && ((reg = arm_reg_parse (&str, REG_TYPE_RN)) != FAIL))
 		{
-		  cur_reg = reg;
+		  in_range = 0;
 		  continue;
 		}
 	      else if (reg == FAIL && rt == REG_TYPE_RN
 		       && ((reg = arm_reg_parse (&str, REG_TYPE_PSEUDO)) != FAIL))
 		{
-		  cur_reg = reg;
+		  in_range = 0;
 		  continue;
 		}
 
@@ -4301,7 +4301,7 @@ s_arm_unwind_save_pseudo (long range)
 {
   valueT op;
 
-  if (range & (1 << 12))
+  if (range & (1 << 9))
     {
       /* Opcode for restoring RA_AUTH_CODE.  */
       op = 0xb4;
@@ -4724,15 +4724,19 @@ s_arm_unwind_save_mmxwcg (void)
 static void
 s_arm_unwind_save_mixed (long range, long mask_range)
 {
+
+  mask_range = tc_dw2range_to_arm_range (mask_range);
+
   while (mask_range)
     {
       long mask_bit = mask_range & -mask_range;
+      long pseudo_reg = tc_arm_range_to_dw2range (mask_bit);
       long subrange = range & (mask_bit - 1);
 
       if (subrange)
-	s_arm_unwind_save_core (subrange);
+        s_arm_unwind_save_core (subrange);
 
-      s_arm_unwind_save_pseudo (mask_bit);
+      s_arm_unwind_save_pseudo (pseudo_reg);
       range &= ~subrange;
       mask_range &= ~mask_bit;
     }
@@ -23998,7 +24002,7 @@ static const struct reg_entry reg_names[] =
      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),
+  REGDEF(ra_auth_code,9,PSEUDO),
 };
 #undef REGDEF
 #undef REGNUM
@@ -27936,6 +27940,54 @@ tc_arm_regname_to_dw2regnum (char *regname)
   return FAIL;
 }
 
+/* Cast a bitmap representation of a register set from DWARF-2 modulo 134
+   pseudo-register numbering to the corresponding physical reg. numbers. */
+
+long
+tc_dw2range_to_arm_range (long pseudo_range)
+{
+  long phys_range = 0;
+  long reg = 0;
+  while (pseudo_range)
+    {
+      reg = pseudo_range & -pseudo_range;
+      switch (reg)
+        {
+          case (1 << 9):
+            phys_range |= (1 << 12);
+            break;
+          default:
+            break;
+        }
+      pseudo_range &= ~reg;
+    }
+  return phys_range;
+}
+
+
+/* from DWARF-2 modulo 134 pseudo-register numbering retrieve physical regs. */
+
+long
+tc_arm_range_to_dw2range (long phys_range)
+{
+  long pseudo_range = 0;
+  long reg = 0;
+  while (phys_range)
+    {
+      reg = phys_range & -phys_range;
+      switch (reg)
+        {
+          case (1 << 12):
+            pseudo_range |= (1 << 9);
+            break;
+          default:
+            break;
+        }
+      phys_range &= ~reg;
+    }
+  return pseudo_range;
+}
+
 #ifdef TE_PE
 void
 tc_pe_dwarf2_emit_offset (symbolS *symbol, unsigned int size)
diff --git a/gas/config/tc-arm.h b/gas/config/tc-arm.h
index d2176e03c98..8a8cb15d702 100644
--- a/gas/config/tc-arm.h
+++ b/gas/config/tc-arm.h
@@ -359,6 +359,8 @@ extern void arm_handle_align (struct frag *);
 extern bool arm_fix_adjustable (struct fix *);
 extern int arm_elf_section_type (const char *, size_t);
 extern int tc_arm_regname_to_dw2regnum (char *regname);
+extern long tc_dw2range_to_arm_range (long);
+extern long tc_arm_range_to_dw2range (long);
 extern void tc_arm_frame_initial_instructions (void);
 
 #ifdef TE_PE
-- 
2.36.1

Re: [PATCH][Binutils][GAS] arm: Fix DWARF pseudo-register numbering convention

[Richard Earnshaw]

On 22/07/2022 13:12, Victor Do Nascimento via Binutils wrote:
> Hi all,
> 
> This patch restores the assembler internal numbering of DWARF pseudo-
> registers to the preferred modulo 134 convention (see "4.1 DWARF register
> names" in [1]) and amends the unwinder .save directive processing code to
> handle this.
> 
> Issue:
> 1.  DWARF pseudo-registers to be assigned their own reg_entry numbering
>      system based on modulo 134. Pseudo-reg no. 143 thus gets assigned
>      to (143 % 134) = 9.
>      Not orthogonal to core-register numbering.
> 2.  We must have a way to to map pseudo-register numbers to
>      corresponding physical register numbers and vice-versa. More
>      concretely, issues first arise when invoking s_arm_unwind_save_mixed.
> 
>      Consider parsing the hypothetical directive:
> 
>          .save{r4-r7, r10, ra_auth_core, lr}
> 
>      As `.save`-ing ra_auth_code requires a special DWARF opcode, this
>      directive must be split into several sub-statements, i.e.
> 
>          .save{r4-r7, r10}
>          .save{ra_auth_code}
>          .save{lr}
> 
.save directives have a side effect of adjusting the stack calculations, 
so I think we need to emit these in the opposite order, with the items 
at higher addresses first (stack grows downwards).  Ergo:

	.save{lr}
	.save{ra_auth_code}
	.save{r4-r7, r10}

>      we pass s_arm_unwind_save_mixed 2 bitmaps - one of physical register
>      numbers and one of pseudo-register numbers.
> 
>          core-register bitmap:   000011110010001
>          pseudo-register bitmap: 000000000100000
> 
>      Under proposed modulo 134 numbering system, this would erroneously
>      produce:
> 
>          .save{r4-r7}
>          .save{ra_auth_code}
>          .save{r10, lr}
> 
>      the pseudo-reg bitmap must be cast to physical register space to
>      know how to split .save directives correctly. So something like:
> 
>          core-register bitmap:           000011110010001
>          recast pseudo-register bitmap:  000000000000100
> 
>      would give us the correct split of the .save statements.
> 
> 3.  so our bitmap of pseudo-registers cast in physical register space
>      tells us that bit 12 came from a pseudo-register, but with no
>      guarantee (or indeed any reason to expect) that there is
>      preservation of register ordering in mapping between DWARF register
>      space and core-register space, in the presence of multiple
>      pseudo-registers, there is no immediate way of inferring which bit
>      in the DWARF bitmap mapped onto bit 12. This therefore creates the
>      need for a function to convert DWARF bitmaps into core-register
>      bitmaps for splitting the register list and then another for
>      translating core-register numbers back to DWARF register numbers to
>      correctly handle the pseudo-registers.
>      This is the least invasive implementation for handling the proposed
>      DWARF modulo 134 numbering system, but also seems to involve a great
>      deal of mapping to and fro numbering conventions.
> 
> I've not yet thought of an alternative that is better and doesn't
> require too many invasive alterations to the existing codebase (though
> I'm sure it may well be out there).
> 

Your problem comes from trying to mash together two bitmaps describing 
distinct elements and assuming that the ra_code will always be copied to 
IP (it probably will be, but it's dangerous to assume that).  Don't do 
that.  Instead, I think you need to build the component .save objects as 
you break down the input string; but since you parse the string 
left-to-right and want the push operations to describe right-to-left, 
you'll probably want some form of recursion.

As a rough outline, I'd suggest your parser does something like

parse_dot_save(char **str_p)
{
   core_regs = 0;

   while ((reg = reg_parse (CORE, str_p) != INVALID)
     core_regs |= reg;
   if (core_regs)
     {
       // recurse for remainder of input
       parse_dot_save (str_p);
       // Now emit the regs we parsed.
       emit_dot_save (core_regs);
       return;
     }
   // Pseudo regs are always emitted singly.
   regno = reg_parse (PSEUDOS, str_p);
   if (regno != INVALID)
     {
       // recurse for remainder of input
       parse_dot_save (str_p);
       emit_dot_save_pseudo (regno);
       return;
     }
   // error case
}

error, comma and end-of-list handling will complicate the above 
slightly, but that should be the general gist of it.  If that doesn't 
work, you'll need to build a table of actions during the parse and then 
emit the actions in reverse order once you've finished.  Try not to 
assume there will be exactly one pseudo in the list (we might add more 
at a later date) or that there will be exactly two sublists of core 
registers (there might be zero, one, two, or perhaps even many one day).

Furthermore, I'd suggest you don't try to convert the pseudos into a 
bitmask, there's no real value, and thus you might as well give the 
register its natural dwarf number internally.

R.

> Therefore, any feedback/advice/suggestions would be most welcome.
> 
> Proposed changes:
> 1.  Implement a function to cast a bitmap of pseudo-registers to
>      corresponding physical register numbers: tc_dw2range_to_arm_range.
> 2.  When emitting an unwinder save instruction for a pseudo-register
>      have a function that maps back physical register ranges back to the
>      correct pseudo-register numbers:  tc_arm_range_to_dw2range
> 
> [1] <https://github.com/ARM-software/abi-aa/blob/main/aadwarf32/aadwarf32.rst>
> 
> Many thanks,
> Victor
> 
> gas/Changelog:
> 
>      * config/tc-arm.c:
>      (reg_names): Renumber RA_AUTH_CODE from 12 to 9.
>      (parse_reg_list): Ensure correct range-handling.
>      (s_arm_unwind_save_pseudo): Fix register number.
>      (s_arm_unwind_save_mixed): Add number system interconversion.
>      (tc_dw2range_to_arm_range): New.
>      (tc_arm_range_to_dw2range): New.
>      * config/tc-arm.h: Add new function prototypes.
> ---
>   gas/config/tc-arm.c | 64 ++++++++++++++++++++++++++++++++++++++++-----
>   gas/config/tc-arm.h |  2 ++
>   2 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index 2e6d175482e..f66d2acca0e 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -1948,13 +1948,13 @@ parse_reg_list (char ** strp, enum reg_list_els etype)
>   	      if (reg == FAIL && rt == REG_TYPE_PSEUDO
>   		  && ((reg = arm_reg_parse (&str, REG_TYPE_RN)) != FAIL))
>   		{
> -		  cur_reg = reg;
> +		  in_range = 0;
>   		  continue;
>   		}
>   	      else if (reg == FAIL && rt == REG_TYPE_RN
>   		       && ((reg = arm_reg_parse (&str, REG_TYPE_PSEUDO)) != FAIL))
>   		{
> -		  cur_reg = reg;
> +		  in_range = 0;
>   		  continue;
>   		}
>   
> @@ -4301,7 +4301,7 @@ s_arm_unwind_save_pseudo (long range)
>   {
>     valueT op;
>   
> -  if (range & (1 << 12))
> +  if (range & (1 << 9))
>       {
>         /* Opcode for restoring RA_AUTH_CODE.  */
>         op = 0xb4;
> @@ -4724,15 +4724,19 @@ s_arm_unwind_save_mmxwcg (void)
>   static void
>   s_arm_unwind_save_mixed (long range, long mask_range)
>   {
> +
> +  mask_range = tc_dw2range_to_arm_range (mask_range);
> +
>     while (mask_range)
>       {
>         long mask_bit = mask_range & -mask_range;
> +      long pseudo_reg = tc_arm_range_to_dw2range (mask_bit);
>         long subrange = range & (mask_bit - 1);
>   
>         if (subrange)
> -	s_arm_unwind_save_core (subrange);
> +        s_arm_unwind_save_core (subrange);
>   
> -      s_arm_unwind_save_pseudo (mask_bit);
> +      s_arm_unwind_save_pseudo (pseudo_reg);
>         range &= ~subrange;
>         mask_range &= ~mask_bit;
>       }
> @@ -23998,7 +24002,7 @@ static const struct reg_entry reg_names[] =
>        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),
> +  REGDEF(ra_auth_code,9,PSEUDO),
>   };
>   #undef REGDEF
>   #undef REGNUM
> @@ -27936,6 +27940,54 @@ tc_arm_regname_to_dw2regnum (char *regname)
>     return FAIL;
>   }
>   
> +/* Cast a bitmap representation of a register set from DWARF-2 modulo 134
> +   pseudo-register numbering to the corresponding physical reg. numbers. */
> +
> +long
> +tc_dw2range_to_arm_range (long pseudo_range)
> +{
> +  long phys_range = 0;
> +  long reg = 0;
> +  while (pseudo_range)
> +    {
> +      reg = pseudo_range & -pseudo_range;
> +      switch (reg)
> +        {
> +          case (1 << 9):
> +            phys_range |= (1 << 12);
> +            break;
> +          default:
> +            break;
> +        }
> +      pseudo_range &= ~reg;
> +    }
> +  return phys_range;
> +}
> +
> +
> +/* from DWARF-2 modulo 134 pseudo-register numbering retrieve physical regs. */
> +
> +long
> +tc_arm_range_to_dw2range (long phys_range)
> +{
> +  long pseudo_range = 0;
> +  long reg = 0;
> +  while (phys_range)
> +    {
> +      reg = phys_range & -phys_range;
> +      switch (reg)
> +        {
> +          case (1 << 12):
> +            pseudo_range |= (1 << 9);
> +            break;
> +          default:
> +            break;
> +        }
> +      phys_range &= ~reg;
> +    }
> +  return pseudo_range;
> +}
> +
>   #ifdef TE_PE
>   void
>   tc_pe_dwarf2_emit_offset (symbolS *symbol, unsigned int size)
> diff --git a/gas/config/tc-arm.h b/gas/config/tc-arm.h
> index d2176e03c98..8a8cb15d702 100644
> --- a/gas/config/tc-arm.h
> +++ b/gas/config/tc-arm.h
> @@ -359,6 +359,8 @@ extern void arm_handle_align (struct frag *);
>   extern bool arm_fix_adjustable (struct fix *);
>   extern int arm_elf_section_type (const char *, size_t);
>   extern int tc_arm_regname_to_dw2regnum (char *regname);
> +extern long tc_dw2range_to_arm_range (long);
> +extern long tc_arm_range_to_dw2range (long);
>   extern void tc_arm_frame_initial_instructions (void);
>   
>   #ifdef TE_PE