Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

When debugging two inferiors (or more) against gdbserver, and the
inferiors have different architectures, such as e.g., on x86_64
GNU/Linux and one inferior is 64-bit while the other is 32-bit, then
GDB can get confused with the different architectures in a couple
spots.

In both cases I ran into, GDB incorrectly ended up using the
architecture of whatever happens to be the selected inferior instead
of the architecture of some other given inferior:

#1 - When parsing the expedited registers in stop replies.

#2 - In the default implementation of the target_thread_architecture
     target method.

These resulted in instances of the infamous "Remote 'g' packet reply
is too long" error.  For example, with the test added in this commit,
we get:

~~~
  Continuing.
  Remote 'g' packet reply is too long (expected 440 bytes, got 816 bytes): ad064000000000000[snip]
  (gdb) FAIL: gdb.multi/multi-arch.exp: inf1 event with inf2 selected: continue to hello_loop

  c
  Continuing.
  Truncated register 50 in remote 'g' packet
  (gdb) PASS: gdb.multi/multi-arch.exp: inf2 event with inf1 selected: c
~~~

This commit fixes that.

gdb/ChangeLog:
2017-10-04  Pedro Alves  <palves@redhat.com>

	* remote.c (get_remote_arch_state): New 'gdbarch' parameter.  Use
	it instead of target_gdbarch.
	(get_remote_state, get_remote_packet_size): Adjust
	get_remote_arch_state calls, passing down target_gdbarch
	explicitly.
	(packet_reg_from_regnum, packet_reg_from_pnum): New parameter
	'gdbarch' and use it instead of target_gdbarch.
	(get_memory_packet_size): Adjust get_remote_arch_state calls,
	passing down target_gdbarch explicitly.
	(struct stop_reply) <arch>: New field.
	(remote_parse_stop_reply): Use the stopped thread's architecture,
	not the current inferior's.  Save the architecture in the
	stop_reply.
	(process_stop_reply): Use the stop reply's architecture.
	(process_g_packet, remote_fetch_registers)
	(remote_prepare_to_store, store_registers_using_G)
	(remote_store_registers): Adjust get_remote_arch_state calls,
	using the regcache's architecture.
	(remote_get_trace_status): Adjust get_remote_arch_state calls,
	passing down target_gdbarch explicitly.
	* spu-multiarch.c (spu_thread_architecture): Defer to the target
	beneath instead of calling target_gdbarch.
	* target.c (default_thread_architecture): Use the specified
	inferior's architecture, instead of the current inferior's
	architecture (via target_gdbarch).

gdb/testsuite/ChangeLog:
2017-10-04  Pedro Alves  <palves@redhat.com>

	* gdb.multi/hangout.c: Include <unistd.h>.
	(hangout_loop): New function.
	(main): Call alarm.  Call hangout_loop in a loop.
	* gdb.multi/hello.c: Include <unistd.h>.
	(hello_loop): New function.
	(main): Call alarm.  Call hangout_loop in a loop.
	* gdb.multi/multi-arch.exp: Test running to a breakpoint one
	inferior with the other selected.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9b00cd9c0225cbff50135a1e97439ad709626748..377be93d9f4608e5dade85d44f41295caa99f882 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,31 @@
+2017-10-04  Pedro Alves  <palves@redhat.com>
+
+       * remote.c (get_remote_arch_state): New 'gdbarch' parameter.  Use
+       it instead of target_gdbarch.
+       (get_remote_state, get_remote_packet_size): Adjust
+       get_remote_arch_state calls, passing down target_gdbarch
+       explicitly.
+       (packet_reg_from_regnum, packet_reg_from_pnum): New parameter
+       'gdbarch' and use it instead of target_gdbarch.
+       (get_memory_packet_size): Adjust get_remote_arch_state calls,
+       passing down target_gdbarch explicitly.
+       (struct stop_reply) <arch>: New field.
+       (remote_parse_stop_reply): Use the stopped thread's architecture,
+       not the current inferior's.  Save the architecture in the
+       stop_reply.
+       (process_stop_reply): Use the stop reply's architecture.
+       (process_g_packet, remote_fetch_registers)
+       (remote_prepare_to_store, store_registers_using_G)
+       (remote_store_registers): Adjust get_remote_arch_state calls,
+       using the regcache's architecture.
+       (remote_get_trace_status): Adjust get_remote_arch_state calls,
+       passing down target_gdbarch explicitly.
+       * spu-multiarch.c (spu_thread_architecture): Defer to the target
+       beneath instead of calling target_gdbarch.
+       * target.c (default_thread_architecture): Use the specified
+       inferior's architecture, instead of the current inferior's
+       architecture (via target_gdbarch).
+
 2017-10-04  Pedro Alves  <palves@redhat.com>
 
        * regcache.c (get_thread_arch_regcache): Remove null_ptid special

