RE: [PATCH 1/9] gdb: Generalize handling of the shadow stack pointer.

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Thiago,

Thanks a lot for this detailed review!
I applied most of your comments, please find my feedback to your review below.

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Friday, 31 October 2025 02:32
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/9] gdb: Generalize handling of the shadow stack
> pointer.
> 
> Christina Schimpe <christina.schimpe@intel.com> writes:
> 
> > Until now, handling of the shadow stack pointer has been done in the
> > target dependent implementations of the gdbarch hook
> > 'gdbarch_shadow_stack_push'.  Also amd64 and aarch64 linux specific
> > unwinders for the shadow stack pointer are implemented.
> > In a following commit a subcommmand "backtrace shadow" of the ordinary
> 
> Too mmany m's in "subcommand".

Fixed.

> 
> > backtrace command will be added to print the shadow stack backtrace.
> > This requires more target-independent logic to handle the shadow stack
> > pointer.  To avoid that we duplicate the logic, add new source and
> > header files "shadow-stack" for the implementation of
> > shadow_stack_push and shadow stack pointer unwinding in a target-
> independent way.
> 
> <snip>
> 
> > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index
> > 500ac77d75a..95af82c2632 100644
> > --- a/gdb/aarch64-tdep.c
> > +++ b/gdb/aarch64-tdep.c
> 
> As mentioned before, this change is also needed in aarch64-tdep.c to make
> this patch series work for AArch64:
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index
> 95af82c26327..9e866fc319d4 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -4780,6 +4780,10 @@ 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);
> 
> +  if (tdep->has_gcs ())
> +    /* AArch64's shadow stack pointer is the GCSPR.  */
> +    set_gdbarch_ssp_regnum (gdbarch, tdep->gcs_reg_base);
> +

Added.

>    /* ABI */
>    set_gdbarch_short_bit (gdbarch, 16);
>    set_gdbarch_int_bit (gdbarch, 32);
> 
> > -static value *
> > -amd64_linux_dwarf2_prev_ssp (const frame_info_ptr &this_frame,
> > -			     void **this_cache, int regnum)
> > -{
> > -  value *v = frame_unwind_got_register (this_frame, regnum, regnum);
> > -  gdb_assert (v != nullptr);
> > -
> > -  gdbarch *gdbarch = get_frame_arch (this_frame);
> > -
> > -  if (v->entirely_available () && !v->optimized_out ())
> > -    {
> > -      int size = register_size (gdbarch, regnum);
> > -      bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > -      CORE_ADDR ssp = extract_unsigned_integer (v->contents_all ().data (),
> > -						size, byte_order);
> > -
> > -      /* Using /proc/PID/smaps we can only check if the current shadow
> > -	 stack pointer SSP points to shadow stack memory.  Only if this is
> > -	 the case a valid previous shadow stack pointer can be
> > -	 calculated.  */
> > -      std::pair<CORE_ADDR, CORE_ADDR> range;
> > -      if (linux_address_in_shadow_stack_mem_range (ssp, &range))
> > -	{
> > -	  /* The shadow stack grows downwards.  To compute the previous
> > -	     shadow stack pointer, we need to increment SSP.  */
> > -	  CORE_ADDR new_ssp
> > -	    = ssp + amd64_linux_shadow_stack_element_size_aligned
> (gdbarch);
> > -
> > -	  /* There can be scenarios where we have a shadow stack pointer
> > -	     but the shadow stack is empty, as no call instruction has
> > -	     been executed yet.  If NEW_SSP points to the end of or before
> > -	     (<=) the current shadow stack memory range we consider
> > -	     NEW_SSP as valid (but empty).  */
> > -	  if (new_ssp <= range.second)
> 
> IIUC, the '<=' comparison above isn't preserved by this patch. This function is
> replaced by dwarf2_prev_ssp, which uses
> gdbarch_address_in_shadow_stack_memory_range for this if condition,
> whose comparison in find_addr_mem_range is:
> 
>       bool addr_in_mem_range
>         = (addr >= map.start_address && addr < map.end_address);
> 
> Is this intended?

Arg, thanks for catching that!

I think I missed that because I introduced a typo/bug in the call

	      || gdbarch_address_in_shadow_stack_memory_range (gdbarch,
							       ssp,
							       &range))

which made the unwinding work properly in case of amd64.
However, the proper fix should be to pass new_ssp to gdbarch_address_in_shadow_stack_memory_range
instead, and to implement gdbarch_top_addr_empty_shadow_stack also for amd64.

Does that make sense?

> > -	    return frame_unwind_got_address (this_frame, regnum, new_ssp);
> > -	}
> > -    }
> > -
> > -  /* Return a value which is marked as unavailable in case we could not
> > -     calculate a valid previous shadow stack pointer.  */
> > -  value *retval
> > -    = value::allocate_register (get_next_frame_sentinel_okay (this_frame),
> > -				regnum, register_type (gdbarch, regnum));
> > -  retval->mark_bytes_unavailable (0, retval->type ()->length ());
> > -  return retval;
> > -}
> 
> <snip>
> 
> > diff --git a/gdb/shadow-stack.c b/gdb/shadow-stack.c new file mode
> > 100644 index 00000000000..d153d5fc846
> > --- /dev/null
> > +++ b/gdb/shadow-stack.c
> > @@ -0,0 +1,167 @@
> > +/* Manage a shadow stack pointer for GDB, the GNU debugger.
> > +
> > +   Copyright (C) 2024-2025 Free Software Foundation, Inc.
> 
> Should this really start at 2024? According to Andrew Burgess¹:

Yes, 2024 is correct in this case since our gdb-oneapi supported bt shadow since 2024. 

> > The start date should be when the patches were first posted to the
> > list, or otherwise made publicly available (e.g. Intel specific GDB release?).
> > The end date should be updated to 2025.
> 
> Same question for other files created by this patch series.
> 
> > +   This file is part of GDB.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see
> > + <http://www.gnu.org/licenses/>.  */
> > +
> > +#include "defs.h"
> > +#include "arch-utils.h"
> > +#include "gdbcore.h"
> > +#include "extract-store-integer.h"
> > +#include "frame.h"
> > +#include "frame-unwind.h"
> > +#include "shadow-stack.h"
> > +
> > +enum class ssp_update_direction
> > +{
> > +  /* Update ssp towards the bottom of the shadow stack.  */
> > +  bottom = 0,
> > +
> > +  /* Update ssp towards the top of the shadow stack.  */
> > +  top
> > +};
> 
> I find the bottom/top nomenclature confusing, especially because it's
> supposed to mean the same thing whether the stack grows up or down. In
> my mind, if the stack grow down then top means "oldest element", but if the
> stack grows up, then top means "newest element".
> But in this patch it seems that top means "newest element" regardless of the
> direction of stack growth.

