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

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

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Mittwoch, 26. November 2025 05:19
> 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.
> 
> Hello Christina,
> 
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> 
> > Thanks a lot for this detailed review!
> > I applied most of your comments, please find my feedback to your review
> below.
> 
> You're welcome!
> 
> >> -----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:
> >>
> >> > -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?
> 
> Yes, I agree.
> 
> >> > -	    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.
> 
> Ah, right. Thanks for clarifying.
> 
> >> > +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.
> 
> Thank you. In my view it also matches the nomenclature in frame.h, which
> also doesn't use vertical concepts. E.g.,
> 
>   /* Given a FRAME, return the next (more inner, younger) or previous
>      (more outer, older) frame.  */
>   extern frame_info_ptr get_prev_frame (const frame_info_ptr &);
>   extern frame_info_ptr get_next_frame (const frame_info_ptr &);
> 
> >> 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 ?
> 
> Yes, thanks for making the change.
> 
> >> > +/* 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.
> 
> Hm. When I edited it here and changed "&range" to "nullptr" the line ended
> exactly at column 80. Which is arguably not ideal, so I don't mind either way.
> 
> >> > +    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
> 
> Ah, I wasn't aware of the soft limit. Thanks for pointing it out.
> 
> > So I'll keep it as is, if that's fine for you.
> 
> Yes, of course.
> 
> >> > 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 %.
> 
> There was a discussion about it in this thread:
> 
> https://sourceware.org/pipermail/gdb-patches/2024-February/206632.html
> 
> It resulted in this patch:
> 
> https://inbox.sourceware.org/gdb-patches/20240326190806.89541-4-
> simon.marchi@efficios.com/
> 
> And it's also in the wiki²:
> 
>   A .c, .cc or .h file should directly include the .h file of every
>   declaration and/or definition it directly refers to. Exception: Do not
>   include defs.h, server.h, common-defs.h directly.
> 
> --
> Thiago
> 
> ² https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
> Standards#Include_Files

Hi Thiago,

Thank you for the feedback and sharing this.:)

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