diff --git a/gdb/remote.c b/gdb/remote.c
index 8c47d0662cd282d0eba90c86f19268a20d2f3f71..3ceabcf1aadce7c335821e7be9e46e73619c72a1 100644 (file)
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -654,11 +654,11 @@ remote_get_noisy_reply ()
 static struct gdbarch_data *remote_gdbarch_data_handle;
 
 static struct remote_arch_state *
-get_remote_arch_state (void)
+get_remote_arch_state (struct gdbarch *gdbarch)
 {
-  gdb_assert (target_gdbarch () != NULL);
+  gdb_assert (gdbarch != NULL);
   return ((struct remote_arch_state *)
-         gdbarch_data (target_gdbarch (), remote_gdbarch_data_handle));
+         gdbarch_data (gdbarch, remote_gdbarch_data_handle));
 }
 
 /* Fetch the global remote target state.  */
@@ -671,7 +671,7 @@ get_remote_state (void)
      function which calls getpkt also needs to be mindful of changes
      to rs->buf, but this call limits the number of places which run
      into trouble.  */
-  get_remote_arch_state ();
+  get_remote_arch_state (target_gdbarch ());
 
   return get_remote_state_raw ();
 }
@@ -878,7 +878,7 @@ static long
 get_remote_packet_size (void)
 {
   struct remote_state *rs = get_remote_state ();
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  remote_arch_state *rsa = get_remote_arch_state (target_gdbarch ());
 
   if (rs->explicit_packet_size)
     return rs->explicit_packet_size;
@@ -887,9 +887,10 @@ get_remote_packet_size (void)
 }
 
 static struct packet_reg *