Yes, that was my understanding. So independent in which direction a shadow stack
grows based on the architecture/OS, top always means newest element.  But I think
it is not a problem to take one of your suggestions.

> I would suggest changing the enum names above to something that's not
> related to the vertical axis, so that their meaning will be clear regardless of
> which direction the stack grows. A few suggestions:
> shrink/grow, older/younger, outer/inner.

I'd take outer/inner and describe it as follows:

enum class ssp_update_direction
{
  /* Update ssp towards the oldest (outermost) element of the shadow
     stack.  */
  outer = 0,

  /* Update ssp towards the most recent (innermost) element of the
     shadow stack.  */
  inner
};

Is that understandable ?

> > +/* Return a new shadow stack pointer which is incremented or
> decremented
> > +   by COUNT elements dependent on DIRECTION.  */
> > +
> > +static CORE_ADDR
> > +update_shadow_stack_pointer (gdbarch *gdbarch, CORE_ADDR ssp,
> > +			     const ssp_update_direction direction) {
> > +  bool increment = (gdbarch_stack_grows_down (gdbarch))
> > +		    ? (direction == ssp_update_direction::bottom)
> > +		      : (direction == ssp_update_direction::top);
> 
> All the parentheses above are superfluous and can be removed.

Ok, fixed.

> > +  CORE_ADDR new_ssp;
> 
> This variable is unused and can be removed. Because by default GDB build
> with -Werror, this patch breaks the build with an "unused variable"
> error until patch 6 which removes this variable.

Arg, I missed that, because it's also fixed in a later patch.
I now removed it also from this patch.

> > +  if (increment)
> > +    return ssp + gdbarch_shadow_stack_element_size_aligned (gdbarch);
> > +  else
> > +    return ssp - gdbarch_shadow_stack_element_size_aligned (gdbarch);
> > +}
> > +
> > +/* See shadow-stack.h.  */
> > +
> > +void shadow_stack_push (gdbarch *gdbarch, regcache *regcache,
> 
> There's no need for a gdbarch argument. You can get it from the regcache.

Fixed.

> > +			const CORE_ADDR new_addr)
> > +{
> > +  if (!gdbarch_address_in_shadow_stack_memory_range_p (gdbarch)
> > +      || gdbarch_ssp_regnum (gdbarch) == -1)
> > +    return;
> > +
> > +  bool shadow_stack_enabled;
> > +  std::optional<CORE_ADDR> ssp
> > +    = gdbarch_get_shadow_stack_pointer (gdbarch, regcache,
> > +					shadow_stack_enabled);
> > +  if (!ssp.has_value () || !shadow_stack_enabled)
> > +    return;
> > +
> > +  const CORE_ADDR new_ssp
> > +    = update_shadow_stack_pointer (gdbarch, *ssp,
> > +				   ssp_update_direction::top);
> > +
> > +  /* If NEW_SSP does not point to shadow stack memory, we assume the
> stack
> > +     is full.  */
> > +  std::pair<CORE_ADDR, CORE_ADDR> range;
> > +  if (!gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> > +						     new_ssp,
> > +						     &range))
> 
> Range isn't really needed by this function. I suggest changing
> gdbarch_address_in_shadow_stack_memory_range to allow for it to be
> nullptr and then pass nullptr here.

