From patchwork Wed Jun 26 22:00:31 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 33445 Received: (qmail 98046 invoked by alias); 26 Jun 2019 22:00:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 98026 invoked by uid 89); 26 Jun 2019 22:00:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, LIKELY_SPAM_BODY, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=HERE, Location X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Jun 2019 22:00:52 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1458FC1EB201; Wed, 26 Jun 2019 22:00:46 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3F013608C1; Wed, 26 Jun 2019 22:00:43 +0000 (UTC) From: Sergio Durigan Junior To: GDB Patches Cc: andrew.burgess@embecosm.com, fche@redhat.com, jakub@redhat.com, Sergio Durigan Junior Subject: [PATCH] Adjust i386 registers on SystemTap probes' arguments (PR breakpoints/24541) Date: Wed, 26 Jun 2019 18:00:31 -0400 Message-Id: <20190626220031.6302-1-sergiodj@redhat.com> MIME-Version: 1.0 X-IsSubscribed: yes 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) +{ + 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 ();