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

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

[Sergio Durigan Junior]

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)
+{
+  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;
+    }
+}
+
 \f
 
 /* 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

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

[Sergio Durigan Junior]

On Wednesday, June 26 2019, I wrote:

> 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'.

Just noticed that I forgot to include the PR number in the ChangeLog
entry.  I've just done that locally.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> gdb/ChangeLog:
Sergio> 2019-06-27  Sergio Durigan Junior  <sergiodj@redhat.com>

Sergio> 	* gdbarch.c: Regenerate.
Sergio> 	* gdbarch.h: Regenerate.
Sergio> 	* gdbarch.sh: Add 'stap_adjust_register'.
Sergio> 	* i386-tdep.c: Include '<unordered_set>'.
Sergio> 	(i386_stap_adjust_register): New function.
Sergio> 	(i386_elf_init_abi): Register 'i386_stap_adjust_register'.
Sergio> 	* stap-probe.c (stap_parse_register_operand): Call
Sergio> 	'gdbarch_stap_adjust_register'.

Thank you for the patch and the excellent explanation.

This is ok.

For a while I was updating some SystemTap wiki page to link to these
kinds of bugs, so that anybody else interested in supporting these
probes had a list of oddities to work from.  This one would seem to
qualify for that.

Tom

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

[Sergio Durigan Junior]

On Friday, June 28 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> gdb/ChangeLog:
> Sergio> 2019-06-27  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> Sergio> 	* gdbarch.c: Regenerate.
> Sergio> 	* gdbarch.h: Regenerate.
> Sergio> 	* gdbarch.sh: Add 'stap_adjust_register'.
> Sergio> 	* i386-tdep.c: Include '<unordered_set>'.
> Sergio> 	(i386_stap_adjust_register): New function.
> Sergio> 	(i386_elf_init_abi): Register 'i386_stap_adjust_register'.
> Sergio> 	* stap-probe.c (stap_parse_register_operand): Call
> Sergio> 	'gdbarch_stap_adjust_register'.
>
> Thank you for the patch and the excellent explanation.
>
> This is ok.

Thank you, Tom.

> For a while I was updating some SystemTap wiki page to link to these
> kinds of bugs, so that anybody else interested in supporting these
> probes had a list of oddities to work from.  This one would seem to
> qualify for that.

Good point.  I will edit
https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation and
include this bug for reference.

Pushed: 7d7571f0c14b4673ca95f6dc31d6f07d429e6697

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Andrew Burgess]

* 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;
> +    }
> +}
> +
>  \f
>  
>  /* 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
> 

[PATCH] gdb: Remove a non-const reference parameter

[Andrew Burgess]

Non-const reference parameter should be avoided according to the GDB
coding standard:

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

This commit updates the gdbarch method gdbarch_stap_adjust_register,
and the one implementation i386_stap_adjust_register to avoid using a
non-const reference parameter.

I've also removed the kfail from the testsuite for bug 24541, as this
issue is now resolved.

gdb/ChangeLog:

	PR breakpoints/24541
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbarch.sh: Adjust return type and parameter types for
	'stap_adjust_register'.
	(i386_stap_adjust_register): Adjust signature and return new
	register name.
	* stap-probe.c (stap_parse_register_operand): Adjust use of
	'gdbarch_stap_adjust_register'.

gdb/testsuite/ChangeLog:

	PR breakpoints/24541
	* gdb.mi/mi-catch-cpp-exceptions.exp: Remove kfail due to 24541.
---
 gdb/ChangeLog                                    | 12 ++++++++++++
 gdb/gdbarch.c                                    |  6 +++---
 gdb/gdbarch.h                                    |  4 ++--
 gdb/gdbarch.sh                                   |  2 +-
 gdb/i386-tdep.c                                  | 12 ++++++++----
 gdb/stap-probe.c                                 | 15 ++++++++-------
 gdb/testsuite/ChangeLog                          |  5 +++++
 gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp | 22 +---------------------
 8 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index cc7d0ace66c..725b98fcd9f 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -4530,14 +4530,14 @@ gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch)
   return gdbarch->stap_adjust_register != NULL;
 }
 
-void
-gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum)
+std::string
+gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const 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);
+  return gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
 }
 
 void
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 0857d2f3027..c3556d38419 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1376,8 +1376,8 @@ extern void set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, gdbar
 
 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);
+typedef std::string (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
+extern std::string gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const 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.
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index f3d1bf489ac..2f9fbbc56cd 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1053,7 +1053,7 @@ M;int;stap_parse_special_token;struct stap_parse_info *p;p
 # 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
+M;std::string;stap_adjust_register;struct stap_parse_info *p, const std::string \&regname, int regnum;p, regname, regnum
 
 # DTrace related functions.
 
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 00c1f8d7499..b956604178f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -4389,9 +4389,9 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 /* Implementation of 'gdbarch_stap_adjust_register', as defined in
    gdbarch.h.  */
 