I agree, fixed.

> Also, the line above fits in 80 columns and doesn't need to be broken, even if
> "&range" is changed to "nullptr".

It is more than 80 columns for me.

> 
> > +    error (_("No space left on the shadow stack."));
> > +
> > +  /* On x86 there can be a shadow stack token at bit 63.  For x32,  the
> > +     address size is only 32 bit.  Always write back the full 8 bytes to
> > +     include the shadow stack token.  */
> 
> s/8 bytes/element size/

Fixed.

> 
> > +  const int element_size
> > +    = gdbarch_shadow_stack_element_size_aligned (gdbarch);
> > +
> > +  const bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +
> > +  write_memory_unsigned_integer (new_ssp, element_size, byte_order,
> > +				 (ULONGEST) new_addr);
> > +
> > +  regcache_raw_write_unsigned (regcache,
> > +			       gdbarch_ssp_regnum (gdbarch),
> > +			       new_ssp);
> 
> The line above fits in 80 columns and doesn't need to be broken.

I count 81 columns and there is also a soft limit of 74 characters:

https://sourceware.org/legacy-ml/gdb-patches/2014-01/msg00216.html

So I'll keep it as is, if that's fine for you.

> 
> > +}
> > +
> > +/* See shadow-stack.h.  */
> > +
> > +value *
> > +dwarf2_prev_ssp (const frame_info_ptr &this_frame, void **this_cache,
> > +		 int regnum)
> > +{
> > +  value *v = frame_unwind_got_register (this_frame, regnum, regnum);
> > +  gdb_assert (v != nullptr);
> > +
> > +  gdbarch *gdbarch = get_frame_arch (this_frame);
> > +
> > +  if (gdbarch_address_in_shadow_stack_memory_range_p (gdbarch)
> > +      && v->entirely_available () && !v->optimized_out ())
> > +    {
> > +      const int size = register_size (gdbarch, regnum);
> > +      bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +      CORE_ADDR ssp = extract_unsigned_integer
> > +	(v->contents_all ().data (), size, byte_order);
> > +
> > +      /* Only if the current shadow stack pointer SSP points to shadow
> > +	 stack memory a valid previous shadow stack pointer can be
> > +	 calculated.  */
> > +      std::pair<CORE_ADDR, CORE_ADDR> range;
> > +      if (gdbarch_address_in_shadow_stack_memory_range (gdbarch, ssp,
> > +							&range))
> 
> This line fits in 80 columns and doesn't need to be broken.

In this case I'll take your suggestion, since it looks much nicer unbroken. 😊

> 
> > +	{
> > +	  /* Note that a shadow stack memory range can change, due to
> > +	     shadow stack switches for instance on x86 for an inter-
> > +	     privilege far call or when calling an interrupt/exception
> > +	     handler at a higher privilege level.  Shadow stack for
> > +	     userspace is supported for amd64 linux starting with
> > +	     Linux kernel v6.6.  However, shadow stack switches are not
> > +	     supported due to missing kernel space support.  We therefore
> > +	     implement this unwinder without support for shadow stack
> > +	     switches for now.  */
> > +	  const CORE_ADDR new_ssp
> > +	    = update_shadow_stack_pointer (gdbarch, ssp,
> > +					   ssp_update_direction::bottom);
> > +
> > +	  /* On x86, if NEW_SSP points to the end of RANGE, it indicates
> > +	     that NEW_SSP is valid, but the shadow stack is empty.  In
> > +	     contrast, on ARM's Guarded Control Stack, if NEW_SSP points
> > +	     to the end of RANGE, it means that the shadow stack pointer
> > +	     is invalid.  */
> > +	  bool is_top_addr_empty_shadow_stack
> > +	    = gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
> > +	      && gdbarch_top_addr_empty_shadow_stack (gdbarch, new_ssp,
> > +range);
> > +
> > +	  /* Check whether the new SSP is valid.  Depending on the
> > +	     architecture, this may rely on both
> > +	     IS_TOP_ADDR_EMPTY_SHADOW_STACK and the return value of
> > +	     gdbarch_address_in_shadow_stack_memory_range, or on the
> > +	     latter only.  */
> > +	  if (is_top_addr_empty_shadow_stack
> 
> Considering that, as previously mentioned,
> gdbarch_address_in_shadow_stack_memory_range uses '<' rather than '<='
> in the memory range check, AArch64 doesn't need
> gdbarch_top_addr_empty_shadow_stack.

Agree, but for amd64 I need it... (as described before).

> 
> The way this if condition is written, I think it's x86_64 that would need to
> provide an gdbarch_top_addr_empty_shadow_stack implementation.
> I'm surprised it doesn't.

As described before, I missed to pass new_ssp instead of ssp, which made this
code work for amd64.  But it's not correct, and I need
gdbarch_top_addr_empty_shadow_stack.
Thanks again for catching that!

> > +	      || gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> > +							       ssp,
> 
> Shouldn't this argument be new_ssp?

Already discussed before.

> 
> > +							       &range))
> > +	    return frame_unwind_got_address (this_frame, regnum, new_ssp);
> > +	}
> > +    }
> > +
> > +  /* Return a value which is marked as unavailable, in case we could not
> > +     calculate a valid previous shadow stack pointer.  */
> > +  value *retval
> > +    = value::allocate_register (get_next_frame_sentinel_okay (this_frame),
> > +				regnum, register_type (gdbarch, regnum));
> > +  retval->mark_bytes_unavailable (0, retval->type ()->length ());
> > +  return retval;
> > +
> 
> Spurious empty line added here.

