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

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

Hi Tom, 

Thanks a lot for the review.

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Donnerstag, 19. Februar 2026 19:20
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org; thiago.bauermann@linaro.org
> Subject: Re: [PATCH v2 6/9] gdb: Add command option 'bt -shadow' to print
> the shadow stack backtrace.
> 
> >>>>> Christina Schimpe <christina.schimpe@intel.com> writes:
> 
> > Add command option '-shadow" to the backtrace command to print the
> > shadow stack backtrace instead of the normal backtrace.
> 
> Thanks.
> 
> > -annotate_frame_function_name (void)
> > +annotate_frame_function_name (bool shadowstack_frame)
> >  {
> >    if (annotation_level == 2)
> > -    printf_unfiltered (("\n\032\032frame-function-name\n"));
> > +    {
> > +      if (!shadowstack_frame)
> > +	printf_unfiltered (("\n\032\032frame-function-name\n"));
> > +      else
> > +	printf_unfiltered (("\n\032\032shadow-stack-frame-function-
> name\n"));
> > +    }
> 
> I think it is fine to just drop all the annotation changes.
> As far as I know, no client even uses annotation_level > 1.
> 
> Emacs, maybe the only existing user of annotations, passes --fullname which
> uses:
> 
> 	  case 'f':
> 	    annotation_level = 1;
> 

Ah, thanks for sharing that, I will remove them now.

> > +  if (should_print_location (print_what) || sal.symtab == nullptr)
> > +    {
> > +      gdb::unique_xmalloc_ptr<char> funname = find_pc_funname
> > + (frame.value);
> > +
> > +      { /* Extra scope to print frame tuple.  */
> > +	ui_out_emit_tuple tuple_emitter (uiout, "shadow-stack-frame");
> 
> The extra scope doesn't really look necessary here, since:
> > +      } /* Extra scope to print frame tuple.  */
> > +
> > +      uiout->text ("\n");
> 
> ... it seems fine to emit this text before the tuple emitter is destroyed?  And
> there's already an extras scope from the "then" block.

Hm, I followed the logic in the equivalent function for the normal backtrace: stack.c:print_frame.
But right now, I cannot think about a specific reason why the extra scope is important.

In similar code in the following patch
"gdb: Provide gdbarch hook to distinguish shadow stack backtrace elements."
I also don't have an extra scope before the new line... and it did not cause any problems.
So you're right, I can probably remove it.

> > +  gdb_flush (gdb_stdout);

> Is flushing really needed?  I feel like I saw this in a different patch in the series
> as well, and forgot to ask about it there.

In the similar function to print the normal backtrace (stack.c: do_print_frame_info), we have
the flushing, too. 

This should make sure that the output is directly visible to the user, which might be
important in case of long stack traces. 
Or in case of an error at least all frames that were correctly unwound are printed.
 
> > +
> > +/* 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.  */
> 
> It seems odd to combine a bool return and an out parameter when the enum
> has a "no_error" value.
> unwind_prev_shadow_stack_frame_info
> Maybe just a bool return would be more appropriate and then the callers
> could set their own out parameters.  Especially since one caller doesn't even
> need this.

You're right. I will change that. 

> > -static const char *const print_frame_info_choices[] =
> > +const char *const print_frame_info_choices[] =

> This is public now but I didn't see other uses of it.

It's a left-over from v1 of this series, thanks for catching it.

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