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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

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.

-- 
Thiago