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

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

Christina Schimpe <christina.schimpe@intel.com> writes:

> Add the mi command for the command "backtrace -shadow".
> Similar to the mi interface for the ordinary backtrace command,
> support low-frame and high-frame as command line parameters.
>
> Example print of a full shadow stack backtrace:
> ~~~
> (gdb)
> -shadow-stack-list-frames
> ^done,shadow-stack=[
> shadow-stack-frame={level="0",addr="0x00007ffff7c3fe70",
>   func="__libc_start_call_main",file="../sysdeps/nptl/libc_start_call_main.h",
>   fullname="/usr/[...]/sysdeps/nptl/libc_start_call_main.h",
>   line="58",arch="i386:x86-64"},
> shadow-stack-frame={level="1",addr="0x00007ffff7c3ff20",
>   func="__libc_start_main_impl",file="../csu/libc-start.c",
>   fullname="/usr/[...]/csu/libc-start.c",
>   line="128",arch="i386:x86-64"},
> shadow-stack-frame={level="2",addr="0x0000000000401075",
>   func="_start",arch="i386:x86-64"}]
> ~~~
>
> Example print of a shadow stack backtrace using low- and high-frame:
> ~~~
> (gdb)
> -shadow-stack-list-frames 0 1
> ^done,shadow-stack=[
> shadow-stack-frame={level="0",addr="0x00007ffff7c3fe70",
>   func="__libc_start_call_main",file="../sysdeps/nptl/libc_start_call_main.h",
>   fullname="/usr/[...]/sysdeps/nptl/libc_start_call_main.h",
>   line="58",arch="i386:x86-64"},
> shadow-stack-frame={level="1",addr="0x00007ffff7c3ff20",
>   func="__libc_start_main_impl",file="../csu/libc-start.c",
>   fullname="/usr/[...]/csu/libc-start.c",
>   line="128",arch="i386:x86-64"}]
> ~~~
> ---
>  gdb/NEWS                                      |   8 ++
>  gdb/doc/gdb.texinfo                           |  51 ++++++++
>  gdb/mi/mi-cmd-stack.c                         | 100 +++++++++++++++
>  gdb/mi/mi-cmds.c                              |   2 +
>  gdb/mi/mi-cmds.h                              |   1 +
>  gdb/shadow-stack.c                            | 121 ++++++++++++------
>  gdb/shadow-stack.h                            |  40 ++++++
>  .../gdb.mi/mi-shadow-stack-signal.exp         |  69 ++++++++++
>  gdb/testsuite/gdb.mi/mi-shadow-stack.exp      |  93 ++++++++++++++
>  9 files changed, 449 insertions(+), 36 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-shadow-stack-signal.exp
>  create mode 100644 gdb/testsuite/gdb.mi/mi-shadow-stack.exp

I don't have much experience with MI, so I'll only comment on the
translation strings. Though to be honest in all this time AFAICT GDB
hasn't ever been translated so I wonder how useful it is to pay
attention to this.

> +/* 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)
> +{
> +  const std::string mi_cmd_name = "-shadow-stack-list-frames";
> +  /* There should either be low - high range, or no arguments.  */
> +  if ((argc != 0) && (argc != 2))
> +    error (_("%s: Usage: [FRAME_LOW FRAME_HIGH]"), mi_cmd_name.c_str ());
> +
> +  /* 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 = mi_cmd_name + ": Invalid option " + err_str + ".";
> +	  error (_("%s"), err_str.c_str ());

Passing "%s" as a string to be translated doesn't work. gettext extracts
the translatable strings by scanning the source code at build time, it's
not done at runtime.

You can see this if you run this in the GDB build directory:

$ make -C gdb gdb.pot 

Then you'll see in $build_dir/gdb/po/gdb.pot:

#: compile/compile.c:112 cp-namespace.c:528 dwarf2/read.c:1021
#: dwarf2/read.c:1030 mi/mi-cmd-stack.c:802 shadow-stack.c:632
#: shadow-stack.c:649 utils.c:657 read.c:1021 read.c:1030 mi-cmd-stack.c:802
#: compile.c:112
#, possible-c-format
msgid "%s"
msgstr ""

Which isn't very translatable. The only string containing "Invalid
option" in gdb.pot is from mi-cmd-var.c, unrelated to this code.

> +	}
> +    }
> +  else
> +    {
> +      /* No arguments, print the whole shadow stack backtrace.  */
> +      frame_low = -1;
> +      frame_high = -1;
> +    }
> +}

⋮

> +CORE_ADDR
> +get_validated_shadow_stack_pointer (gdbarch *gdbarch)
>  {
>    if (!target_has_stack ())
> -    error (_("No shadow stack."));
> +    {
> +      if (!current_uiout->is_mi_like_p ())
> +	error (_("No shadow stack."));
> +      else
> +	error (_("-shadow-stack-list-frames: No shadow stack."));
> +    }
>  
> -  gdbarch *gdbarch = get_current_arch ();
>    if (!gdbarch_address_in_shadow_stack_memory_range_p (gdbarch)
>        || !gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
>        || gdbarch_ssp_regnum (gdbarch) == -1)
> -    error (_("Printing of the shadow stack backtrace is not supported for"
> -	     " the current target."));
> +    {
> +      std::string err_str = "Printing of the shadow stack backtrace";
> +      err_str += " is not supported for the current target.";
> +      if (!current_uiout->is_mi_like_p ())
> +	error (_("%s"), err_str.c_str());
> +      else
> +	error (_("-shadow-stack-list-frames: %s"), err_str.c_str());

Same problem in the two _() calls above, and the two below.

> +    }
>  
>    regcache *regcache = get_thread_regcache (inferior_thread ());
>    bool shadow_stack_enabled = false;
> @@ -621,10 +642,25 @@ backtrace_shadow_command (const frame_print_options &fp_opts,
>  					shadow_stack_enabled);
>  
>    if (!start_ssp.has_value () || !shadow_stack_enabled)
> -    error (_("Shadow stack is not enabled for the current thread."));
> +    {
> +      std::string err_str
> +	= "Shadow stack is not enabled for the current thread.";
> +      if (!current_uiout->is_mi_like_p ())
> +	error (_("%s"), err_str.c_str());
> +      else
> +	error (_("-shadow-stack-list-frames: %s"), err_str.c_str());
> +    }
>  
> -  /* Check if START_SSP points to a shadow stack memory range and use
> -     the returned range to determine when to stop unwinding.
> +  return *start_ssp;
> +}

-- 
Thiago
(he/him)