-static void
+static std::string
 i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
-			   std::string &regname, int regnum)
+			   const std::string &regname, int regnum)
 {
   static const std::unordered_set<std::string> reg_assoc
     = { "ax", "bx", "cx", "dx",
@@ -4402,14 +4402,18 @@ i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
       /* 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;
+      return regname;
     }
 
   if (reg_assoc.find (regname) != reg_assoc.end ())
     {
       /* Use the extended version of the register.  */
-      regname = "e" + regname;
+      std::string tmp = "e" + regname;
+      return tmp;
     }
+
+  /* Use the requested register.  */
+  return regname;
 }
 
 \f
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index aa1c8144d8a..747bbcfb8c1 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -774,23 +774,24 @@ stap_parse_register_operand (struct stap_parse_info *p)
      code would like to perform on the register name.  */
   if (gdbarch_stap_adjust_register_p (gdbarch))
     {
-      std::string oldregname = regname;
+      std::string newregname
+	= gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
 
-      gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
-
-      if (regname != oldregname)
+      if (regname != newregname)
 	{
 	  /* 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 ());
+	  regnum = user_reg_map_name_to_regnum (gdbarch, newregname.c_str (),
+						newregname.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 ());
+			    newregname.c_str (), regname.c_str ());
+
+	  regname = newregname;
 	}
     }
 
diff --git a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
index fa5b11e3e58..4b87e4b62ba 100644
--- a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
+++ b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
@@ -130,27 +130,7 @@ with_test_prefix "all with invalid regexp" {
     setup_catchpoint "throw" "-r blahblah"
     setup_catchpoint "rethrow" "-r woofwoof"
     setup_catchpoint "catch" "-r miowmiow"
-
-    # Would like to use 'continue_to_breakpoint_in_main' here, if
-    # there wasn't a bug that requires a use of kfail.
-
-    mi_send_resuming_command "exec-continue" \
-	"exec-continue"
-    set testname "run until breakpoint in main"
-    gdb_expect {
-	-re "could not find minimal symbol for typeinfo address.*$mi_gdb_prompt$" {
-	    kfail "gdb/24541" "${testname}"
-	}
-	-re "\\*stopped,bkptno=\"$decimal\",reason=\"breakpoint-hit\",disp=\"keep\".*func=\"__cxa_throw\".*$mi_gdb_prompt$" {
-	    kfail "gdb/24541" "${testname}"
-	}
-	-re "\\*stopped,reason=\"breakpoint-hit\".*func=\"main\".*line=\"${main_lineno}\".*$mi_gdb_prompt$" {
-	    pass "${testname}"
-	}
-	timeout {
-	    fail "${testname} (timeout)"
-	}
-    }
+    continue_to_breakpoint_in_main
 }
 
 # Now check that all of the commands with a regexp that does match,
-- 
2.14.5

Re: [PATCH] gdb: Remove a non-const reference parameter

[Simon Marchi]

On 2019-07-02 11:21 a.m., Andrew Burgess wrote:
> Non-const reference parameter should be avoided according to the GDB
> coding standard:
> 
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
> 
> This commit updates the gdbarch method gdbarch_stap_adjust_register,
> and the one implementation i386_stap_adjust_register to avoid using a
> non-const reference parameter.
> 
> I've also removed the kfail from the testsuite for bug 24541, as this
> issue is now resolved.

Hi Andrew,

So I was the one who added this rule to the guide.  When I asked for opinions and
didn't get any, I thought it was because it wasn't very controversial and chose to add it.

Since then, Pedro mentioned a few times he didn't agree with it.

I'd like to know what others think, should we keep it (and enforce it) or remove it?

Simon

Re: [PATCH] gdb: Remove a non-const reference parameter

[Sergio Durigan Junior]

On Tuesday, July 02 2019, Andrew Burgess wrote:

> Non-const reference parameter should be avoided according to the GDB
> coding standard:
>
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
>
> This commit updates the gdbarch method gdbarch_stap_adjust_register,
> and the one implementation i386_stap_adjust_register to avoid using a
> non-const reference parameter.
>
> I've also removed the kfail from the testsuite for bug 24541, as this
> issue is now resolved.

Thanks, Andrew (and sorry for the delay; yesterday was a holiday here).

This is OK.

> gdb/ChangeLog:
>
> 	PR breakpoints/24541
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh: Adjust return type and parameter types for
> 	'stap_adjust_register'.
> 	(i386_stap_adjust_register): Adjust signature and return new
> 	register name.
> 	* stap-probe.c (stap_parse_register_operand): Adjust use of
> 	'gdbarch_stap_adjust_register'.
>
> gdb/testsuite/ChangeLog:
>
> 	PR breakpoints/24541
> 	* gdb.mi/mi-catch-cpp-exceptions.exp: Remove kfail due to 24541.
> ---
>  gdb/ChangeLog                                    | 12 ++++++++++++
>  gdb/gdbarch.c                                    |  6 +++---
>  gdb/gdbarch.h                                    |  4 ++--
>  gdb/gdbarch.sh                                   |  2 +-
>  gdb/i386-tdep.c                                  | 12 ++++++++----
>  gdb/stap-probe.c                                 | 15 ++++++++-------
>  gdb/testsuite/ChangeLog                          |  5 +++++
>  gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp | 22 +---------------------
>  8 files changed, 40 insertions(+), 38 deletions(-)
>
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index cc7d0ace66c..725b98fcd9f 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4530,14 +4530,14 @@ gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch)
>    return gdbarch->stap_adjust_register != NULL;
>  }
>  
> -void
> -gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum)
> +std::string
> +gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const 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);
> +  return gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
>  }
>  
>  void
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 0857d2f3027..c3556d38419 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1376,8 +1376,8 @@ extern void set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, gdbar
>  
>  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);
> +typedef std::string (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
> +extern std::string gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const 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.
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index f3d1bf489ac..2f9fbbc56cd 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1053,7 +1053,7 @@ M;int;stap_parse_special_token;struct stap_parse_info *p;p
>  # 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
> +M;std::string;stap_adjust_register;struct stap_parse_info *p, const std::string \&regname, int regnum;p, regname, regnum
>  
>  # DTrace related functions.
>  
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 00c1f8d7499..b956604178f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -4389,9 +4389,9 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>  /* Implementation of 'gdbarch_stap_adjust_register', as defined in
>     gdbarch.h.  */
>  
> -static void
> +static std::string
>  i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
> -			   std::string &regname, int regnum)
> +			   const std::string &regname, int regnum)
>  {
>    static const std::unordered_set<std::string> reg_assoc
>      = { "ax", "bx", "cx", "dx",
> @@ -4402,14 +4402,18 @@ i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
>        /* 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;
> +      return regname;
>      }
>  
>    if (reg_assoc.find (regname) != reg_assoc.end ())
>      {
>        /* Use the extended version of the register.  */
> -      regname = "e" + regname;
> +      std::string tmp = "e" + regname;
> +      return tmp;
>      }
> +
> +  /* Use the requested register.  */
> +  return regname;
>  }
>  
>  \f
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index aa1c8144d8a..747bbcfb8c1 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -774,23 +774,24 @@ stap_parse_register_operand (struct stap_parse_info *p)
>       code would like to perform on the register name.  */
>    if (gdbarch_stap_adjust_register_p (gdbarch))
>      {
> -      std::string oldregname = regname;
> +      std::string newregname
> +	= gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
>  
> -      gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
> -
> -      if (regname != oldregname)
> +      if (regname != newregname)
>  	{
>  	  /* 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 ());
> +	  regnum = user_reg_map_name_to_regnum (gdbarch, newregname.c_str (),
> +						newregname.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 ());
> +			    newregname.c_str (), regname.c_str ());
> +
> +	  regname = newregname;
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> index fa5b11e3e58..4b87e4b62ba 100644
> --- a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> +++ b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> @@ -130,27 +130,7 @@ with_test_prefix "all with invalid regexp" {
>      setup_catchpoint "throw" "-r blahblah"
>      setup_catchpoint "rethrow" "-r woofwoof"
>      setup_catchpoint "catch" "-r miowmiow"
> -
> -    # Would like to use 'continue_to_breakpoint_in_main' here, if
> -    # there wasn't a bug that requires a use of kfail.
> -
> -    mi_send_resuming_command "exec-continue" \
> -	"exec-continue"
> -    set testname "run until breakpoint in main"
> -    gdb_expect {
> -	-re "could not find minimal symbol for typeinfo address.*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,bkptno=\"$decimal\",reason=\"breakpoint-hit\",disp=\"keep\".*func=\"__cxa_throw\".*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,reason=\"breakpoint-hit\".*func=\"main\".*line=\"${main_lineno}\".*$mi_gdb_prompt$" {
> -	    pass "${testname}"
> -	}
> -	timeout {
> -	    fail "${testname} (timeout)"
> -	}
> -    }
> +    continue_to_breakpoint_in_main
>  }
>  
>  # Now check that all of the commands with a regexp that does match,
> -- 
> 2.14.5

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] gdb: Remove a non-const reference parameter

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Non-const reference parameter should be avoided according to the GDB
Andrew> coding standard:

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

Andrew> This commit updates the gdbarch method gdbarch_stap_adjust_register,
Andrew> and the one implementation i386_stap_adjust_register to avoid using a
Andrew> non-const reference parameter.

Thanks for doing this.  I probably should have pushed back on the
original patch a little for this reason.

Andrew> +      std::string tmp = "e" + regname;
Andrew> +      return tmp;

This can just be `return "e" + regname' I think.

Looks ok otherwise.

Tom

Re: [PATCH] gdb: Remove a non-const reference parameter

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> So I was the one who added this rule to the guide.  When I asked for opinions and
Simon> didn't get any, I thought it was because it wasn't very controversial and chose to add it.

Simon> Since then, Pedro mentioned a few times he didn't agree with it.

Simon> I'd like to know what others think, should we keep it (and enforce it) or remove it?

Pedro raised good points on this topic, but in this particular case I
think the code is clearer without modifying the parameter.

Tom

Re: [PATCH] gdb: Remove a non-const reference parameter

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-07-02 16:21:19 +0100]:

> Non-const reference parameter should be avoided according to the GDB
> coding standard:
> 
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
> 
> This commit updates the gdbarch method gdbarch_stap_adjust_register,
> and the one implementation i386_stap_adjust_register to avoid using a
> non-const reference parameter.
> 
> I've also removed the kfail from the testsuite for bug 24541, as this
> issue is now resolved.
> 
> gdb/ChangeLog:
> 
> 	PR breakpoints/24541
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh: Adjust return type and parameter types for
> 	'stap_adjust_register'.
> 	(i386_stap_adjust_register): Adjust signature and return new
> 	register name.
> 	* stap-probe.c (stap_parse_register_operand): Adjust use of
> 	'gdbarch_stap_adjust_register'.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR breakpoints/24541
> 	* gdb.mi/mi-catch-cpp-exceptions.exp: Remove kfail due to
> 	24541.

I went ahead and pushed this patch, after making the improvement Tom
suggested.

There didn't seem to be a rush of voices suggesting the non-const rule
be dropped.  We can always revert this if the rule is later removed.

Thanks,
Andrew



> ---
>  gdb/ChangeLog                                    | 12 ++++++++++++
>  gdb/gdbarch.c                                    |  6 +++---
>  gdb/gdbarch.h                                    |  4 ++--
>  gdb/gdbarch.sh                                   |  2 +-
>  gdb/i386-tdep.c                                  | 12 ++++++++----
>  gdb/stap-probe.c                                 | 15 ++++++++-------
>  gdb/testsuite/ChangeLog                          |  5 +++++
>  gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp | 22 +---------------------
>  8 files changed, 40 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index cc7d0ace66c..725b98fcd9f 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4530,14 +4530,14 @@ gdbarch_stap_adjust_register_p (struct gdbarch *gdbarch)
>    return gdbarch->stap_adjust_register != NULL;
>  }
>  
> -void
> -gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, std::string &regname, int regnum)
> +std::string
> +gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const 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);
> +  return gdbarch->stap_adjust_register (gdbarch, p, regname, regnum);
>  }
>  
>  void
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 0857d2f3027..c3556d38419 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1376,8 +1376,8 @@ extern void set_gdbarch_stap_parse_special_token (struct gdbarch *gdbarch, gdbar
>  
>  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);
> +typedef std::string (gdbarch_stap_adjust_register_ftype) (struct gdbarch *gdbarch, struct stap_parse_info *p, const std::string &regname, int regnum);
> +extern std::string gdbarch_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p, const 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.
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index f3d1bf489ac..2f9fbbc56cd 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1053,7 +1053,7 @@ M;int;stap_parse_special_token;struct stap_parse_info *p;p
>  # 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
> +M;std::string;stap_adjust_register;struct stap_parse_info *p, const std::string \&regname, int regnum;p, regname, regnum
>  
>  # DTrace related functions.
>  
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 00c1f8d7499..b956604178f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -4389,9 +4389,9 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>  /* Implementation of 'gdbarch_stap_adjust_register', as defined in
>     gdbarch.h.  */
>  
> -static void
> +static std::string
>  i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
> -			   std::string &regname, int regnum)
> +			   const std::string &regname, int regnum)
>  {
>    static const std::unordered_set<std::string> reg_assoc
>      = { "ax", "bx", "cx", "dx",
> @@ -4402,14 +4402,18 @@ i386_stap_adjust_register (struct gdbarch *gdbarch, struct stap_parse_info *p,
>        /* 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;
> +      return regname;
>      }
>  
>    if (reg_assoc.find (regname) != reg_assoc.end ())
>      {
>        /* Use the extended version of the register.  */
> -      regname = "e" + regname;
> +      std::string tmp = "e" + regname;
> +      return tmp;
>      }
> +
> +  /* Use the requested register.  */
> +  return regname;
>  }
>  
>  \f
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index aa1c8144d8a..747bbcfb8c1 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -774,23 +774,24 @@ stap_parse_register_operand (struct stap_parse_info *p)
>       code would like to perform on the register name.  */
>    if (gdbarch_stap_adjust_register_p (gdbarch))
>      {
> -      std::string oldregname = regname;
> +      std::string newregname
> +	= gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
>  
> -      gdbarch_stap_adjust_register (gdbarch, p, regname, regnum);
> -
> -      if (regname != oldregname)
> +      if (regname != newregname)
>  	{
>  	  /* 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 ());
> +	  regnum = user_reg_map_name_to_regnum (gdbarch, newregname.c_str (),
> +						newregname.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 ());
> +			    newregname.c_str (), regname.c_str ());
> +
> +	  regname = newregname;
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> index fa5b11e3e58..4b87e4b62ba 100644
> --- a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> +++ b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
> @@ -130,27 +130,7 @@ with_test_prefix "all with invalid regexp" {
>      setup_catchpoint "throw" "-r blahblah"
>      setup_catchpoint "rethrow" "-r woofwoof"
>      setup_catchpoint "catch" "-r miowmiow"
> -
> -    # Would like to use 'continue_to_breakpoint_in_main' here, if
> -    # there wasn't a bug that requires a use of kfail.
> -
> -    mi_send_resuming_command "exec-continue" \
> -	"exec-continue"
> -    set testname "run until breakpoint in main"
> -    gdb_expect {
> -	-re "could not find minimal symbol for typeinfo address.*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,bkptno=\"$decimal\",reason=\"breakpoint-hit\",disp=\"keep\".*func=\"__cxa_throw\".*$mi_gdb_prompt$" {
> -	    kfail "gdb/24541" "${testname}"
> -	}
> -	-re "\\*stopped,reason=\"breakpoint-hit\".*func=\"main\".*line=\"${main_lineno}\".*$mi_gdb_prompt$" {
> -	    pass "${testname}"
> -	}
> -	timeout {
> -	    fail "${testname} (timeout)"
> -	}
> -    }
> +    continue_to_breakpoint_in_main
>  }
>  
>  # Now check that all of the commands with a regexp that does match,
> -- 
> 2.14.5
>