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

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

"Joos, Christina" <christina.joos@intel.com> writes:

>> -----Original Message-----
>> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Sent: Samstag, 12. September 2026 08:03
>> To: Joos, Christina <christina.joos@intel.com>
>> Cc: gdb-patches@sourceware.org; tom@tromey.com
>> Subject: Re: [PATCH v4 02/13] aarch64: Implement gdbarch function
>> top_addr_empty_shadow_stack.
>> 
>> Christina Schimpe <christina.schimpe@intel.com> writes:
>> 
>> > 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);
>
> Yes, of course.
> One nit: I think the GDB Coding Standards still require some braces here.

Ah, right. Because of the comment.

> So, my overall changes for this patch will look as follows (Luis requested some
> comment changes, too).
>
> ~~~
> $ git diff
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index d789b8569d2..94893dd4bcf 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1923,8 +1923,8 @@ aarch64_top_addr_empty_shadow_stack
>       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'.  */
> +  /* The GCS grows down, and the oldest entry is the value '0', not an
> +     address.  */
>    return addr >= range.second - 8;
>  }
>
> @@ -4807,10 +4807,13 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>      {
>        /* 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);
>      }
>
> +  /* Implementing this hook is required to allow the unwinding of
> +     the shadow stack pointer in shadow-stack.c:dwarf2_prev_ssp.  */
> +  set_gdbarch_top_addr_empty_shadow_stack
> +    (gdbarch, aarch64_top_addr_empty_shadow_stack);
> +
> ~~~
>
> I hope this is ok from your side, if not please let me know. 😊

Yes, it looks great. Thanks!

-- 
Thiago
(he/him)