Fixed.

> > +}
> > diff --git a/gdb/shadow-stack.h b/gdb/shadow-stack.h new file mode
> > 100644 index 00000000000..5c3ba80974e
> > --- /dev/null
> > +++ b/gdb/shadow-stack.h
> > @@ -0,0 +1,39 @@
> > +/* Definitions to manage a shadow stack pointer for GDB, the GNU
> debugger.
> > +
> > +   Copyright (C) 2024-2025 Free Software Foundation, Inc.
> > +
> > +   This file is part of GDB.
> > +
> > +   This program is free software; you can redistribute it and/or
> > + modify
> > +
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see
> > + <http://www.gnu.org/licenses/>.  */
> > +
> > +#ifndef GDB_SHADOW_STACK_H
> > +#define GDB_SHADOW_STACK_H
> > +
> > +/* If shadow stack is enabled, push the address NEW_ADDR on the
> shadow
> > +   stack and update the shadow stack pointer accordingly.  */
> > +
> > +void shadow_stack_push (gdbarch *gdbarch, regcache *regcache,
> 
> Recently, the project has been trying to make the header files contain all the
> headers and definitions that they need, for the benefit of IDE and language
> server users, so that these tools don't emit spurious errors when showing a
> header file.

Ah, ok I wasn't aware. Do you have a link for that ? I think I cannot follow 100 %.

> In this case, clangd is complaining here that regcache is unknown. There's no
> need to include regcache.h just for it, just adding a "class regcache" forward
> declaration at the beginning of the file is enough.
> 
> > +			const CORE_ADDR new_addr);

Ok, added!

> > +
> > +/* Unwind the previous shadow stack pointer of THIS_FRAME's shadow
> stack
> > +   pointer.  REGNUM is the register number of the shadow stack pointer.
> > +   Return a value that is unavailable in case we cannot unwind the
> > +   previous shadow stack pointer.  Otherwise, return a value containing
> > +   the previous shadow stack pointer.  */
> > +
> > +value * dwarf2_prev_ssp (const frame_info_ptr &this_frame,
> 
> The space after "value *" should be removed.

Fixed.

> 
> > +			 void **this_cache, int regnum);
> > +
> > +#endif /* GDB_SHADOW_STACK_H */
> --
> Thiago
> 
> ¹ https://inbox.sourceware.org/gdb-patches/87ldo6c84t.fsf@redhat.com/


Christina
Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928