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, 

Thanks a lot for your feedback. 

For the hook gdbarch_get_shadow_stack_size I still need your GCS implementation. 😊 
I suggest a separate patch for this with you as the only author and I'll include this in my v3
then, too. Does that make sense?

Please see my comments to your feedback below. 

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Freitag, 6. März 2026 05:31
> 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.
> 
> 
> I'll have a closer look at this patch next week, including your comments about
> the count argument to the get_trailing_outermost_shadow_stack_frame_info
> in the thread for v1 of this series.. Unfortunately I didn't have much time to dig
> into it yet.
> 
> Some minor comments for now:
> 
> Christina Schimpe <christina.schimpe@intel.com> writes:
> 
> > diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index
> > 656daa0f0ee..a4eabccf667 100644
> > --- a/gdb/amd64-linux-tdep.c
> > +++ b/gdb/amd64-linux-tdep.c
> > @@ -1963,6 +1963,29 @@ amd64_linux_top_addr_empty_shadow_stack
> >    return addr == range.second;
> >  }
> >
> > +/* Return the number of elements which are currently on the shadow stack
> > +   based on 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.  */
> > +
> > +static long
> > +amd64_linux_get_shadow_stack_size
> > +  (gdbarch *gdbarch,
> > +   const std::optional<CORE_ADDR> ssp,
> > +   const std::pair<CORE_ADDR, CORE_ADDR> range) {
> > +  /* For x86, if we don't have a shadow stack pointer, we can assume
> > +     that the shadow stack is disabled for the current thread.  */
> > +  if (!ssp.has_value ())
> > +    return -1;
> 
> As I mention a bit below, the only caller of this function passes a value for the
> ssp argument, so this is dead code.

I agree, thanks. I removed this.
 
> > +  const unsigned long shadow_stack_bytes = range.second - *ssp;
> > +
> > +  gdb_assert ((shadow_stack_bytes % 8) == 0);
> 
> I don't think this should be an assert. If it fails, it triggers an internal error in
> GDB.  In this case it could indeed mean an internal error (GDB somehow got
> the SSP or range wrong), but it could also be (and probably more likely) an
> inconsistent state of the inferior. This can happen in a program being debugged
> so GDB should be able to handle it gracefully, and if possible provide useful
> information to the user.

I agree. This is rather something that is outside GDB's control.

From the documentation for internal errors:
"Internal errors indicate programming errors such as assertion failures, as opposed to
   more general errors beyond the application's control.  "

So based on that I rather would choose a normal error, not an internal error.
What do you think ?

> > +  return shadow_stack_bytes / 8;
> > +}
> 
>   ⋮
> 
> > +/* 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)
> 
> The reason argument can also be a reference.

True, however, due to Tom's feedback this parameter no longer exists for this function.

> > +{
> > +  /* 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.  */
> > +
> > +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);
> 
> This is the only caller of gdbarch_get_shadow_stack_size. Does its ssp
> argument need to be std::optional<CORE_ADDR>, or can it simply be a
> CORE_ADDR?

No, I agree. I'll make it a const CORE_ADDR.
 
> > +  /* 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;
> > +
> > +  /* 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});
> > +}
> 
>   ⋮
> 
> > diff --git a/gdb/shadow-stack.h b/gdb/shadow-stack.h index
> > 5f8395ec047..5370becfc9a 100644
> > --- a/gdb/shadow-stack.h
> > +++ b/gdb/shadow-stack.h
> > @@ -35,4 +35,10 @@ void shadow_stack_push (regcache *regcache, const
> > CORE_ADDR new_addr);  value *dwarf2_prev_ssp (const frame_info_ptr
> &this_frame,
> >  			void **this_cache, int regnum);
> >
> > +/* Implementation of "backtrace shadow" comand.  */
> > +
> > +void backtrace_shadow_command
> > +  (const frame_print_options &fp_opts,
> > +   const char *count_exp, int from_tty);
> > +
> >  #endif /* GDB_SHADOW_STACK_H */
> 
> This header needs to forward-declare "struct frame_print_options;", similarly
> to the "class regcache;" forward declaration.
> 
> Actually, one thing I missed in my review of patch 1 is that it also needs to
> forward-declare "class frame_info_ptr;" which is used in the prototype of
> dwarf2_prev_ssp.

Agree - will fix.

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