RE: [PATCH 9/9] gdb, mi: Add -shadow-stack-list-frames command

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

Hi Thiago, 

Thank you for the feedback!

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Mittwoch, 26. November 2025 05:27
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 9/9] gdb, mi: Add -shadow-stack-list-frames command
> 
> Christina Schimpe <christina.schimpe@intel.com> writes:
> 
> > +/* Parse arguments of -shadow-stack-list-frames command and set
> FRAME_LOW
> > +   and FRAME_HIGH accordingly.  Throw an error in case the arguments are
> > +   invalid.  */
> > +static void
> > +mi_cmd_shadow_stack_list_frames_parse_args (const char *const *argv,
> > +					    int argc, int &frame_low,
> > +					    int &frame_high)
> > +{
> > +  /* There should either be low - high range, or no arguments.  */
> > +  if ((argc != 0) && (argc != 2))
> > +    error (_("-shadow-stack-list-frames: Usage: [FRAME_LOW
> > +FRAME_HIGH]"));
> > +
> > +  /* If there is a range, set it.  */
> > +  if (argc == 2)
> > +    {
> > +      frame_low = atoi (argv[0]);
> > +      frame_high = atoi (argv[1]);
> > +      std::string err_str;
> > +      if (frame_low < 0)
> > +	{
> > +	  err_str = "``" + std::to_string (frame_low) + "''";
> > +	  if (frame_high < 0)
> > +	    err_str += " and ``" + std::to_string (frame_high) + "''";
> > +	}
> > +      else if (frame_high < 0)
> > +	err_str = "``" + std::to_string (frame_high) + "''";
> > +
> > +      if (!err_str.empty ())
> > +	{
> > +	  err_str = "-shadow-stack-list-frames: Invalid option " + err_str;
> > +	  error (_("%s."), err_str.c_str ());
> 
> The call to _() here isn't very useful, because it puts the string "%s."
> in the message file for translation:
> 
> #: gnu-nat.c:3090 mi/mi-cmd-stack.c:810
> #, possible-c-format
> msgid "%s."
> msgstr ""
> 
> And we see from the comment that gnu-nat.c has the same problem. :)
> 
> Messages constructed piece by piece aren't translatable and so should be
> avoided if possible.
> 
> > +	}
> > +    }
> > +  else
> > +    {
> > +      /* No arguments, print the whole shadow stack backtrace.  */
> > +      frame_low = -1;
> > +      frame_high = -1;
> > +    }
> > +}
> > +
> > +/* Print a list of the shadow stack frames.  Args can be none, in which
> > +   case we want to print the whole shadow stack backtrace, or a pair of
> > +   numbers specifying the frame numbers at which to start and stop the
> > +   display.  If the two numbers are equal, a single frame will be
> > +   displayed.  */
> > +
> > +void
> > +mi_cmd_shadow_stack_list_frames (const char *command,
> > +				 const char *const *argv,
> > +				 int argc)
> > +{
> > +  int frame_low;
> > +  int frame_high;
> > +
> > +  mi_cmd_shadow_stack_list_frames_parse_args (argv, argc, frame_low,
> > +					      frame_high);
> > +
> > +  if (!target_has_stack ())
> > +    error (_("-shadow-stack-list-frames: No shadow stack."));
> > +
> > +  gdbarch *gdbarch = get_current_arch ();
> > +  if (!gdbarch_address_in_shadow_stack_memory_range_p (gdbarch))
> > +    error (_("-shadow-stack-list-frames: Printing of shadow stack \
> > +	      backtrace is not supported for the current target."));
> 
> The string above contains all the whitespace used for indentation. It should
> be:
> 
>     error (_("-shadow-stack-list-frames: Printing of shadow stack "
> 	     "backtrace is not supported for the current target."));
> 
> Same for the other multi-line messages in this patch.
> 
> > +  regcache *regcache = get_thread_regcache (inferior_thread ());
> > + bool shadow_stack_enabled = false;
> > +
> > +  std::optional<CORE_ADDR> start_ssp
> > +    = gdbarch_get_shadow_stack_pointer (gdbarch, regcache,
> > +					shadow_stack_enabled);
> > +  if (!start_ssp.has_value () || !shadow_stack_enabled)
> > +    error (_("-shadow-stack-list-frames: Shadow stack is not enabled for \
> > +	      the current target."));
> 
> My comment on patch 6 about a similar message also applies here:
> 
>   At least for AArch64, GCS can be enabled or disabled per-thread so
>   I would say "not enabled for the current thread" here.
> 
> > diff --git a/gdb/shadow-stack.c b/gdb/shadow-stack.c index
> > 42032f51781..ef8e31c730e 100644
> > --- a/gdb/shadow-stack.c
> > +++ b/gdb/shadow-stack.c
> > @@ -270,6 +270,12 @@ do_print_shadow_stack_frame_info
> >        uiout->field_string
> >  	("addr", hex_string_custom (frame.value, element_size * 2),
> >  	 address_style.style ());
> > +
> > +      if (uiout->is_mi_like_p ())
> > +	{
> > +	  uiout->field_string
> > +	    ("arch", (gdbarch_bfd_arch_info (gdbarch))->printable_name);
> 
> Extraneous parentheses around gdbarch_bfd_arch_info (gdbarch).
> 
> > +	}
> >        uiout->text ("\n");
> >        gdb_flush (gdb_stdout);
> >        return;
> > @@ -333,6 +339,12 @@ do_print_shadow_stack_frame_info
> >  	    if (lib != nullptr)
> >  	      print_lib (uiout, lib, true);
> >  	  }
> > +
> > +	if (uiout->is_mi_like_p ())
> > +	  {
> > +	    uiout->field_string
> > +	      ("arch", (gdbarch_bfd_arch_info (gdbarch))->printable_name);
> 
> Extraneous parentheses around gdbarch_bfd_arch_info (gdbarch).
> 
> > +	  }
> >        } /* Extra scope to print frame tuple.  */
> >
> >        uiout->text ("\n");
> 
> <snip>
> 
> > diff --git a/gdb/testsuite/gdb.mi/mi-shadow-stack-signal.exp
> > b/gdb/testsuite/gdb.mi/mi-shadow-stack-signal.exp
> > new file mode 100644
> > index 00000000000..219f1b8ad78
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.mi/mi-shadow-stack-signal.exp
> 
> My only suggestion for this testcase is to add "amd64" to the filename, since
> it can only run on x86. Perhaps mi-amd64-shadow-stack-signal.exp?
> 
> > diff --git a/gdb/testsuite/gdb.mi/mi-shadow-stack.exp
> > b/gdb/testsuite/gdb.mi/mi-shadow-stack.exp
> > new file mode 100644
> > index 00000000000..bbbc7ec5460
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.mi/mi-shadow-stack.exp
> 
> Samme suggestion here, about adding "amd64" to the filename.

I applied all of it except the filename, since for mi I don't see any file
starting with amd64-* in the testsuite folder gdb.mi/*.

What happens if you run this for GCS ? Could we create a source file
which is architecture independent?

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