Adjust i386 registers on SystemTap probes' arguments (PR breakpoints/24541)

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>

	PR breakpoints/24541
	* 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'.

1 files changed, 32 insertions, 0 deletions

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index a0c169d..cc7d0ac 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.  */
@@ -1397,6 +1399,12 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *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));
   fprintf_unfiltered (file,
@@ -4516,6 +4524,30 @@ set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch,
 }
 
 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)
 {
   gdb_assert (gdbarch != NULL);