Re: [PATCH v2 2/7] gdb/aarch64: Add custom printing for POR_EL0

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

<srinath.parvathaneni@arm.com> writes:

> From: Srinath Parvathaneni <srinath.parvathaneni@arm.com>
>
> Add custom printing support for the POR_EL0 register when displayed
> using `info registers` or `info all-registers`.
>
> The register value is decoded into a human-readable "rwx"
> representation for each nibble in the POR_EL0 register, making the
> POE permissions easier to interpret.
>
> Example:
> (gdb) set $por_el0=0xffffffff77777777
> (gdb) info register por_el0
> por_el0        0xffffffff77777777  [P15=??? P14=??? P13=??? P12=??? P11=??? P10=??? P9=??? P8=??? P7=rwx P6=rwx P5=rwx P4=rwx P3=rwx P2=rwx P1=rwx P0=rwx ]

Nice.

> ---
>  gdb/aarch64-tdep.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index f1e1a9c7a19..3ef4e696c7c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3135,6 +3135,86 @@ aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
>  		  p_regnum);
>  }
>  
> +/* Convert a POR_EL0 Perm<m> overlay permission encoding into rwx-style string.
> +   For example:
> +     0b0011 -> "r-x" (3)
> +     0b0101 -> "rw-" (5)
> +     0b0111 -> "rwx" (7)
> +     0b0000 -> "---" (0)
> +   Reserved encodings (0b1xxx) are returned as "???".  */
> +
> +static const char*
> +aarch64_perm_overlay_decode (unsigned int perm)
> +{
> +  switch (perm)
> +   {
> +     case 0: return "---";
> +     case 1: return "r--";
> +     case 2: return "--x";
> +     case 3: return "r-x";
> +     case 4: return "-w-";
> +     case 5: return "rw-";
> +     case 6: return "-wx";
> +     case 7: return "rwx";
> +     default: return "???";
> +   }
> +}
> +
> +/* Display POE reigster POR_EL0 in the following format for the 'info registers'

Typo: reigster

> +   and 'info all-registers' commands:
> +   <register-name> <hex-value> [<decoded per-protection-key permissions>]  */
> +
> +static void
> +aarch64_print_poe_register_info (struct ui_file *file,
> +				 int regnum,
> +				 const char *name,
> +				 const frame_info_ptr &frame)
> +{
> +  value *val = value_of_register (regnum, get_next_frame_sentinel_okay (frame));
> +  ULONGEST por_el0 = (ULONGEST) value_as_long (val);
> +  gdb_printf (file, "%-14s 0x%s  [", name, phex_nz (por_el0, 8));
> +
> +  for (int i = 15; i >= 0; --i)
> +    {
> +      unsigned int perm = (por_el0 >> (i * 4)) & 0xf;
> +      gdb_printf (file, "P%d=%s ", i, aarch64_perm_overlay_decode (perm));
> +    }
> +
> +  gdb_puts ("]\n", file);
> +}
> +
> +/* For 'info registers' and 'info all-registers', print POR_EL0 using the
> +   custom POE register printer and all other registers using the default
> +   register printer.  */
> +
> +static void
> +aarch64_print_registers_info (struct gdbarch *gdbarch,
> +			      struct ui_file *file,
> +			      const frame_info_ptr &frame,
> +			      int regnum,
> +			      bool print_all)
> +{
> +  const int numregs = gdbarch_num_cooked_regs (gdbarch);
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +
> +  if (regnum == -1)
> +    {
> +      for (int i = 0; i < numregs; i++)
> +	{
> +	  if (i == tdep->poe_regnum)
> +	    {
> +	      aarch64_print_poe_register_info (file, i, "por_el0", frame);
> +	      continue;
> +	    }
> +	  default_print_registers_info (gdbarch, file, frame,  i, print_all);
> +	}
> +    }
> +  else if (regnum == tdep->poe_regnum)
> +    aarch64_print_poe_register_info (file, regnum, "por_el0", frame);
> +  else
> +    default_print_registers_info (gdbarch, file, frame, regnum, print_all);
> +}
> +
>  /* Implement the "pseudo_register_reggroup_p" tdesc_arch_data method.  */
>  
>  static bool

Instead of adding this gdbarch hook, have you considered using enum
target types¹ in the feature XML instead? E.g:

<feature name="org.gnu.gdb.aarch64.poe">
  <enum id="perms_enum" size="4">
    <evalue name="---" value="0"/>
    <evalue name="r--" value="1"/>
    <evalue name="r-x" value="2"/>
    <evalue name="r-x" value="3"/>
    <evalue name="-w-" value="4"/>
    <evalue name="rw-" value="5"/>
    <evalue name="-wx" value="6"/>
    <evalue name="rwx" value="7"/>
    <!-- Maybe add the other 8 values as "???"? -->
  </enum>
  <flags id="por_flags" size="8">
    <field name="P0" start="0" end="3" type="perms_enum"/>
    <field name="P1" start="4" end="7" type="perms_enum"/>
    <field name="P2" start="8" end="11" type="perms_enum"/>
    <field name="P3" start="12" end="15" type="perms_enum"/>
    <!-- And so on until P15. -->
  </flags>

  <reg name="por_el0" bitsize="64" type="por_flags" group="system"/>
</feature>

Note that I haven't tested the above and I'm not sure it would actually
work (or work well).

I see two advantages:

- It works in the "print $por_el0" case too, not only "info register
  $por_el0". Or does this patch also work with "print $por_el0" as well?

- It's less code.

> @@ -4141,6 +4221,10 @@ aarch64_features_from_target_desc (const struct target_desc *tdesc)
>    features.fpmr = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpmr")
>  		   != nullptr);
>  
> +  /* Check for POE feature.  */
> +  features.poe = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.poe")
> +		  != nullptr);
> +
>    return features;
>  }
>  
> @@ -4774,6 +4858,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    tdep->gcs_reg_base = first_gcs_regnum;
>    tdep->gcs_linux_reg_base = first_gcs_linux_regnum;
>    tdep->fpmr_regnum = fpmr_regnum;
> +  tdep->poe_regnum = poe_regnum;
>  
>    /* Set the SME register set details.  The pseudo-registers will be adjusted
>       later.  */

This change and the one above it in aarch64_features_from_target_desc
should be in the previous patch instead.

-- 
Thiago
(he/him)

¹ https://sourceware.org/gdb/current/onlinedocs/gdb.html/Enum-Target-Types.html#Enum-Target-Types