RE: [PATCH v2 6/9] gdb: Add command option 'bt -shadow' to print the shadow stack backtrace.

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

Hi Thiago, 

Thank you for the review, please find my feedback below.

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Dienstag, 12. Mai 2026 05:32
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 6/9] gdb: Add command option 'bt -shadow' to print
> the shadow stack backtrace.
> 
> Hello Christina,
> 
> I was having one more look at this patch today. I just have a few nits:
> 
> Christina Schimpe <christina.schimpe@intel.com> writes:
> 
> > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> > index d44e59a523d..1f24a3a6d72 100644
> > --- a/gdb/gdbarch_components.py
> > +++ b/gdb/gdbarch_components.py
> > @@ -2830,9 +2830,11 @@ provided:
> >    unwinding and inferior function calls
> >  - top_addr_empty_shadow_stack: required for shadow stack pointer
> > unwinding
> >  - ssp_regnum: required for inferior function calls.
> > -
> >  If the shadow stack alignment is not the predefault of 8 bytes,
> > configure  the gdbarch value shadow_stack_element_size_aligned.
> > +To support the command line option 'backtrace -shadow' in addition to
> > +all values and methods listed above also the gdbarch hook
> > +get_shadow_stack_size, has to be provided.
> >
> >  If possible, return the shadow stack pointer.  If the shadow stack
> > feature is enabled then set SHADOW_STACK_ENABLED to true, otherwise
> @@
> > -2893,3 +2895,19 @@ this value.
> >      predefault="8",
> >      invalid=False,
> >  )
> > +
> > +Method(
> > +    comment="""
> > +Return the number of elements which are currently on the shadow stack
> > +based on the shadow stack pointer SSP and the shadow stack memory
> > +RANGE [start_address, end_address) of the current thread.
> > +In case shadow stack is not enabled for the current thread, return -1.
> > +""",
> > +    type="long",
> > +    name="get_shadow_stack_size",
> > +    params=[
> > +        ("const std::optional<CORE_ADDR>", "ssp"),
> > +        ("const std::pair<CORE_ADDR, CORE_ADDR>", "range")
> > +    ],
> > +    predicate=True,
> > +)
> 
> Small nit: we aim to keep the Python files in the style used by the Black
> Python code formatter. If you run it on this file:
> 
> $ black gdb/gdbarch_components.py
> 
> It changes the above to:
> 
>      name="get_shadow_stack_size",
>      params=[
>          ("const std::optional<CORE_ADDR>", "ssp"),
> -        ("const std::pair<CORE_ADDR, CORE_ADDR>", "range")
> +        ("const std::pair<CORE_ADDR, CORE_ADDR>", "range"),
>      ],
>      predicate=True,
>  )

Good point I'll run black. 
Otherwise, this hook might disappear due to the potential interface changes
described here: 
https://sourceware.org/pipermail/gdb-patches/2026-May/227481.html
Let's see where this is going. 😊

> > +/* Read the memory at shadow stack pointer SSP and assign it to
> > +   RETURN_VALUE.  In case we cannot read the memory, set REASON to
> > +   ssp_unwind_stop_reason::memory_read_error and return false.  */
> > +
> > +static bool
> > +read_shadow_stack_memory (gdbarch *gdbarch, CORE_ADDR ssp,
> > +			  CORE_ADDR &return_value,
> > +			  ssp_unwind_stop_reason *reason)
> 
> "reason" can also be a reference, since it's assumed to be non-null.

Tom suggested to remove reason as parameter, but this might change in general
too with the changes described above..

> > +{
> > +  /* On x86 there can be a shadow stack token at bit 63.  For x32, the
> > +     address size is only 32 bit.  Thus, we still must use
> > +     gdbarch_shadow_stack_element_size_aligned (and not
> gdbarch_addr_bit)
> > +     to read the full element for x32 as well.  */
> > +  const int element_size
> > +    = gdbarch_shadow_stack_element_size_aligned (gdbarch);
> > +
> > +  const bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +  if (!safe_read_memory_unsigned_integer (ssp, element_size, byte_order,
> > +					  &return_value))
> > +    {
> > +      *reason = ssp_unwind_stop_reason::memory_read_error;
> > +      return false;
> > +    }
> > +
> > +  return true;
> > +}
> > +
> > +/*  If possible, return the starting shadow stack frame info needed to
> handle
> > +    COUNT outermost frames.  FRAME should point to the innermost
> (newest)
> > +    element of the shadow stack.  RANGE is the shadow stack memory
> range
> > +    [start_address, end_address) corresponding to FRAME's shadow stack
> pointer.
> > +    If COUNT is bigger than the number of elements on the shadow stack,
> return
> > +    FRAME.  In case of failure, assign an appropriate
> ssp_unwind_stop_reason in
> > +    FRAME->UNWIND_stop_REASON.  */
> 
> I like this new wording of the doc comment. Thanks!
> 
> Just one nit: UNWIND_stop_REASON should be all upper-case. :)

Thanks, will fix.