-packet_reg_from_regnum (struct remote_arch_state *rsa, long regnum)
+packet_reg_from_regnum (struct gdbarch *gdbarch, struct remote_arch_state *rsa,
+                       long regnum)
 {
-  if (regnum < 0 && regnum >= gdbarch_num_regs (target_gdbarch ()))
+  if (regnum < 0 && regnum >= gdbarch_num_regs (gdbarch))
     return NULL;
   else
     {
@@ -901,11 +902,12 @@ packet_reg_from_regnum (struct remote_arch_state *rsa, long regnum)
 }
 
 static struct packet_reg *
-packet_reg_from_pnum (struct remote_arch_state *rsa, LONGEST pnum)
+packet_reg_from_pnum (struct gdbarch *gdbarch, struct remote_arch_state *rsa,
+                     LONGEST pnum)
 {
   int i;
 
-  for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
+  for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
     {
       struct packet_reg *r = &rsa->regs[i];
 
@@ -1049,7 +1051,7 @@ static long
 get_memory_packet_size (struct memory_packet_config *config)
 {
   struct remote_state *rs = get_remote_state ();
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  remote_arch_state *rsa = get_remote_arch_state (target_gdbarch ());
 
   long what_they_get;
   if (config->fixed_p)
@@ -6364,6 +6366,9 @@ typedef struct stop_reply
 
   struct target_waitstatus ws;
 
+  /* The architecture associated with the expedited registers.  */
+  gdbarch *arch;
+
   /* Expedited registers.  This makes remote debugging a bit more
      efficient for those targets that provide critical registers as
      part of their normal status mechanism (as another roundtrip to
@@ -6838,7 +6843,7 @@ strprefix (const char *p, const char *pend, const char *prefix)
 static void
 remote_parse_stop_reply (char *buf, struct stop_reply *event)
 {
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  remote_arch_state *rsa = NULL;
   ULONGEST addr;
   const char *p;
   int skipregs = 0;
@@ -7018,9 +7023,47 @@ Packet: '%s'\n"),
                 reason.  */
              if (p_temp == p1)
                {
-                 struct packet_reg *reg = packet_reg_from_pnum (rsa, pnum);
+                 /* If we haven't parsed the event's thread yet, find
+                    it now, in order to find the architecture of the
+                    reported expedited registers.  */
+                 if (event->ptid == null_ptid)
+                   {
+                     const char *thr = strstr (p1 + 1, ";thread:");
+                     if (thr != NULL)
+                       event->ptid = read_ptid (thr + strlen (";thread:"),
+                                                NULL);
+                     else
+                       event->ptid = magic_null_ptid;
+                   }
+
+                 if (rsa == NULL)
+                   {
+                     inferior *inf = (event->ptid == null_ptid
+                                      ? NULL
+                                      : find_inferior_ptid (event->ptid));
+                     /* If this is the first time we learn anything
+                        about this process, skip the registers
+                        included in this packet, since we don't yet
+                        know which architecture to use to parse them.
+                        We'll determine the architecture later when
+                        we process the stop reply and retrieve the
+                        target description, via
+                        remote_notice_new_inferior ->
+                        post_create_inferior.  */
+                     if (inf == NULL)
+                       {
+                         p = strchrnul (p1 + 1, ';');
+                         p++;
+                         continue;
+                       }
+
+                     event->arch = inf->gdbarch;
+                     rsa = get_remote_arch_state (event->arch);
+                   }
+
+                 packet_reg *reg
+                   = packet_reg_from_pnum (event->arch, rsa, pnum);
                  cached_reg_t cached_reg;
-                 struct gdbarch *gdbarch = target_gdbarch ();
 
                  if (reg == NULL)
                    error (_("Remote sent bad register number %s: %s\n\
@@ -7029,13 +7072,13 @@ Packet: '%s'\n"),
 
                  cached_reg.num = reg->regnum;
                  cached_reg.data = (gdb_byte *)
-                   xmalloc (register_size (gdbarch, reg->regnum));
+                   xmalloc (register_size (event->arch, reg->regnum));
 
                  p = p1 + 1;
                  fieldsize = hex2bin (p, cached_reg.data,
-                                      register_size (gdbarch, reg->regnum));
+                                      register_size (event->arch, reg->regnum));
                  p += 2 * fieldsize;
-                 if (fieldsize < register_size (gdbarch, reg->regnum))
+                 if (fieldsize < register_size (event->arch, reg->regnum))
                    warning (_("Remote reply is too short: %s"), buf);
 
                  VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
@@ -7251,7 +7294,7 @@ process_stop_reply (struct stop_reply *stop_reply,
       if (stop_reply->regcache)
        {
          struct regcache *regcache
-           = get_thread_arch_regcache (ptid, target_gdbarch ());
+           = get_thread_arch_regcache (ptid, stop_reply->arch);
          cached_reg_t *reg;
          int ix;
 
@@ -7608,7 +7651,7 @@ process_g_packet (struct regcache *regcache)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct remote_state *rs = get_remote_state ();
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  remote_arch_state *rsa = get_remote_arch_state (gdbarch);
   int i, buf_len;
   char *p;
   char *regs;
@@ -7742,7 +7785,8 @@ static void
 remote_fetch_registers (struct target_ops *ops,
                        struct regcache *regcache, int regnum)
 {
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  remote_arch_state *rsa = get_remote_arch_state (gdbarch);
   int i;
 
   set_remote_traceframe ();
@@ -7750,7 +7794,7 @@ remote_fetch_registers (struct target_ops *ops,
 
   if (regnum >= 0)
     {
-      struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum);
+      packet_reg *reg = packet_reg_from_regnum (gdbarch, rsa, regnum);
 
       gdb_assert (reg != NULL);
 
@@ -7776,7 +7820,7 @@ remote_fetch_registers (struct target_ops *ops,
 
   fetch_registers_using_g (regcache);
 
-  for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
+  for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
     if (!rsa->regs[i].in_g_packet)
       if (!fetch_register_using_p (regcache, &rsa->regs[i]))
        {
@@ -7792,7 +7836,7 @@ remote_fetch_registers (struct target_ops *ops,
 static void
 remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
 {
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  remote_arch_state *rsa = get_remote_arch_state (regcache->arch ());
   int i;
 
   /* Make sure the entire registers array is valid.  */
@@ -7858,7 +7902,7 @@ static void
 store_registers_using_G (const struct regcache *regcache)
 {
   struct remote_state *rs = get_remote_state ();
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  remote_arch_state *rsa = get_remote_arch_state (regcache->arch ());
   gdb_byte *regs;
   char *p;
 
@@ -7897,7 +7941,8 @@ static void
 remote_store_registers (struct target_ops *ops,
                        struct regcache *regcache, int regnum)
 {
-  struct remote_arch_state *rsa = get_remote_arch_state ();
+  struct gdbarch *gdbarch = regcache->arch ();
+  remote_arch_state *rsa = get_remote_arch_state (gdbarch);
   int i;
 
   set_remote_traceframe ();
@@ -7905,7 +7950,7 @@ remote_store_registers (struct target_ops *ops,
 
   if (regnum >= 0)
     {
-      struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum);
+      packet_reg *reg = packet_reg_from_regnum (gdbarch, rsa, regnum);
 
       gdb_assert (reg != NULL);
 
@@ -7929,7 +7974,7 @@ remote_store_registers (struct target_ops *ops,
 
   store_registers_using_G (regcache);
 
-  for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
+  for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
     if (!rsa->regs[i].in_g_packet)
       if (!store_register_using_P (regcache, &rsa->regs[i]))
        /* See above for why we do not issue an error here.  */
@@ -12697,7 +12742,8 @@ remote_get_trace_status (struct target_ops *self, struct trace_status *ts)
   if (packet_support (PACKET_qTStatus) == PACKET_DISABLE)
     return -1;
 
-  trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
+  trace_regblock_size
+    = get_remote_arch_state (target_gdbarch ())->sizeof_g_packet;
 
   putpkt ("qTStatus");
 

diff --git a/gdb/spu-multiarch.c b/gdb/spu-multiarch.c
index a935a72c3058d5ccd9e89adb4cb0288e93be6064..e521c7e6ac2d9d710b21dc2c55a7ae9670c49eaa 100644 (file)
--- a/gdb/spu-multiarch.c
+++ b/gdb/spu-multiarch.c
@@ -121,7 +121,8 @@ spu_thread_architecture (struct target_ops *ops, ptid_t ptid)
   if (parse_spufs_run (ptid, &spufs_fd, &spufs_addr))
     return spu_gdbarch (spufs_fd);
 
-  return target_gdbarch ();
+  target_ops *beneath = find_target_beneath (ops);
+  return beneath->to_thread_architecture (beneath, ptid);
 }
 
 /* Override the to_region_ok_for_hw_watchpoint routine.  */

diff --git a/gdb/target.c b/gdb/target.c
index aded0ba34f12b697a3a89bf4d156dc0d35553f21..f2dccd23ced7034a35e843a7a6527bed5d993945 100644 (file)
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3137,7 +3137,9 @@ default_watchpoint_addr_within_range (struct target_ops *target,
 static struct gdbarch *
 default_thread_architecture (struct target_ops *ops, ptid_t ptid)
 {
-  return target_gdbarch ();
+  inferior *inf = find_inferior_ptid (ptid);
+  gdb_assert (inf != NULL);
+  return inf->gdbarch;
 }
 
 static int

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index ddae8e015a4caaa7c1668ab6a5553fa38449be81..73cf40b83942bbebe67f93c364c1ec0828374ed5 100644 (file)
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,14 @@
+2017-10-04  Pedro Alves  <palves@redhat.com>
+
+       * gdb.multi/hangout.c: Include <unistd.h>.
+       (hangout_loop): New function.
+       (main): Call alarm.  Call hangout_loop in a loop.
+       * gdb.multi/hello.c: Include <unistd.h>.
+       (hello_loop): New function.
+       (main): Call alarm.  Call hangout_loop in a loop.
+       * gdb.multi/multi-arch.exp: Test running to a breakpoint one
+       inferior with the other selected.
+
 2017-10-04  Simon Marchi  <simon.marchi@ericsson.com>
 
        * gdb.mi/list-thread-groups-available.exp: New file.

diff --git a/gdb/testsuite/gdb.multi/hangout.c b/gdb/testsuite/gdb.multi/hangout.c
index 44dfba6893711f4c50b56ed0bfd62868c11ca90b..7266a188f859a711b8d8805afefa9a5a9c56c7ec 100644 (file)
--- a/gdb/testsuite/gdb.multi/hangout.c
+++ b/gdb/testsuite/gdb.multi/hangout.c
@@ -16,14 +16,28 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <stdio.h>
+#include <unistd.h>
+
+static void
+hangout_loop (void)
+{
+}
 
 int
 main(int argc, char *argv[])
 {
   int i;
 
+  alarm (30);
+
   for (i = 0; i < argc; ++i)
     {
       printf("Arg %d is %s\n", i, argv[i]);
     }
+
+  while (1)
+    {
+      hangout_loop ();
+      usleep (20);
+    }
 }

diff --git a/gdb/testsuite/gdb.multi/hello.c b/gdb/testsuite/gdb.multi/hello.c
index 2c5bee99b5cb6e6e228784dbe56f404f8dad5b6e..7cc5f3c32bfa9e909202fa3b6ce2347e81c535b3 100644 (file)
--- a/gdb/testsuite/gdb.multi/hello.c
+++ b/gdb/testsuite/gdb.multi/hello.c
@@ -16,6 +16,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <stdlib.h>
+#include <unistd.h>
 
 short hglob = 1;
 
@@ -37,15 +38,27 @@ hello(int x)
   return x + 45;
 }
 
+static void
+hello_loop (void)
+{
+}
+
 int
 main()
 {
   int tmpx;
 
+  alarm (30);
+
   bar();
   tmpx = hello(glob);
   commonfun();
   glob = tmpx;
   commonfun();
-}
 
+  while (1)
+    {
+      hello_loop ();
+      usleep (20);
+    }
+}

diff --git a/gdb/testsuite/gdb.multi/multi-arch.exp b/gdb/testsuite/gdb.multi/multi-arch.exp
index d73b4f7aeecf069aad900f8a4acff71a6aed568e..733afa098c03c8de71eb67a5c54bd88587cde1a9 100644 (file)
--- a/gdb/testsuite/gdb.multi/multi-arch.exp
+++ b/gdb/testsuite/gdb.multi/multi-arch.exp
@@ -96,3 +96,27 @@ if ![runto_main] then {
 
 gdb_test "info inferiors" \
     "Executable.*${exec1}.*${exec2}.*"
+
+# Now select inferior 2, and trigger an event in inferior 1.  This
+# tries to check that GDB doesn't incorrectly uses the architecture of
+# inferior 2 when parsing the expedited registers in a stop reply for
+# inferior 1 (when remote debugging).
+
+gdb_test_no_output "set schedule-multiple on"
+
+with_test_prefix "inf1 event with inf2 selected" {
+    gdb_test "inferior 2" "Switching to inferior 2.*thread 2\.1.*main.*${srcfile2}.*"
+    gdb_test "b hello_loop" "Breakpoint .* at .*${srcfile1}.*"
+    gdb_test "c" " hello_loop.*" "continue to hello_loop"
+}
+
+delete_breakpoints
+
+# Same, but the other way around: select inferior 1 and trigger an
+# event in inferior 2.
+
+with_test_prefix "inf2 event with inf1 selected" {
+    gdb_test "inferior 1" "Switching to inferior 1.*thread 1\.1.*hello_loop.*${srcfile1}.*"
+    gdb_test "b hangout_loop" "Breakpoint .* at .*${srcfile2}.*"
+    gdb_test "c" " hangout_loop.*" "continue to hangout_loop"
+}