This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Adjust i386 registers on SystemTap probes' arguments (PR breakpoints/24541)
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, fche at redhat dot com, jakub at redhat dot com
- Date: Sat, 29 Jun 2019 15:58:20 +0100
- Subject: Re: [PATCH] Adjust i386 registers on SystemTap probes' arguments (PR breakpoints/24541)
- References: <20190626220031.6302-1-sergiodj@redhat.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 ®name, 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 ®name, int regnum);
> +extern void gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string ®name, 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 \®name, 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 ®name, 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
>