From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68376 invoked by alias); 29 Jun 2019 14:58:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 68356 invoked by uid 89); 29 Jun 2019 14:58:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,LIKELY_SPAM_BODY,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=Provider, target_board, H*RU:sk:host86-, non-const X-HELO: mail-wm1-f68.google.com Received: from mail-wm1-f68.google.com (HELO mail-wm1-f68.google.com) (209.85.128.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 29 Jun 2019 14:58:25 +0000 Received: by mail-wm1-f68.google.com with SMTP id c66so11757510wmf.0 for ; Sat, 29 Jun 2019 07:58:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=QsCDlWEi6lSP4anSuYWlDXvs1/BHZD7aymSHINLIDEo=; b=coY4BuxTjuSR7BQNciJoWVU5IiMyZIxD2BHXwVlJn9QWLNj49HL6ZREPVufA4YF0n5 rUCeOOJxA5EZ/ctgLOoGdFgMmaivVxAu9FriwJr2CmsaCsvLQfQl7yY8LlEc6wvD+1cb 8OJYVnYL0iMa6jJft0Ol+5LM//SheW1hMDH/xEp7Mswbc0GOa5Eu0/PfH0NhGBqgfcdH lbLyKHfdX7ZtZqYn3D6XI8kRGP2tUx5yadG9kPoB7G7o4rkdfyCTXWPBFn+jCWnauzwr iExy4sZ3v5TTczDz3upHHSGz8repbAjNtBWMaeHJpuUqkRF/qZswMrny3wP4J7KrlLu6 NbXQ== Return-Path: Received: from localhost (host86-128-12-99.range86-128.btcentralplus.com. [86.128.12.99]) by smtp.gmail.com with ESMTPSA id a3sm4180912wmb.35.2019.06.29.07.58.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 29 Jun 2019 07:58:22 -0700 (PDT) Date: Sat, 29 Jun 2019 14:58:00 -0000 From: Andrew Burgess To: Sergio Durigan Junior Cc: GDB Patches , fche@redhat.com, jakub@redhat.com Subject: Re: [PATCH] Adjust i386 registers on SystemTap probes' arguments (PR breakpoints/24541) Message-ID: <20190629145820.GR23204@embecosm.com> References: <20190626220031.6302-1-sergiodj@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190626220031.6302-1-sergiodj@redhat.com> X-Fortune: "I'd love to go out with you, but I've been scheduled for a karma transplant." X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-06/txt/msg00643.txt.bz2 * Sergio Durigan Junior [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 > > * gdbarch.c: Regenerate. > * gdbarch.h: Regenerate. > * gdbarch.sh: Add 'stap_adjust_register'. > * i386-tdep.c: Include ''. > (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 > #include > +#include > > /* 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 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 >