> > +
> > +static std::optional<shadow_stack_frame_info>
> > +get_trailing_outermost_shadow_stack_frame_info
> > +  (gdbarch *gdbarch, const std::pair<CORE_ADDR, CORE_ADDR> range,
> > +   const ULONGEST count, shadow_stack_frame_info &frame) {
> > +  gdb_assert (gdbarch_get_shadow_stack_size_p (gdbarch));
> > +
> > +  const long shadow_stack_size
> > +    = gdbarch_get_shadow_stack_size (gdbarch,
> > +				     std::optional<CORE_ADDR> (frame.ssp),
> > +				     range);
> > +
> > +  /* We should only get here in case shadow stack is enabled for the
> > +     current thread.  */
> > +  gdb_assert (shadow_stack_size >= 0);
> > +
> > +  const long level = shadow_stack_size - count;
> 
> Just for the record: as I'm sure you realized, I was wrong in my comment in
> v1's review where I said this expression to calculate level was missing a "- 1"
> term. This looks good. Sorry about that.
> 
> I think this function and the loop that calls it in backtrace_shadow_command
> are all good now.

I don't think you were wrong.  😊
The earlier function description did not match the meaning of count. I described it in this thread
https://inbox.sourceware.org/gdb-patches/PH0PR11MB76364987546F1D5365516856F98CA@PH0PR11MB7636.namprd11.prod.outlook.com/
(it starts with " Ah, I think now I get your point")

> > +  /* COUNT exceeds the number of elements on the shadow stack.  Return
> the
> > +     starting shadow stack frame info FRAME.  */  if (level <= 0)
> > +    return std::optional<shadow_stack_frame_info> (frame);
> > +
> > +  CORE_ADDR new_ssp = update_shadow_stack_pointer
> > +    (gdbarch, frame.ssp, level, ssp_update_direction::outer);
> > +
> > +  if (gdbarch_stack_grows_down (gdbarch))
> > +    gdb_assert (new_ssp < range.second);  else
> > +    gdb_assert (new_ssp >= range.first);
> > +
> > +  CORE_ADDR new_value;
> > +  if (!read_shadow_stack_memory (gdbarch, new_ssp, new_value,
> > +				 &frame.unwind_stop_reason))
> > +    return {};
> > +
> > +  return std::optional<shadow_stack_frame_info>
> > +    ({new_ssp, new_value, (unsigned long) level,
> > +      ssp_unwind_stop_reason::no_error});
> > +}
> > +
> > +std::optional<shadow_stack_frame_info>
> > +shadow_stack_frame_info::unwind_prev_shadow_stack_frame_info
> > +  (gdbarch *gdbarch, std::pair<CORE_ADDR, CORE_ADDR> range) {
> > +  /* If the user's backtrace limit has been exceeded, stop.  We must
> > +     add two to the current level; one of those accounts for
> > +     backtrace_limit being 1-based and the level being 0-based, and the
> > +     other accounts for the level of the new frame instead of the level
> > +     of the current frame.  */
> > +  if (this->level + 2 > user_set_backtrace_options.backtrace_limit)
> > +    return {};
> > +
> > +  CORE_ADDR new_ssp
> > +    = update_shadow_stack_pointer (gdbarch, this->ssp, 1,
> > +				   ssp_update_direction::outer);
> > +
> > +   if (gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
> > +       && gdbarch_top_addr_empty_shadow_stack (gdbarch, new_ssp,
> range))
> > +     return {};
> > +   else if (gdbarch_stack_grows_down (gdbarch))
> 
> The four lines above are indented with one extra space.

Will fix.

> ⋮
> > +  if (read_shadow_stack_memory (gdbarch, *start_ssp, new_value,
> &reason))
> > +    current = {*start_ssp, new_value, 0,
> > + ssp_unwind_stop_reason::no_error};
> > +
> > +  std::optional<shadow_stack_frame_info> trailing = current;
> > +
> > +  LONGEST count = -1;
> > +  if (current.has_value () && count_exp != nullptr)
> > +    {
> > +      count = parse_and_eval_long (count_exp);
> > +      /* If count is negative, update trailing with the shadow stack frame
> > +	 info from which we should start printing.  */
> > +      if (count < 0)
> > +	{
> > +	  trailing = get_trailing_outermost_shadow_stack_frame_info
> > +		       (gdbarch, range, std::abs (count), *current);
> > +
> > +	  if (!trailing.has_value ())
> > +	    reason = current->unwind_stop_reason;
> > +	}
> > +    }
> > +
> > +  if (!trailing.has_value ())
> > +    {
> > +      gdb_assert (reason != ssp_unwind_stop_reason::no_error);
> > +
> > +      error (_("Cannot print shadow stack backtrace: %s.\n"),
> > +	     ssp_unwind_stop_reason_to_err_string (reason));
> > +    }
> > +
> > +  current = trailing;
> 
> The assignment above is now redundant, since it's also in the for loop initial
> condition.

Thanks, will remove!


> > +  for (current = trailing; current.has_value () && count != 0; count--)
> > +    {
> > +      QUIT;
> > +
> > +      print_shadow_stack_frame_info (gdbarch, fp_opts, *current,
> > + LOCATION);
> > +
> > +      trailing = current;
> > +      current = current->unwind_prev_shadow_stack_frame_info (gdbarch,
> range);
> > +    }
> > +
> > +  /* If we've stopped before the end, mention that.  */  if
> > + (current.has_value () && from_tty)
> > +    gdb_printf (_("(More shadow stack frames follow...)\n"));
> > +
> > +  /* Due to the loop above, trailing always has a value at this
> > + point.  */  gdb_assert (trailing.has_value ());
> > +
> > +  /* If we've run out of shadow stack frames, and the reason appears to
> > +     be an error condition, print it.  */
> > +  if (!current.has_value ()
> > +      && trailing->unwind_stop_reason >
> ssp_unwind_stop_reason::no_error)
> > +    gdb_printf (_("Shadow stack backtrace stopped at shadow stack " \
> > +		  "pointer %s due to: %s.\n"),
> > +		paddress (gdbarch, trailing->ssp),
> > +		ssp_unwind_stop_reason_to_err_string
> > +		  (trailing->unwind_stop_reason));
> > +}
> 
> --
> Thiago

Christina
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 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 Muenchen HRB 186928