Re: [PATCH] Adjust i386 registers on SystemTap probes' arguments (PR breakpoints/24541)

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Sergio Durigan Junior <sergiodj@redhat.com> [2019-06-26 18:00:31 -0400]:

> This bug has been reported on PR breakpoints/24541, but it is possible
> to reproduce it easily by running:
> 
>   make check-gdb TESTS=gdb.base/stap-probe.exp RUNTESTFLAGS='--target_board unix/-m32'
> 
> The underlying cause is kind of complex, and involves decisions made
> by GCC and the sys/sdt.h header file about how to represent a probe
> argument that lives in a register in 32-bit programs.  I'll use
> Andrew's example on the bug to illustrate the problem.
> 
> libstdc++ has a probe named "throw" with two arguments.  On i386, the
> probe is:
> 
>   stapsdt              0x00000028       NT_STAPSDT (SystemTap probe descriptors)
>     Provider: libstdcxx
>     Name: throw
>     Location: 0x00072c96, Base: 0x00133d64, Semaphore: 0x00000000
>     Arguments: 4@%si 4@%di
> 
> I.e., the first argument is an unsigned 32-bit value (represented by
> the "4@") that lives on %si, and the second argument is an unsigned
> 32-bit value that lives on %di.  Note the discrepancy between the
> argument size reported by the probe (32-bit) and the register size
> being used to store the value (16-bit).
> 
> However, if you take a look at the disassemble of a program that uses
> this probe, you will see:
> 
>     00072c80 <__cxa_throw@@CXXABI_1.3>:
>        72c80:       57                      push   %edi
>        72c81:       56                      push   %esi
>        72c82:       53                      push   %ebx
>        72c83:       8b 74 24 10             mov    0x10(%esp),%esi
>        72c87:       e8 74 bf ff ff          call   6ec00 <__cxa_finalize@plt+0x980>
>        72c8c:       81 c3 74 e3 10 00       add    $0x10e374,%ebx
>        72c92:       8b 7c 24 14             mov    0x14(%esp),%edi
>        72c96:       90                      nop                      <----------------- PROBE IS HERE
>        72c97:       e8 d4 a2 ff ff          call   6cf70 <__cxa_get_globals@plt>
>        72c9c:       83 40 04 01             addl   $0x1,0x4(%eax)
>        72ca0:       83 ec 04                sub    $0x4,%esp
>        72ca3:       ff 74 24 1c             pushl  0x1c(%esp)
>        72ca7:       57                      push   %edi
>        72ca8:       56                      push   %esi
>        72ca9:       e8 62 a3 ff ff          call   6d010 <__cxa_init_primary_exception@plt>
>        72cae:       8d 70 40                lea    0x40(%eax),%esi
>        72cb1:       c7 00 01 00 00 00       movl   $0x1,(%eax)
>        72cb7:       89 34 24                mov    %esi,(%esp)
>        72cba:       e8 61 96 ff ff          call   6c320 <_Unwind_RaiseException@plt>
>        72cbf:       89 34 24                mov    %esi,(%esp)
>        72cc2:       e8 c9 84 ff ff          call   6b190 <__cxa_begin_catch@plt>
>        72cc7:       e8 d4 b3 ff ff          call   6e0a0 <_ZSt9terminatev@plt>
>        72ccc:       66 90                   xchg   %ax,%ax
>        72cce:       66 90                   xchg   %ax,%ax
> 
> Note how the program is actually using %edi, and not %di, to store the
> second argument.  This is the problem here.
> 
> GDB will basically read the probe argument, then read the contents of
> %di, and then cast this value to uint32_t, which causes the wrong
> value to be obtained.  In the gdb.base/stap-probe.exp case, this makes
> GDB read the wrong memory location, and not be able to display a test
> string.  In Andrew's example, this causes GDB to actually stop at a
> "catch throw" when it should actually have *not* stopped.
> 
> After some discussion with Frank Eigler and Jakub Jelinek, it was
> decided that this bug should be fixed on the client side (i.e., the
> program that actually reads the probes), and this is why I'm proposing
> this patch.
> 
> The idea is simple: we will have a gdbarch method, which, for now, is
> only used by i386.  The generic code that deals with register operands
> on gdb/stap-probe.c will call this method if it exists, passing the
> current parse information, the register name and its number.
> 
> The i386 method will then verify if the register size is greater or
> equal than the size reported by the stap probe (the "4@" part).  If it
> is, we're fine.  Otherwise, it will check if we're dealing with any of
> the "extendable" registers (like ax, bx, si, di, sp, etc.).  If we
> are, it will change the register name to include the "e" prefix.
> 
> I have tested the patch here in many scenarios, and it fixes Andrew's
> bug and also the regressions I mentioned before, on
> gdb.base/stap-probe.exp.  No regressions where found on other tests.
> 
> Comments?
> 
> gdb/ChangeLog:
> 2019-06-27  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh: Add 'stap_adjust_register'.
> 	* i386-tdep.c: Include '<unordered_set>'.
> 	(i386_stap_adjust_register): New function.
> 	(i386_elf_init_abi): Register 'i386_stap_adjust_register'.
> 	* stap-probe.c (stap_parse_register_operand): Call
> 	'gdbarch_stap_adjust_register'.
> ---
>  gdb/gdbarch.c    | 32 ++++++++++++++++++++++++++++++++
>  gdb/gdbarch.h    | 30 ++++++++++++++++++++++++++++++
>  gdb/gdbarch.sh   | 25 +++++++++++++++++++++++++
>  gdb/i386-tdep.c  | 29 +++++++++++++++++++++++++++++
>  gdb/stap-probe.c | 31 ++++++++++++++++++++++++++++---
>  5 files changed, 144 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index a0c169d74d..cc7d0ace66 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -324,6 +324,7 @@ struct gdbarch
>    const char * stap_gdb_register_suffix;
>    gdbarch_stap_is_single_operand_ftype *stap_is_single_operand;
>    gdbarch_stap_parse_special_token_ftype *stap_parse_special_token;
> +  gdbarch_stap_adjust_register_ftype *stap_adjust_register;
>    gdbarch_dtrace_parse_probe_argument_ftype *dtrace_parse_probe_argument;
>    gdbarch_dtrace_probe_is_enabled_ftype *dtrace_probe_is_enabled;
>    gdbarch_dtrace_enable_probe_ftype *dtrace_enable_probe;
> @@ -687,6 +688,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>    /* Skip verify of stap_gdb_register_suffix, invalid_p == 0 */
>    /* Skip verify of stap_is_single_operand, has predicate.  */
>    /* Skip verify of stap_parse_special_token, has predicate.  */
> +  /* Skip verify of stap_adjust_register, has predicate.  */
>    /* Skip verify of dtrace_parse_probe_argument, has predicate.  */
>    /* Skip verify of dtrace_probe_is_enabled, has predicate.  */
>    /* Skip verify of dtrace_enable_probe, has predicate.  */
> @@ -1396,6 +1398,12 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>    fprintf_unfiltered (file,
>                        "gdbarch_dump: stack_frame_destroyed_p = <%s>\n",
>                        host_address_to_string (gdbarch->stack_frame_destroyed_p));
> +  fprintf_unfiltered (file,
> +                      "gdbarch_dump: gdbarch_stap_adjust_register_p() = %d\n",
> +                      gdbarch_stap_adjust_register_p (gdbarch));
> +  fprintf_unfiltered (file,
> +                      "gdbarch_dump: stap_adjust_register = <%s>\n",
> +                      host_address_to_string (gdbarch->stap_adjust_register));
>    fprintf_unfiltered (file,
>                        "gdbarch_dump: stap_gdb_register_prefix = %s\n",
>                        pstring (gdbarch->stap_gdb_register_prefix));
> @@ -4515,6 +4523,30 @@ set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch,
>    gdbarch->stap_parse_special_token = stap_parse_special_token;
>  }
>  
> +int
> +gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  return gdbarch->stap_adjust_register != NULL;
> +}
> +
> +void
> +gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->stap_adjust_register != NULL);
> +  if (gdbarch_debug >= 2)
> +    fprintf_unfiltered (gdb_stdlog, "gdbarch_stap_adjust_register called\n");
> +  gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
> +}
> +
> +void
> +set_gdbarch_stap_adjust_register (struct gdbarch *gdbarch,
> +                                  gdbarch_stap_adjust_register_ftype stap_adjust_register)
> +{
> +  gdbarch->stap_adjust_register = stap_adjust_register;
> +}
> +
>  int
>  gdbarch_dtrace_parse_probe_argument_p (struct gdbarch *gdbarch)
>  {
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7ebd365a31..0857d2f302 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1350,6 +1350,36 @@ typedef int (gdbarch_stap_parse_special_token_ftype) (struct gdbarch *gdbarch, s
>  extern int gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, struct stap_parse_info *p);
>  extern void set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, gdbarch_stap_parse_special_token_ftype *stap_parse_special_token);
>  
> +/* Perform arch-dependent adjustments to a register name.
> +  
> +   In very specific situations, it may be necessary for the register
> +   name present in a SystemTap probe's argument to be handled in a
> +   special way.  For example, on i386, GCC may over-optimize the
> +   register allocation and use smaller registers than necessary.  In
> +   such cases, the client that is reading and evaluating the SystemTap
> +   probe (ourselves) will need to actually fetch values from the wider
> +   version of the register in question.
> +  
> +   To illustrate the example, consider the following probe argument
> +   (i386):
> +  
> +      4@%ax
> +  
> +   This argument says that its value can be found at the %ax register,
> +   which is a 16-bit register.  However, the argument's prefix says
> +   that its type is "uint32_t", which is 32-bit in size.  Therefore, in
> +   this case, GDB should actually fetch the probe's value from register
> +   %eax, not %ax.  In this scenario, this function would actually
> +   replace the register name from %ax to %eax.
> +  
> +   The rationale for this can be found at PR breakpoints/24541. */
> +
> +extern int gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch);
> +
> +typedef void (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum);
> +extern void gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum);
> +extern void set_gdbarch_stap_adjust_register (struct gdbarch *gdbarch, gdbarch_stap_adjust_register_ftype *stap_adjust_register);
> +
>  /* DTrace related functions.
>     The expression to compute the NARTGth+1 argument to a DTrace USDT probe.
>     NARG must be >= 0. */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 59493d8c21..f3d1bf489a 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1030,6 +1030,31 @@ M;int;stap_is_single_operand;const char *s;s
>  # parser), and should advance the buffer pointer (p->arg).
>  M;int;stap_parse_special_token;struct stap_parse_info *p;p
>  
> +# Perform arch-dependent adjustments to a register name.
> +#
> +# In very specific situations, it may be necessary for the register
> +# name present in a SystemTap probe's argument to be handled in a
> +# special way.  For example, on i386, GCC may over-optimize the
> +# register allocation and use smaller registers than necessary.  In
> +# such cases, the client that is reading and evaluating the SystemTap
> +# probe (ourselves) will need to actually fetch values from the wider
> +# version of the register in question.
> +#
> +# To illustrate the example, consider the following probe argument
> +# (i386):
> +#
> +#    4@%ax
> +#
> +# This argument says that its value can be found at the %ax register,
> +# which is a 16-bit register.  However, the argument's prefix says
> +# that its type is "uint32_t", which is 32-bit in size.  Therefore, in
> +# this case, GDB should actually fetch the probe's value from register
> +# %eax, not %ax.  In this scenario, this function would actually
> +# replace the register name from %ax to %eax.
> +#
> +# The rationale for this can be found at PR breakpoints/24541.
> +M;void;stap_adjust_register;struct stap_parse_info *p, std::string \&regname, int regnum;p, regname, regnum
> +
>  # DTrace related functions.
>  
>  # The expression to compute the NARTGth+1 argument to a DTrace USDT probe.
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index c542edf28a..00c1f8d749 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -64,6 +64,7 @@
>  #include "parser-defs.h"
>  #include <ctype.h>
>  #include <algorithm>
> +#include <unordered_set>
>  
>  /* Register names.  */
>  
> @@ -4385,6 +4386,32 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>    return 0;
>  }
>  
> +/* Implementation of 'gdbarch_stap_adjust_register', as defined in
> +   gdbarch.h.  */
> +
> +static void
> +i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
> +			   std::string &regname, int regnum)
> +{

I know I'm a bit late on this (the patch having been pushed already)
but, I didn't think non-const reference parameters were part of the
GDB coding standard, see:

   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead

I think this might be better as:

  static std::string
  i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
		              const std::string regname, int regnum)
  {

If you'd be happy with that then I'm happy to roll a patch at some
point next week.

Thanks,
Andrew


> +  static const std::unordered_set<std::string> reg_assoc
> +    = { "ax", "bx", "cx", "dx",
> +	"si", "di", "bp", "sp" };
> +
> +  if (register_size (gdbarch, regnum) >= TYPE_LENGTH (p->arg_type))
> +    {
> +      /* If we're dealing with a register whose size is greater or
> +	 equal than the size specified by the "[-]N@" prefix, then we
> +	 don't need to do anything.  */
> +      return;
> +    }
> +
> +  if (reg_assoc.find (regname) != reg_assoc.end ())
> +    {
> +      /* Use the extended version of the register.  */
> +      regname = "e" + regname;
> +    }
> +}
> +
>  
>  
>  /* gdbarch gnu_triplet_regexp method.  Both arches are acceptable as GDB always
> @@ -4433,6 +4460,8 @@ i386_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>  				      i386_stap_is_single_operand);
>    set_gdbarch_stap_parse_special_token (gdbarch,
>  					i386_stap_parse_special_token);
> +  set_gdbarch_stap_adjust_register (gdbarch,
> +				    i386_stap_adjust_register);
>  
>    set_gdbarch_in_indirect_branch_thunk (gdbarch,
>  					i386_in_indirect_branch_thunk);
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index e5a901b4bf..aa1c8144d8 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -762,13 +762,38 @@ stap_parse_register_operand (struct stap_parse_info *p)
>  	regname += gdb_reg_suffix;
>      }
>  
> +  int regnum = user_reg_map_name_to_regnum (gdbarch, regname.c_str (),
> +					    regname.size ());
> +
>    /* Is this a valid register name?  */
> -  if (user_reg_map_name_to_regnum (gdbarch,
> -				   regname.c_str (),
> -				   regname.size ()) == -1)
> +  if (regnum == -1)
>      error (_("Invalid register name `%s' on expression `%s'."),
>  	   regname.c_str (), p->saved_arg);
>  
> +  /* Check if there's any special treatment that the arch-specific
> +     code would like to perform on the register name.  */
> +  if (gdbarch_stap_adjust_register_p (gdbarch))
> +    {
> +      std::string oldregname = regname;
> +
> +      gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
> +
> +      if (regname != oldregname)
> +	{
> +	  /* This is just a check we perform to make sure that the
> +	     arch-dependent code has provided us with a valid
> +	     register name.  */
> +	  regnum = user_reg_map_name_to_regnum (gdbarch, regname.c_str (),
> +						regname.size ());
> +
> +	  if (regnum == -1)
> +	    internal_error (__FILE__, __LINE__,
> +			    _("Invalid register name '%s' after replacing it"
> +			      " (previous name was '%s')"),
> +			    regname.c_str (), oldregname.c_str ());
> +	}
> +    }
> +
>    write_exp_elt_opcode (&p->pstate, OP_REGISTER);
>    str.ptr = regname.c_str ();
>    str.length = regname.size ();
> -- 
> 2.21.0
>