Re: [PATCH v4 02/13] aarch64: Implement gdbarch function top_addr_empty_shadow_stack.

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

Christina Schimpe <christina.schimpe@intel.com> writes:

> @@ -3243,7 +3196,7 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>      {
>        set_gdbarch_get_shadow_stack_pointer (gdbarch,
>  					aarch64_linux_get_shadow_stack_pointer);

As I just found out and I just mentioned in my email on patch 8, ideally
this gdbarch hook should be set unconditionally, not only when
tdep->has_gcs_linux ().

But this would be a change unrelated to your patch. I can make it later
when I submit patches enabling "bt -shadow" for AArch64.

> -      tdep->fn_prev_gcspr = aarch64_linux_dwarf2_prev_gcspr;
> +      tdep->fn_prev_gcspr = dwarf2_prev_ssp;
>      }
>  }
>  
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 848cee3043c..d789b8569d2 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1908,6 +1908,26 @@ aarch64_pop_gcs_entry (regcache *regs)
>    regcache_cooked_write_unsigned (regs, tdep->gcs_reg_base, gcs_addr + 8);
>  }
>  
> +/* Implement the "top_addr_empty_shadow_stack" gdbarch method.  */
> +
> +static bool
> +aarch64_top_addr_empty_shadow_stack
> +  (const CORE_ADDR addr,
> +   const std::pair<CORE_ADDR, CORE_ADDR> range)
> +{
> +  gdb_assert (addr >= range.first);
> +
> +  /* For AArch64, addr must be strictly less than the upper address in the
> +     range, but other architectures allow it to be equal to the upper
> +     address when the stack is empty so GDB core works with those addresses
> +     and can send them our way.  */
> +  gdb_assert (addr <= range.second);
> +
> +  /* The GCS grows down, and the oldest entry isn't an address.
> +     Just the value '0'.  */
> +  return addr >= range.second - 8;
> +}
> +
>  /* Implement the "push_dummy_call" gdbarch method.  */
>  
>  static CORE_ADDR
> @@ -4783,9 +4803,13 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    /* Register a hook for converting a memory tag to a string.  */
>    set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
>  
> -  /* AArch64's shadow stack pointer is the GCSPR.  */
>    if (tdep->has_gcs ())
> -    set_gdbarch_ssp_regnum (gdbarch, tdep->gcs_reg_base);
> +    {
> +      /* AArch64's shadow stack pointer is the GCSPR.  */
> +      set_gdbarch_ssp_regnum (gdbarch, tdep->gcs_reg_base);
> +      set_gdbarch_top_addr_empty_shadow_stack
> +	(gdbarch, aarch64_top_addr_empty_shadow_stack);
> +    }

On the other hand, since this is new code added by this patch, could you
please change this to the following?

  if (tdep->has_gcs ())
    /* AArch64's shadow stack pointer is the GCSPR.  */
    set_gdbarch_ssp_regnum (gdbarch, tdep->gcs_reg_base);
  set_gdbarch_top_addr_empty_shadow_stack (gdbarch,
					   aarch64_top_addr_empty_shadow_stack);

-- 
Thiago
(he/him)