[PATCH 0/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[PATCH 0/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

This series fixes some problems I uncovered while working on my
multi-target branch [1].

Namely 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, resulting in instances of the infamous "Remote 'g' packet reply
is too long" error.  For example, with the test added in patch #3, 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
~~~

The fix I was aiming for is in patch #3.  The other two patches
preemptively fix problems that patch #3 alone would cause.  They're
sorted this way to avoid introducing temporary regressions.

[1] - https://github.com/palves/gdb/tree/palves/multi-target

Pedro Alves (3):
  Redesign mock environment for gdbarch selftests
  Reimplement support for "maint print registers" with no running
    inferior yet
  Fix "Remote 'g' packet reply is too long" problems with multiple
    inferiors

 gdb/frame.c                            |  17 ------
 gdb/frame.h                            |   8 ---
 gdb/gdbarch-selftests.c                | 105 +++++++++++++++++++++++++++------
 gdb/progspace.c                        |  12 ----
 gdb/progspace.h                        |  11 ++++
 gdb/regcache.c                         |  33 ++++++-----
 gdb/regcache.h                         |  10 ----
 gdb/remote.c                           |  92 +++++++++++++++++++++--------
 gdb/target.c                           |  14 ++---
 gdb/target.h                           |  10 ++++
 gdb/testsuite/gdb.multi/hangout.c      |  14 +++++
 gdb/testsuite/gdb.multi/hello.c        |  15 ++++-
 gdb/testsuite/gdb.multi/multi-arch.exp |  24 ++++++++
 13 files changed, 250 insertions(+), 115 deletions(-)

-- 
2.5.5

[PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

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-02  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.
	(remote_parse_stop_reply): Use the stopped thread's architecture,
	not the current inferior's.
	(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.
	* target.c (default_thread_architecture): Use the sepecified
	inferior's architecture, instead of the current inferior's
	architecture (via target_gdbarch).

gdb/testsuite/ChangeLog:
2017-10-02  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.
---
 gdb/remote.c                           | 92 ++++++++++++++++++++++++----------
 gdb/target.c                           |  4 +-
 gdb/testsuite/gdb.multi/hangout.c      | 14 ++++++
 gdb/testsuite/gdb.multi/hello.c        | 15 +++++-
 gdb/testsuite/gdb.multi/multi-arch.exp | 24 +++++++++
 5 files changed, 121 insertions(+), 28 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 54d810f..b6a81a2 100644
--- 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 ();
+  struct 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 ();
+  struct remote_arch_state *rsa = get_remote_arch_state (target_gdbarch ());
 
   long what_they_get;
   if (config->fixed_p)
@@ -6835,7 +6837,8 @@ 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;
+  struct gdbarch *reply_arch = NULL;
   ULONGEST addr;
   const char *p;
   int skipregs = 0;
@@ -7015,9 +7018,43 @@ 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.  */
+		      if (inf == NULL)
+			{
+			  p = strchrnul (p1 + 1, ';');
+			  p++;
+			  continue;
+			}
+
+		      reply_arch = inf->gdbarch;
+		      rsa = get_remote_arch_state (reply_arch);
+		    }
+
+		  struct packet_reg *reg = packet_reg_from_pnum (reply_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\
@@ -7026,13 +7063,13 @@ Packet: '%s'\n"),
 
 		  cached_reg.num = reg->regnum;
 		  cached_reg.data = (gdb_byte *)
-		    xmalloc (register_size (gdbarch, reg->regnum));
+		    xmalloc (register_size (reply_arch, reg->regnum));
 
 		  p = p1 + 1;
 		  fieldsize = hex2bin (p, cached_reg.data,
-				       register_size (gdbarch, reg->regnum));
+				       register_size (reply_arch, reg->regnum));
 		  p += 2 * fieldsize;
-		  if (fieldsize < register_size (gdbarch, reg->regnum))
+		  if (fieldsize < register_size (reply_arch, reg->regnum))
 		    warning (_("Remote reply is too short: %s"), buf);
 
 		  VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
@@ -7605,7 +7642,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 ();
+  struct remote_arch_state *rsa = get_remote_arch_state (gdbarch);
   int i, buf_len;
   char *p;
   char *regs;
@@ -7739,7 +7776,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 ();
@@ -7747,7 +7785,7 @@ remote_fetch_registers (struct target_ops *ops,
 
   if (regnum >= 0)
     {
-      struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum);
+      struct packet_reg *reg = packet_reg_from_regnum (gdbarch, rsa, regnum);
 
       gdb_assert (reg != NULL);
 
@@ -7773,7 +7811,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]))
 	{
@@ -7789,7 +7827,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 ();
+  struct remote_arch_state *rsa = get_remote_arch_state (regcache->arch ());
   int i;
 
   /* Make sure the entire registers array is valid.  */
@@ -7855,7 +7893,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;
 
@@ -7894,7 +7932,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 ();
@@ -7902,7 +7941,7 @@ remote_store_registers (struct target_ops *ops,
 
   if (regnum >= 0)
     {
-      struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum);
+      struct packet_reg *reg = packet_reg_from_regnum (gdbarch, rsa, regnum);
 
       gdb_assert (reg != NULL);
 
@@ -7926,7 +7965,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.  */
@@ -12653,7 +12692,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/target.c b/gdb/target.c
index 733c086..cb66158 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3131,7 +3131,9 @@ default_watchpoint_addr_within_range (struct target_ops *target,
 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/gdb.multi/hangout.c b/gdb/testsuite/gdb.multi/hangout.c
index 44dfba6..7266a18 100644
--- 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 2c5bee9..7cc5f3c 100644
--- 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 d73b4f7..733afa0 100644
--- 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"
+}
-- 
2.5.5

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> @@ -7015,9 +7018,43 @@ 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.  */

Could you add a comment about when/how does GDB fetch the target
description of the first-time-seen process?

> +		      if (inf == NULL)
> +			{
> +			  p = strchrnul (p1 + 1, ';');
> +			  p++;
> +			  continue;
> +			}

Otherwise, patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>  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;
>  }

It is right, but forgot to mention that we need to update
spu_thread_architecture too,

  if (parse_spufs_run (ptid, &spufs_fd, &spufs_addr))
    return spu_gdbarch (spufs_fd);

  return target_gdbarch ();

it looks wrong to call target_gdbarch.  We may need to replace
target_gdbarch with default_thread_architecture.

-- 
Yao (齐尧)

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

On 10/03/2017 12:40 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>  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;
>>  }
> 
> It is right, but forgot to mention that we need to update
> spu_thread_architecture too,
> 
>   if (parse_spufs_run (ptid, &spufs_fd, &spufs_addr))
>     return spu_gdbarch (spufs_fd);
> 
>   return target_gdbarch ();
> 
> it looks wrong to call target_gdbarch.  We may need to replace
> target_gdbarch with default_thread_architecture.

I think you're right.  

The target_gdbarch reference in process_stop_reply looks incorrect
as well (last hunk below).  I'm current running test with the patch below
on top.  Kind of curious that I didn't run into this one before.

From 60956bf76cf0b5582f4085f8f36f1ee0bf7f804f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 2 Oct 2017 18:23:10 +0100
Subject: [PATCH] more target_gdbarch

---
 gdb/remote.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index b6a81a2..894c3de 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6363,6 +6363,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 +6841,6 @@ static void
 remote_parse_stop_reply (char *buf, struct stop_reply *event)
 {
   remote_arch_state *rsa = NULL;
-  struct gdbarch *reply_arch = NULL;
   ULONGEST addr;
   const char *p;
   int skipregs = 0;
@@ -7048,11 +7050,11 @@ Packet: '%s'\n"),
 			  continue;
 			}
 
-		      reply_arch = inf->gdbarch;
-		      rsa = get_remote_arch_state (reply_arch);
+		      event->arch = inf->gdbarch;
+		      rsa = get_remote_arch_state (event->arch);
 		    }
 
-		  struct packet_reg *reg = packet_reg_from_pnum (reply_arch,
+		  struct packet_reg *reg = packet_reg_from_pnum (event->arch,
 								 rsa, pnum);
 		  cached_reg_t cached_reg;
 
@@ -7063,13 +7065,13 @@ Packet: '%s'\n"),
 
 		  cached_reg.num = reg->regnum;
 		  cached_reg.data = (gdb_byte *)
-		    xmalloc (register_size (reply_arch, reg->regnum));
+		    xmalloc (register_size (event->arch, reg->regnum));
 
 		  p = p1 + 1;
 		  fieldsize = hex2bin (p, cached_reg.data,
-				       register_size (reply_arch, reg->regnum));
+				       register_size (event->arch, reg->regnum));
 		  p += 2 * fieldsize;
-		  if (fieldsize < register_size (reply_arch, 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);
@@ -7285,7 +7287,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;
 
-- 
2.5.5

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

On 10/03/2017 01:21 PM, Pedro Alves wrote:
> 
> On 10/03/2017 12:40 PM, Yao Qi wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>
>>>  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;
>>>  }
>>
>> It is right, but forgot to mention that we need to update
>> spu_thread_architecture too,
>>
>>   if (parse_spufs_run (ptid, &spufs_fd, &spufs_addr))
>>     return spu_gdbarch (spufs_fd);
>>
>>   return target_gdbarch ();
>>
>> it looks wrong to call target_gdbarch.  We may need to replace
>> target_gdbarch with default_thread_architecture.
> 
> I think you're right.  
> 
> The target_gdbarch reference in process_stop_reply looks incorrect
> as well (last hunk below).  I'm current running test with the patch below
> on top.  Kind of curious that I didn't run into this one before.
> 

Here's the updated patch.  

Instead of exporting default_thread_architecture,
I'm deferring to the target beneath.  In practice ends up being the
same thing I think, but seems a bit more correct and future proof,
and is also simpler.

I've added the comment about target descriptions.

WDYT?

From 8bcea3ea32a55a3cfac29dde1c97083843047e45 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 3 Oct 2017 13:26:42 +0100
Subject: [PATCH 3/3] 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:
yyyy-mm-dd  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:
yyyy-mm-dd  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.
---
 gdb/remote.c                           | 100 ++++++++++++++++++++++++---------
 gdb/spu-multiarch.c                    |   3 +-
 gdb/target.c                           |   4 +-
 gdb/testsuite/gdb.multi/hangout.c      |  14 +++++
 gdb/testsuite/gdb.multi/hello.c        |  15 ++++-
 gdb/testsuite/gdb.multi/multi-arch.exp |  24 ++++++++
 6 files changed, 130 insertions(+), 30 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 54d810f..9bc69c2 100644
--- 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)
@@ -6361,6 +6363,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
@@ -6835,7 +6840,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;
@@ -7015,9 +7020,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\
@@ -7026,13 +7069,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);
@@ -7248,7 +7291,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;
 
@@ -7605,7 +7648,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;
@@ -7739,7 +7782,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 ();
@@ -7747,7 +7791,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);
 
@@ -7773,7 +7817,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]))
 	{
@@ -7789,7 +7833,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.  */
@@ -7855,7 +7899,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;
 
@@ -7894,7 +7938,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 ();
@@ -7902,7 +7947,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);
 
@@ -7926,7 +7971,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.  */
@@ -12653,7 +12698,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 a935a72..e521c7e 100644
--- 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 aded0ba..f2dccd2 100644
--- 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/gdb.multi/hangout.c b/gdb/testsuite/gdb.multi/hangout.c
index 44dfba6..7266a18 100644
--- 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 2c5bee9..7cc5f3c 100644
--- 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 d73b4f7..733afa0 100644
--- 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"
+}
-- 
2.5.5

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> WDYT?

It is good to me, thanks.

-- 
Yao (齐尧)

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

On 10/04/2017 11:20 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> WDYT?
> 
> It is good to me, thanks.
> 

Thanks for the reviews.  I've pushed the series in.

Pedro Alves

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Ulrich Weigand]

Pedro Alves wrote:

> 	* target.c (default_thread_architecture): Use the sepecified
> 	inferior's architecture, instead of the current inferior's
> 	architecture (via target_gdbarch).

This causes a number of ICEs in the test suite for me, at the point when
default_thread_architecture is called from linux_child_follow_fork here:

          if (!gdbarch_software_single_step_p (target_thread_architecture
                                               (child_lp->ptid)))

Note that at this point there is no inferior for the child, and I think
there will never be one since the child is to be detached immediately.

Given that this is a fork, I guess a simple fix would be to check
for the parent's thread architecture here instead.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

On 10/05/2017 05:50 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
>> 	* target.c (default_thread_architecture): Use the sepecified
>> 	inferior's architecture, instead of the current inferior's
>> 	architecture (via target_gdbarch).
> 
> This causes a number of ICEs in the test suite for me, at the point when
> default_thread_architecture is called from linux_child_follow_fork here:
> 
>           if (!gdbarch_software_single_step_p (target_thread_architecture
>                                                (child_lp->ptid)))
> 
> Note that at this point there is no inferior for the child, and I think
> there will never be one since the child is to be detached immediately.
> 
> Given that this is a fork, I guess a simple fix would be to check
> for the parent's thread architecture here instead.

Indeed, sorry about that.  I noticed this too this afternoon,
and wrote a fix just like you suggest and was just waiting for
a test run to complete.  Results look good, so I'll push it
in in a bit.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Maciej W. Rozycki]

On Mon, 2 Oct 2017, Pedro Alves wrote:

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

 Another regression.  This makes `mips-mti-linux-gnu' GDB stop working 
with older stubs.  I have discovered it in an attempt to regression-test 
said GDB with a remote n64 target and `x86_64-pc-linux-gnu' host, and an 
outstanding change which affects non-XML stubs.  Any attempt to continue 
execution after the initial connection fails with:

[...]
Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 2670
Listening on port 2346
target remote [...]:2346
Remote debugging using [...]:2346
Reading symbols from .../lib64/ld.so.1...done.
[Switching to Thread <main>]
(gdb) continue
Cannot execute this command without a live selected thread.
(gdb) 

This is as from commit 5cd63fda035d.  Up to 5cd63fda035d^ instead I get:

[...]
Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 30044
Listening on port 2346
target remote [...]:2346
Remote debugging using [...]:2346
Reading symbols from .../lib64/ld.so.1...done.
0x000000fff79534f0 in __start () from .../lib64/ld.so.1
(gdb) continue
Continuing.
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?

Breakpoint 1, main () at .../gdb/testsuite/gdb.base/advance.c:41
41	  c = 5;
(gdb) 

At the protocol level the difference starts here (bad):

Sending packet: $qfThreadInfo#bb...Ack
Packet received: m1814
Sending packet: $qsThreadInfo#c8...Ack
Packet received: l
[Switching to Thread <main>]
Sending packet: $qSymbol::#5b...Ack
Packet received: qSymbol:6e70746c5f76657273696f6e
Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
Packet received: OK
Sending packet: $Hg1814#7d...Ack
Packet received: OK
Sending packet: $Hg0#df...Ack
Packet received: E01
(gdb) continue
Cannot execute this command without a live selected thread.
(gdb)

vs (good):

Sending packet: $qfThreadInfo#bb...Ack
Packet received: m154d
Sending packet: $qsThreadInfo#c8...Ack
Packet received: l
Sending packet: $mfff726c4f0,4#cb...Ack
Packet received: 03e0c825
Sending packet: $mfff726c4ec,4#fd...Ack
Packet received: 00000000
Sending packet: $mfff726c4f0,4#cb...Ack
Packet received: 03e0c825
0x000000fff726c4f0 in __start () from .../lib64/ld.so.1
Sending packet: $qSymbol::#5b...Ack
Packet received: qSymbol:6e70746c5f76657273696f6e
Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
Packet received: OK
(gdb) continue
Continuing.
Sending packet: $mfff726c4f0,4#cb...Ack
Packet received: 03e0c825
Sending packet: $Z0,120000d6c,4#36...Ack
Packet received:
Packet Z0 (software-breakpoint) is NOT supported
[...]

The version of `gdbserver' causing this regression is as at commit 
f8b73d13b7ca^, the last MIPS backend version with no XML support.  It's 
likely that later versions hit this regression too, as the mode of failure 
is clearly not XML-related.

 I'll paste it into Bugzilla too; please feel free to tweak as required, 
and, as always, I'll be happy to supply any details missing.

  Maciej

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Pedro Alves]

On 12/12/2017 09:33 PM, Maciej W. Rozycki wrote:
> On Mon, 2 Oct 2017, Pedro Alves wrote:
> 
>> 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.
> 
>  Another regression.  This makes `mips-mti-linux-gnu' GDB stop working 
> with older stubs.  I have discovered it in an attempt to regression-test 
> said GDB with a remote n64 target and `x86_64-pc-linux-gnu' host, and an 
> outstanding change which affects non-XML stubs.  Any attempt to continue 
> execution after the initial connection fails with:
> 
> [...]
> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 2670
> Listening on port 2346
> target remote [...]:2346
> Remote debugging using [...]:2346
> Reading symbols from .../lib64/ld.so.1...done.
> [Switching to Thread <main>]
> (gdb) continue
> Cannot execute this command without a live selected thread.
> (gdb) 
> 
> This is as from commit 5cd63fda035d.  Up to 5cd63fda035d^ instead I get:
> 
> [...]
> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 30044
> Listening on port 2346
> target remote [...]:2346
> Remote debugging using [...]:2346
> Reading symbols from .../lib64/ld.so.1...done.
> 0x000000fff79534f0 in __start () from .../lib64/ld.so.1
> (gdb) continue
> Continuing.
> warning: Could not load shared library symbols for linux-vdso.so.1.
> Do you need "set solib-search-path" or "set sysroot"?
> 
> Breakpoint 1, main () at .../gdb/testsuite/gdb.base/advance.c:41
> 41	  c = 5;
> (gdb) 
> 
> At the protocol level the difference starts here (bad):
> 
> Sending packet: $qfThreadInfo#bb...Ack
> Packet received: m1814
> Sending packet: $qsThreadInfo#c8...Ack
> Packet received: l
> [Switching to Thread <main>]
> Sending packet: $qSymbol::#5b...Ack
> Packet received: qSymbol:6e70746c5f76657273696f6e
> Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
> Packet received: OK
> Sending packet: $Hg1814#7d...Ack
> Packet received: OK
> Sending packet: $Hg0#df...Ack
> Packet received: E01
> (gdb) continue
> Cannot execute this command without a live selected thread.
> (gdb)

So IIUC, the difference is that after this patch, we now start sending those
two Hg / select-thread packets, and I assume that the second one throws,
an error (given the E01) which leaves the thread state out of sync.
Strange, off hand I'm not seeing how this patch can affect/cause this, since
it's mostly touching gdbarch / register bits.

OK, in the bugzilla report you mentioned 
"Empty `qsThreadInfo' reply handling regression causing inability
to execute".

That sequence:

> Sending packet: $qfThreadInfo#bb...Ack
> Packet received: m1814
> Sending packet: $qsThreadInfo#c8...Ack
> Packet received: l

looks OK to me off hand, since "l" means there are no more
threads (other than 1814).

Just to double-check; you're positive that this is the right
commit to blame?  I'm wondering that because the remote.c thread
listing bits were touched by other patches recently, and
I'd suspect those changes first.

> 
> vs (good):
> 
> Sending packet: $qfThreadInfo#bb...Ack
> Packet received: m154d
> Sending packet: $qsThreadInfo#c8...Ack
> Packet received: l
> Sending packet: $mfff726c4f0,4#cb...Ack
> Packet received: 03e0c825
> Sending packet: $mfff726c4ec,4#fd...Ack
> Packet received: 00000000
> Sending packet: $mfff726c4f0,4#cb...Ack
> Packet received: 03e0c825
> 0x000000fff726c4f0 in __start () from .../lib64/ld.so.1
> Sending packet: $qSymbol::#5b...Ack
> Packet received: qSymbol:6e70746c5f76657273696f6e
> Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
> Packet received: OK
> (gdb) continue
> Continuing.
> Sending packet: $mfff726c4f0,4#cb...Ack
> Packet received: 03e0c825
> Sending packet: $Z0,120000d6c,4#36...Ack
> Packet received:
> Packet Z0 (software-breakpoint) is NOT supported
> [...]

So here we didn't used to see any Hg.

> 
> The version of `gdbserver' causing this regression is as at commit 
> f8b73d13b7ca^, the last MIPS backend version with no XML support.  It's 
> likely that later versions hit this regression too, as the mode of failure 
> is clearly not XML-related.
> 

Note: with current master, you can disable XML support with 
"set remote target-features-packet off" before connecting.
Up until recently (7cc244debb58) that command didn't really work.
So maybe it's possible to use current master gdbserver
as proxy for emulating the old gdbserver.

>  I'll paste it into Bugzilla too; please feel free to tweak as required, 
> and, as always, I'll be happy to supply any details missing.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

[Maciej W. Rozycki]

On Tue, 12 Dec 2017, Pedro Alves wrote:

> > [...]
> > Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 2670
> > Listening on port 2346
> > target remote [...]:2346
> > Remote debugging using [...]:2346
> > Reading symbols from .../lib64/ld.so.1...done.
> > [Switching to Thread <main>]
> > (gdb) continue
> > Cannot execute this command without a live selected thread.
> > (gdb) 
> > 
> > This is as from commit 5cd63fda035d.  Up to 5cd63fda035d^ instead I get:
> > 
> > [...]
> > Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 30044
> > Listening on port 2346
> > target remote [...]:2346
> > Remote debugging using [...]:2346
> > Reading symbols from .../lib64/ld.so.1...done.
> > 0x000000fff79534f0 in __start () from .../lib64/ld.so.1
> > (gdb) continue
> > Continuing.
> > warning: Could not load shared library symbols for linux-vdso.so.1.
> > Do you need "set solib-search-path" or "set sysroot"?
> > 
> > Breakpoint 1, main () at .../gdb/testsuite/gdb.base/advance.c:41
> > 41	  c = 5;
> > (gdb) 
> > 
> > At the protocol level the difference starts here (bad):
> > 
> > Sending packet: $qfThreadInfo#bb...Ack
> > Packet received: m1814
> > Sending packet: $qsThreadInfo#c8...Ack
> > Packet received: l
> > [Switching to Thread <main>]
> > Sending packet: $qSymbol::#5b...Ack
> > Packet received: qSymbol:6e70746c5f76657273696f6e
> > Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
> > Packet received: OK
> > Sending packet: $Hg1814#7d...Ack
> > Packet received: OK
> > Sending packet: $Hg0#df...Ack
> > Packet received: E01
> > (gdb) continue
> > Cannot execute this command without a live selected thread.
> > (gdb)
> 
> So IIUC, the difference is that after this patch, we now start sending those
> two Hg / select-thread packets, and I assume that the second one throws,
> an error (given the E01) which leaves the thread state out of sync.

 Right, I somehow missed this error reply.  My first observation was that 
we don't issue these memory examination packets after the `l' response 
anymore (those are clearly used to report where the debuggee has stopped 
at connection).

> Strange, off hand I'm not seeing how this patch can affect/cause this, since
> it's mostly touching gdbarch / register bits.
> 
> OK, in the bugzilla report you mentioned 
> "Empty `qsThreadInfo' reply handling regression causing inability
> to execute".

 I didn't read the description of `qsThreadInfo' carefully enough in the 
manual and didn't notice it is related to `qfThreadInfo'.  Sorry about 
that.  The title of the bug will have to be changed according to further 
findings.

> That sequence:
> 
> > Sending packet: $qfThreadInfo#bb...Ack
> > Packet received: m1814
> > Sending packet: $qsThreadInfo#c8...Ack
> > Packet received: l
> 
> looks OK to me off hand, since "l" means there are no more
> threads (other than 1814).
> 
> Just to double-check; you're positive that this is the right
> commit to blame?  I'm wondering that because the remote.c thread
> listing bits were touched by other patches recently, and
> I'd suspect those changes first.

 I am positive, I tried several times and the failure is always the same 
from 5cd63fda035d onwards, whereas earlier revisions work just fine.

> > vs (good):
> > 
> > Sending packet: $qfThreadInfo#bb...Ack
> > Packet received: m154d
> > Sending packet: $qsThreadInfo#c8...Ack
> > Packet received: l
> > Sending packet: $mfff726c4f0,4#cb...Ack
> > Packet received: 03e0c825
> > Sending packet: $mfff726c4ec,4#fd...Ack
> > Packet received: 00000000
> > Sending packet: $mfff726c4f0,4#cb...Ack
> > Packet received: 03e0c825
> > 0x000000fff726c4f0 in __start () from .../lib64/ld.so.1
> > Sending packet: $qSymbol::#5b...Ack
> > Packet received: qSymbol:6e70746c5f76657273696f6e
> > Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
> > Packet received: OK
> > (gdb) continue
> > Continuing.
> > Sending packet: $mfff726c4f0,4#cb...Ack
> > Packet received: 03e0c825
> > Sending packet: $Z0,120000d6c,4#36...Ack
> > Packet received:
> > Packet Z0 (software-breakpoint) is NOT supported
> > [...]
> 
> So here we didn't used to see any Hg.

 Correct indeed.  Now that you mention it I wonder if there actually was a 
`gdbserver' issue with `Hg0' handling in f8b73d13b7ca^, hmm...  Let's 
check:

	    case 'H':
	      if (own_buf[1] == 'c' || own_buf[1] == 'g' || own_buf[1] == 's')
		{
		  unsigned long gdb_id, thread_id;

		  gdb_id = strtoul (&own_buf[2], NULL, 16);
		  thread_id = gdb_id_to_thread_id (gdb_id);
		  if (thread_id == 0)
		    {
		      write_enn (own_buf);
		      break;
		    }

-- so `Hg0' actually wasn't yet supported back then.  AFAICT it was only 
your commit bd99dc858385 ("Non-stop mode support.") that added this 
special exception.

 So I think we need to handle such a situation somehow, and given that 
`Hg1814' immediately precedes I think that `Hg0' packet can be safely 
discarded from the sequence of requests issued (unless it is meant to 
verify `0' thread ID support, that is).

> > The version of `gdbserver' causing this regression is as at commit 
> > f8b73d13b7ca^, the last MIPS backend version with no XML support.  It's 
> > likely that later versions hit this regression too, as the mode of failure 
> > is clearly not XML-related.
> 
> Note: with current master, you can disable XML support with 
> "set remote target-features-packet off" before connecting.
> Up until recently (7cc244debb58) that command didn't really work.
> So maybe it's possible to use current master gdbserver
> as proxy for emulating the old gdbserver.

 Thanks for the hint, I'll try that as well.

 So far I started regression-testing with 5cd63fda035d^ as the change in 
question is contained with the MIPS backend and there have been no 
significant recent updates there.  This works reasonably well with the 
f8b73d13b7ca^ `gdbserver', except I had to patch `--once' out in the 
harness as not supported back then and execution is slow due to the lack 
of `monitor exit' command support.

 Anyway I think we should strive to continue supporting old stubs as 
people may be stuck with them for some reason (and need current GDB for 
another).

  Maciej

[PATCH 2/3] Reimplement support for "maint print registers" with no running inferior yet

[Pedro Alves]

A following patch will change the default target_thread_architecture
method, like this:

   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;
   }

This is because target_gdbarch is really just
current_inferior()->gdbarch, and it's wrong to return that
architecture when the inferior of the passed in PTID is NOT the
current inferior -- the inferior for PTID may be running a different
architecture.  E.g., a mix of 64-bit and 32-bit inferiors in the same
debug session.

Doing that change above however exposes a problem in "maint print
registers", caught be the testsuite:

 -PASS: gdb.base/maint.exp: maint print registers
 +FAIL: gdb.base/maint.exp: maint print registers (GDB internal error)
...
  gdb/inferior.c:309: internal-error: inferior* find_inferior_pid(int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,

The call stack looks like this:

  #0  0x000000000068b707 in internal_error(char const*, int, char const*, ...) (file=0xa9b958 "gdb/inferior.c", line=309, fmt=0xa9b8e0 "%s: Assertion `%s' failed.") at gdb/common/errors.c:54
  #1  0x00000000006e1c40 in find_inferior_pid(int) (pid=0) at gdb/inferior.c:309
  #2  0x00000000006e1c8d in find_inferior_ptid(ptid_t) (ptid=...) at gdb/inferior.c:323
  #3  0x00000000007c18dc in default_thread_architecture(target_ops*, ptid_t) (ops=0xf86d60 <dummy_target>, ptid=...)
      at gdb/target.c:3134
  #4  0x00000000007b5414 in delegate_thread_architecture(target_ops*, ptid_t) (self=0xf86d60 <dummy_target>, arg1=...)
      at gdb/target-delegates.c:2527
  #5  0x00000000007647b3 in get_thread_regcache(ptid_t) (ptid=...) at gdb/regcache.c:466
  #6  0x00000000007647ff in get_current_regcache() () at gdb/regcache.c:475
  #7  0x0000000000767495 in regcache_print(char const*, regcache_dump_what) (args=0x0, what_to_dump=regcache_dump_none)
      at gdb/regcache.c:1599
  #8  0x0000000000767550 in maintenance_print_registers(char const*, int) (args=0x0, from_tty=1)
      at gdb/regcache.c:1613

I.e., the test does "maint print registers" while the inferior is not
running yet.  This is expected to work, and there's already a hack in
get_thread_arch_regcache to make it work.

Instead of pilling on hacks in the internal of regcache and
target_ops, this commit moves the null_ptid special casing to where it
belongs -- higher up in the call chain in the implementation of "maint
print registers" & co directly.

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

	* regcache.c (get_thread_arch_regcache): Remove null_ptid special
	case.
	(regcache_print): Handle !target_has_registers here instead.
---
 gdb/regcache.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index acec972..bf448ef 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -439,17 +439,7 @@ get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch,
 struct regcache *
 get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
 {
-  struct address_space *aspace;
-
-  /* For the benefit of "maint print registers" & co when debugging an
-     executable, allow dumping the regcache even when there is no
-     thread selected (target_thread_address_space internal-errors if
-     no address space is found).  Note that normal user commands will
-     fail higher up on the call stack due to no
-     target_has_registers.  */
-  aspace = (ptid_equal (null_ptid, ptid)
-	    ? NULL
-	    : target_thread_address_space (ptid));
+  address_space *aspace = target_thread_address_space (ptid);
 
   return get_thread_arch_aspace_regcache  (ptid, gdbarch, aspace);
 }
@@ -1595,15 +1585,28 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
 static void
 regcache_print (const char *args, enum regcache_dump_what what_to_dump)
 {
+  /* Where to send output.  */
+  stdio_file file;
+  ui_file *out;
+
   if (args == NULL)
-    get_current_regcache ()->dump (gdb_stdout, what_to_dump);
+    out = gdb_stdout;
   else
     {
-      stdio_file file;
-
       if (!file.open (args, "w"))
 	perror_with_name (_("maintenance print architecture"));
-      get_current_regcache ()->dump (&file, what_to_dump);
+      out = &file;
+    }
+
+  if (target_has_registers)
+    get_current_regcache ()->dump (out, what_to_dump);
+  else
+    {
+      /* For the benefit of "maint print registers" & co when
+	 debugging an executable, allow dumping a regcache even when
+	 there is no thread selected / no registers.  */
+      regcache dummy_regs (target_gdbarch (), nullptr);
+      dummy_regs.dump (out, what_to_dump);
     }
 }
 
-- 
2.5.5

Re: [PATCH 2/3] Reimplement support for "maint print registers" with no running inferior yet

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> gdb/ChangeLog:
> 2017-10-02  Pedro Alves  <palves@redhat.com>
>
> 	* regcache.c (get_thread_arch_regcache): Remove null_ptid special
> 	case.
> 	(regcache_print): Handle !target_has_registers here instead.

Patch is good to me.

-- 
Yao (齐尧)

[PATCH 1/3] Redesign mock environment for gdbarch selftests

[Pedro Alves]

A following patch will remove this a hack from within regcache's
implementation:

  struct regcache *
  get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
  {
    struct address_space *aspace;

    /* For the benefit of "maint print registers" & co when debugging an
       executable, allow dumping the regcache even when there is no
       thread selected (target_thread_address_space internal-errors if
       no address space is found).  Note that normal user commands will
       fail higher up on the call stack due to no
       target_has_registers.  */
    aspace = (ptid_equal (null_ptid, ptid)
	      ? NULL
	      : target_thread_address_space (ptid));

i.e., it'll no longer be possible to try to build a regcache for
null_ptid.  That change alone would regress the gdbarch self tests
though, causing this:

  (gdb) maintenance selftest
  [...]
  Running selftest register_to_value.
  src/gdb/inferior.c:309: internal-error: inferior* find_inferior_pid(int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Quit this debugging session? (y or n) FAIL: gdb.gdb/unittest.exp: maintenance selftest (GDB internal error)

The problem is that the way the mocking environment for those unit
tests is written is a bit fragile: it creates a special purpose
regcache (and sentinel's frame), using whatever is the current
inferior_ptid (usually null_ptid), and assumes get_current_regcache
will find that in the regcache::current_regcache list.

This commit changes the way the mock environment is created.  It
eliminates the special regcache and frame and instead creates a fuller
mock environment, with a custom mock target_ops, and then a mock
inferior and thread "running" on that target.

If there's already a running target when you type "maint selftest",
then we bail out, instead of pushing a new target on top of the
existing one (and thus killing the debug session).  I think that's OK,
because self tests are really mean to be run from a clean state right
after GDB is started.  I'm adding that skipping just as safe measure
just in case someone (as I've done it) types "maint selftest" on the
command line while already debugging something.

(In my multi-target branch, where this patch originated from, we don't
actually need to bail out, because there each inferior has its own
target stack).

Also, note that the current code was doing:

 current_inferior()->gdbarch = gdbarch;

without taking care to restore the previous gdbarch.  This means that
GDB's state was being left inconsistent after running the self tests,,
further supporting the point that there's probably not much
expectation that mixing "maint selftests" and regular debugging in the
same GDB invocation really works.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* frame.c (create_test_frame): Delete.
	* frame.h (create_test_frame): Delete.
	* gdbarch-selftests.c: Include gdbthread.h and target.h.
	(class regcache_test): Delete.
	(test_target_has_registers, test_target_has_stack)
	(test_target_has_memory, test_target_prepare_to_store)
	(test_target_fetch_registers, test_target_store_registers): New
	functions.
	(test_target_ops): New class.
	(register_to_value_test): Error out if there's execution.  Create
	a fuller mock environment, with mock target_ops, inferior, address
	space, thread and inferior_ptid.
	* progspace.c (struct address_space): Move to ...
	* progspace.h (struct address_space): ... here.
	* regcache.h (regcache::~regcache, regcache::raw_write)
	[GDB_SELF_TEST]: No longer virtual.
	* target.c (default_thread_address_space)
	(default_thread_architecture): Make extern.
	* target.h (default_thread_architecture)
	(default_thread_address_space): Declare.
---
 gdb/frame.c             |  17 --------
 gdb/frame.h             |   8 ----
 gdb/gdbarch-selftests.c | 105 ++++++++++++++++++++++++++++++++++++++++--------
 gdb/progspace.c         |  12 ------
 gdb/progspace.h         |  11 +++++
 gdb/regcache.h          |  10 -----
 gdb/target.c            |  10 +----
 gdb/target.h            |  10 +++++
 8 files changed, 111 insertions(+), 72 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index a74deef..afdc157 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1716,23 +1716,6 @@ select_frame (struct frame_info *fi)
     }
 }
 
-#if GDB_SELF_TEST
-struct frame_info *
-create_test_frame (struct regcache *regcache)
-{
-  struct frame_info *this_frame = XCNEW (struct frame_info);
-
-  sentinel_frame = create_sentinel_frame (NULL, regcache);
-  sentinel_frame->prev = this_frame;
-  sentinel_frame->prev_p = 1;;
-  this_frame->prev_arch.p = 1;
-  this_frame->prev_arch.arch = get_regcache_arch (regcache);
-  this_frame->next = sentinel_frame;
-
-  return this_frame;
-}
-#endif
-
 /* Create an arbitrary (i.e. address specified by user) or innermost frame.
    Always returns a non-NULL value.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index 0249857..1b72610 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -833,14 +833,6 @@ extern struct frame_info *deprecated_safe_get_selected_frame (void);
 
 extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR pc);
 
-#if GDB_SELF_TEST
-
-/* Create a frame for unit test.  Its next frame is sentinel frame,
-   created from REGCACHE.  */
-
-extern struct frame_info *create_test_frame (struct regcache *regcache);
-#endif
-
 /* Return true if the frame unwinder for frame FI is UNWINDER; false
    otherwise.  */
 
diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index f0b8d5d..1e9077a 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -22,25 +22,63 @@
 #include "selftest.h"
 #include "selftest-arch.h"
 #include "inferior.h"
+#include "gdbthread.h"
+#include "target.h"
 
 namespace selftests {
 
-/* A read-write regcache which doesn't write the target.  */
+/* A mock process_stratum target_ops that doesn't read/write registers
+   anywhere.  */
 
-class regcache_test : public regcache
+static int
+test_target_has_registers (target_ops *self)
 {
-public:
-  explicit regcache_test (struct gdbarch *gdbarch)
-    : regcache (gdbarch, NULL, false)
-  {
-    set_ptid (inferior_ptid);
+  return 1;
+}
 
-    current_regcache.push_front (this);
-  }
+static int
+test_target_has_stack (target_ops *self)
+{
+  return 1;
+}
+
+static int
+test_target_has_memory (target_ops *self)
+{
+  return 1;
+}
+
+static void
+test_target_prepare_to_store (target_ops *self, regcache *regs)
+{
+}
 
-  void raw_write (int regnum, const gdb_byte *buf) override
+static void
+test_target_fetch_registers (target_ops *self, regcache *regs, int regno)
+{
+}
+
+static void
+test_target_store_registers (target_ops *self, regcache *regs, int regno)
+{
+}
+
+class test_target_ops : public target_ops
+{
+public:
+  test_target_ops ()
+    : target_ops {}
   {
-    raw_set_cached_value (regnum, buf);
+    to_magic = OPS_MAGIC;
+    to_stratum = process_stratum;
+    to_has_memory = test_target_has_memory;
+    to_has_stack = test_target_has_stack;
+    to_has_registers = test_target_has_registers;
+    to_prepare_to_store = test_target_prepare_to_store;
+    to_fetch_registers = test_target_fetch_registers;
+    to_store_registers = test_target_store_registers;
+    to_thread_architecture = default_thread_architecture;
+    to_thread_address_space = default_thread_address_space;
   }
 };
 
@@ -84,15 +122,48 @@ register_to_value_test (struct gdbarch *gdbarch)
       builtin->builtin_char32,
     };
 
-  current_inferior()->gdbarch = gdbarch;
-
-  struct regcache *regcache = new regcache_test (gdbarch);
-  struct frame_info *frame = create_test_frame (regcache);
+  if (target_has_execution)
+    error (_("inferior is running"));
+
+  /* Create a mock environment.  An inferior with a thread, with a
+     process_stratum target pushed.  */
+
+  test_target_ops mock_target;
+  ptid_t mock_ptid (1, 1);
+  inferior mock_inferior (mock_ptid.pid ());
+  address_space mock_aspace {};
+  mock_inferior.gdbarch = gdbarch;
+  mock_inferior.aspace = &mock_aspace;
+  thread_info mock_thread (&mock_inferior, mock_ptid);
+
+  scoped_restore restore_thread_list
+    = make_scoped_restore (&thread_list, &mock_thread);
+
+  /* Add the mock inferior to the inferior list so that look ups by
+     target+ptid can find it.  */
+  scoped_restore restore_inferior_list
+    = make_scoped_restore (&inferior_list);
+  mock_inferior.next = inferior_list;
+  inferior_list = &mock_inferior;
+
+  /* Switch to the mock inferior.  */
+  scoped_restore_current_inferior restore_current_inferior;
+  set_current_inferior (&mock_inferior);
+
+  /* Push the process_stratum target so we can mock accessing
+     registers.  */
+  push_target (&mock_target);
+  struct on_exit { ~on_exit () { pop_all_targets (); } };
+  on_exit pop_targets;
+
+  /* Switch to the mock thread.  */
+  scoped_restore restore_inferior_ptid
+    = make_scoped_restore (&inferior_ptid, mock_ptid);
+
+  struct frame_info *frame = get_current_frame ();
   const int num_regs = (gdbarch_num_regs (gdbarch)
 			+ gdbarch_num_pseudo_regs (gdbarch));
 
-  SELF_CHECK (regcache == get_current_regcache ());
-
   /* Test gdbarch methods register_to_value and value_to_register with
      different combinations of register numbers and types.  */
   for (const auto &type : types)
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 41e8cd0..ea328c8 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -44,18 +44,6 @@ static int highest_address_space_num;
 
 DEFINE_REGISTRY (program_space, REGISTRY_ACCESS_FIELD)
 
-/* An address space.  It is used for comparing if pspaces/inferior/threads
-   see the same address space and for associating caches to each address
-   space.  */
-
-struct address_space
-{
-  int num;
-
-  /* Per aspace data-pointers required by other GDB modules.  */
-  REGISTRY_FIELDS;
-};
-
 /* Keep a registry of per-address_space data-pointers required by other GDB
    modules.  */
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index f679fe3..85c99a62 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -210,6 +210,17 @@ struct program_space
     REGISTRY_FIELDS;
   };
 
+/* An address space.  It is used for comparing if
+   pspaces/inferior/threads see the same address space and for
+   associating caches to each address space.  */
+struct address_space
+{
+  int num;
+
+  /* Per aspace data-pointers required by other GDB modules.  */
+  REGISTRY_FIELDS;
+};
+
 /* The object file that the main symbol table was loaded from (e.g. the
    argument to the "symbol-file" or "file" command).  */
 
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 3ecdb3b..6f42fb93 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -252,11 +252,6 @@ public:
 
   DISABLE_COPY_AND_ASSIGN (regcache);
 
-  /* class regcache is only extended in unit test, so only mark it
-     virtual when selftest is enabled.  */
-#if GDB_SELF_TEST
-  virtual
-#endif
   ~regcache ()
   {
     xfree (m_registers);
@@ -277,11 +272,6 @@ public:
 
   enum register_status raw_read (int regnum, gdb_byte *buf);
 
-  /* class regcache is only extended in unit test, so only mark it
-     virtual when selftest is enabled.  */
-#if GDB_SELF_TEST
-  virtual
-#endif
   void raw_write (int regnum, const gdb_byte *buf);
 
   template<typename T, typename = RequireLongest<T>>
diff --git a/gdb/target.c b/gdb/target.c
index aded0ba..733c086 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -81,9 +81,6 @@ static int default_verify_memory (struct target_ops *self,
 				  const gdb_byte *data,
 				  CORE_ADDR memaddr, ULONGEST size);
 
-static struct address_space *default_thread_address_space
-     (struct target_ops *self, ptid_t ptid);
-
 static void tcomplain (void) ATTRIBUTE_NORETURN;
 
 static int return_zero (struct target_ops *);
@@ -94,9 +91,6 @@ static void target_command (char *, int);
 
 static struct target_ops *find_default_run_target (const char *);
 
-static struct gdbarch *default_thread_architecture (struct target_ops *ops,
-						    ptid_t ptid);
-
 static int dummy_find_memory_regions (struct target_ops *self,
 				      find_memory_region_ftype ignore1,
 				      void *ignore2);
@@ -2675,7 +2669,7 @@ target_get_osdata (const char *type)
   return target_read_stralloc (t, TARGET_OBJECT_OSDATA, type);
 }
 
-static struct address_space *
+struct address_space *
 default_thread_address_space (struct target_ops *self, ptid_t ptid)
 {
   struct inferior *inf;
@@ -3134,7 +3128,7 @@ default_watchpoint_addr_within_range (struct target_ops *target,
   return addr >= start && addr < start + length;
 }
 
-static struct gdbarch *
+struct gdbarch *
 default_thread_architecture (struct target_ops *ops, ptid_t ptid)
 {
   return target_gdbarch ();
diff --git a/gdb/target.h b/gdb/target.h
index f5ad1e5..ccb9ffd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1789,6 +1789,16 @@ extern int default_child_has_registers (struct target_ops *ops);
 extern int default_child_has_execution (struct target_ops *ops,
 					ptid_t the_ptid);
 
+/* The default target_ops::to_thread_architecture implementation.
+   Simply returns the architecture of the inferior for PTID.  */
+extern struct gdbarch *default_thread_architecture
+  (struct target_ops *ops, ptid_t ptid);
+
+/* The default target_ops::to_thread_address_space implementation.
+   Simply returns the address space of the inferior for PTID.  */
+extern struct address_space *default_thread_address_space
+  (struct target_ops *self, ptid_t ptid);
+
 /* Can the target support the debugger control of thread execution?
    Can it lock the thread scheduler?  */
 
-- 
2.5.5

Re: [PATCH 1/3] Redesign mock environment for gdbarch selftests

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> If there's already a running target when you type "maint selftest",
> then we bail out, instead of pushing a new target on top of the
> existing one (and thus killing the debug session).  I think that's OK,
> because self tests are really mean to be run from a clean state right
> after GDB is started.  I'm adding that skipping just as safe measure
> just in case someone (as I've done it) types "maint selftest" on the
> command line while already debugging something.

That is a goo catch, but IMO, we shouldn't allow running unit tests
during debugging session.  GDB unit tests touch many things, and some of
them are global states in GDB.  It is difficult to know what global
states we need to restore after tests.

>
> (In my multi-target branch, where this patch originated from, we don't
> actually need to bail out, because there each inferior has its own
> target stack).
>
> Also, note that the current code was doing:
>
>  current_inferior()->gdbarch = gdbarch;
>
> without taking care to restore the previous gdbarch.  This means that
> GDB's state was being left inconsistent after running the self tests,,
> further supporting the point that there's probably not much
> expectation that mixing "maint selftests" and regular debugging in the
> same GDB invocation really works.

You are right, we should manage the expectation of mixing debugging and
"maint selftests".  My suggestion is that "please do run selftests
with regular debugging".

> -    raw_set_cached_value (regnum, buf);
> +    to_magic = OPS_MAGIC;
> +    to_stratum = process_stratum;
> +    to_has_memory = test_target_has_memory;
> +    to_has_stack = test_target_has_stack;
> +    to_has_registers = test_target_has_registers;
> +    to_prepare_to_store = test_target_prepare_to_store;
> +    to_fetch_registers = test_target_fetch_registers;

to_fetch_registers is TARGET_DEFAULT_IGNORE, so we don't need define
test_target_fetch_registers if we disallow running selftests with
regular debugging.

> +    to_store_registers = test_target_store_registers;

> +    to_thread_architecture = default_thread_architecture;
> +    to_thread_address_space = default_thread_address_space;

Any reason to set to_thread_address_space and to_thread_architecture?
Can't we use the default value?

-- 
Yao (齐尧)

Re: [PATCH 1/3] Redesign mock environment for gdbarch selftests

[Pedro Alves]

On 10/03/2017 12:06 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> If there's already a running target when you type "maint selftest",
>> then we bail out, instead of pushing a new target on top of the
>> existing one (and thus killing the debug session).  I think that's OK,
>> because self tests are really mean to be run from a clean state right
>> after GDB is started.  I'm adding that skipping just as safe measure
>> just in case someone (as I've done it) types "maint selftest" on the
>> command line while already debugging something.
> 
> That is a goo catch, but IMO, we shouldn't allow running unit tests
> during debugging session.  GDB unit tests touch many things, and some of
> them are global states in GDB.  It is difficult to know what global
> states we need to restore after tests.

On the restore side, I think we should treat those as bugs to look
for when reviewing/working on unit tests, because: #1 - it's
convenient for development to be able to run unit tests multiple
times in the same session.  #2 - because we should strive to avoid
one unit test influencing the environment of following unrelated
unit tests.

On the "allow selftests during debugging", I think it's reasonable to
bail out if/when it'd be very difficult otherwise.  

In this case, maybe I could get it working even while there's
already a target by save/restoring current_target too.  I didn't bother
with that because with multi-target, the problem solves itself simply
because the current_target global disappears and it felt like
unnecessary complication.

> 
>>
>> (In my multi-target branch, where this patch originated from, we don't
>> actually need to bail out, because there each inferior has its own
>> target stack).
>>
>> Also, note that the current code was doing:
>>
>>  current_inferior()->gdbarch = gdbarch;
>>
>> without taking care to restore the previous gdbarch.  This means that
>> GDB's state was being left inconsistent after running the self tests,,
>> further supporting the point that there's probably not much
>> expectation that mixing "maint selftests" and regular debugging in the
>> same GDB invocation really works.
> 
> You are right, we should manage the expectation of mixing debugging and
> "maint selftests".  My suggestion is that "please do run selftests
> with regular debugging".

OK.  I propose we stick with something like I had, and still build
an isolated inferior/aspace etc.  To me, that feels like more robust
mocking/isolation.  If I run the tests while debugging something
with the updated patch (below), I get:

 (gdb) maint selftest 
 Running selftest array_view.
 Running selftest copy_bitwise.
 Running selftest copy_integer_to_size.
 Running selftest current_regcache.
 Running selftest execute_cfa_program.
 Running selftest function_view.
 Running selftest gdb_environ.
 Running selftest gdb_realpath.
 Running selftest memory_error.
 Running selftest offset_type.
 Running selftest optional.
 Running selftest print_one_insn.
 Running selftest producer-parser.
 Running selftest register_to_value.
 Self test failed: arch i386: target already pushed
 Self test failed: arch i386:x86-64: target already pushed
 Self test failed: arch i386:x64-32: target already pushed
 Self test failed: arch i8086: target already pushed
 Self test failed: arch i386:intel: target already pushed
 Self test failed: arch i386:x86-64:intel: target already pushed
 Self test failed: arch i386:x64-32:intel: target already pushed
 Self test failed: arch i386:nacl: target already pushed
 Self test failed: arch i386:x86-64:nacl: target already pushed
 Self test failed: arch i386:x64-32:nacl: target already pushed
 Self test failed: self-test failed at /home/pedro/gdb/mygit/src/gdb/selftest-arch.c:86
 Running selftest rust-lex.
 Running selftest scoped_restore.
 Running selftest string_printf.
 Running selftest string_vprintf.
 Running selftest xml_escape_text.
 Ran 19 unit tests, 1 failed

So it ends up being best effort, with most things passing and
the gdbarch test failing.

WDYT?

(I've switched to checking for current_target.to_stratum >= process_stratum
instead of target_has_execution, as it's a more accurate test.
target_has_execution would return false when debugging a core, 
or when connected to an extended-remote target before typing run,
to name a couple examples.)

> 
> to_fetch_registers is TARGET_DEFAULT_IGNORE, so we don't need define
> test_target_fetch_registers if we disallow running selftests with
> regular debugging.

I've made that change.  (the reason I defined it is not related
to regular debugging.  see below.)

> 
>> +    to_store_registers = test_target_store_registers;
> 
>> +    to_thread_architecture = default_thread_architecture;
>> +    to_thread_address_space = default_thread_address_space;
> 
> Any reason to set to_thread_address_space and to_thread_architecture?
> Can't we use the default value?
> 

The reason is that in the multi-target branch target_ops is a real
C++ class hierarchy and is different enough in details that I forgot
how things worked in current master.  :-P  

The problem is that I forgot that we need to call
complete_target_initialization, in order to install the defaults in
the target, so without the explicit "= default_thread_...;", I was ending
up with those methods set to NULL, and GDB would crash...

Here's the updated patch.

From d2287dfe6bafaa8a3bfd3720fd660a1aacf4693a Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 3 Oct 2017 12:24:08 +0100
Subject: [PATCH] Redesign mock environment for gdbarch selftests

A following patch will remove this hack from within regcache's
implementation:

  struct regcache *
  get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
  {
    struct address_space *aspace;

    /* For the benefit of "maint print registers" & co when debugging an
       executable, allow dumping the regcache even when there is no
       thread selected (target_thread_address_space internal-errors if
       no address space is found).  Note that normal user commands will
       fail higher up on the call stack due to no
       target_has_registers.  */
    aspace = (ptid_equal (null_ptid, ptid)
	      ? NULL
	      : target_thread_address_space (ptid));

i.e., it'll no longer be possible to try to build a regcache for
null_ptid.  That change alone would regress the gdbarch self tests
though, causing this:

  (gdb) maintenance selftest
  [...]
  Running selftest register_to_value.
  src/gdb/inferior.c:309: internal-error: inferior* find_inferior_pid(int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Quit this debugging session? (y or n) FAIL: gdb.gdb/unittest.exp: maintenance selftest (GDB internal error)

The problem is that the way the mocking environment for those unit
tests is written is a bit fragile: it creates a special purpose
regcache (and sentinel's frame), using whatever is the current
inferior_ptid (usually null_ptid), and assumes get_current_regcache
will find that in the regcache::current_regcache list.

This commit changes the way the mock environment is created.  It
eliminates the special regcache and frame and instead creates a fuller
mock environment, with a custom mock target_ops, and then a mock
inferior and thread "running" on that target.

If there's already a running target when you type "maint selftest",
then we error out, instead of pushing a new target on top of the
existing one (and thus killing the debug session).  This results in:

  (gdb) maint selftest
  (...)
  Self test failed: arch i386: target already pushed
  Self test failed: arch i386:x86-64: target already pushed
  Self test failed: arch i386:x64-32: target already pushed
  Self test failed: arch i8086: target already pushed
  Self test failed: arch i386:intel: target already pushed
  Self test failed: arch i386:x86-64:intel: target already pushed
  Self test failed: arch i386:x64-32:intel: target already pushed
  Self test failed: arch i386:nacl: target already pushed
  Self test failed: arch i386:x86-64:nacl: target already pushed
  Self test failed: arch i386:x64-32:nacl: target already pushed
  Self test failed: self-test failed at /home/pedro/gdb/mygit/src/gdb/selftest-arch.c:86
  (...)
  Ran 19 unit tests, 1 failed

I think that's OK, because self tests are really meant to be run from
a clean state right after GDB is started.  I'm adding that erroring
out just as safe measure just in case someone types "maint selftest"
on the command line while already debugging something (as I've done
it).

(In my multi-target branch, where this patch originated from, we don't
actually need to error out, because there each inferior has its own
target stack).

Also, note that the current code was doing:

 current_inferior()->gdbarch = gdbarch;

without taking care to restore the previous gdbarch.  This means that
GDB's state was being left inconsistent after running the self tests,
further supporting the point that there's probably not much
expectation that mixing "maint selftests" and regular debugging in the
same GDB invocation really works.  This patch fixes that, regardless.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* frame.c (create_test_frame): Delete.
	* frame.h (create_test_frame): Delete.
	* gdbarch-selftests.c: Include gdbthread.h and target.h.
	(class regcache_test): Delete.
	(test_target_has_registers, test_target_has_stack)
	(test_target_has_memory, test_target_prepare_to_store)
	(test_target_store_registers): New functions.
	(test_target_ops): New class.
	(register_to_value_test): Error out if there's already a
	process_stratum (or higher) target pushed.  Create a fuller mock
	environment, with mock target_ops, inferior, address space, thread
	and inferior_ptid.
	* progspace.c (struct address_space): Move to ...
	* progspace.h (struct address_space): ... here.
	* regcache.h (regcache::~regcache, regcache::raw_write)
	[GDB_SELF_TEST]: No longer virtual.
---
 gdb/frame.c             |  17 --------
 gdb/frame.h             |   8 ----
 gdb/gdbarch-selftests.c | 105 ++++++++++++++++++++++++++++++++++++++++--------
 gdb/progspace.c         |  12 ------
 gdb/progspace.h         |  11 +++++
 gdb/regcache.h          |  10 -----
 6 files changed, 100 insertions(+), 63 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index a74deef..afdc157 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1716,23 +1716,6 @@ select_frame (struct frame_info *fi)
     }
 }
 
-#if GDB_SELF_TEST
-struct frame_info *
-create_test_frame (struct regcache *regcache)
-{
-  struct frame_info *this_frame = XCNEW (struct frame_info);
-
-  sentinel_frame = create_sentinel_frame (NULL, regcache);
-  sentinel_frame->prev = this_frame;
-  sentinel_frame->prev_p = 1;;
-  this_frame->prev_arch.p = 1;
-  this_frame->prev_arch.arch = get_regcache_arch (regcache);
-  this_frame->next = sentinel_frame;
-
-  return this_frame;
-}
-#endif
-
 /* Create an arbitrary (i.e. address specified by user) or innermost frame.
    Always returns a non-NULL value.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index 0249857..1b72610 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -833,14 +833,6 @@ extern struct frame_info *deprecated_safe_get_selected_frame (void);
 
 extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR pc);
 
-#if GDB_SELF_TEST
-
-/* Create a frame for unit test.  Its next frame is sentinel frame,
-   created from REGCACHE.  */
-
-extern struct frame_info *create_test_frame (struct regcache *regcache);
-#endif
-
 /* Return true if the frame unwinder for frame FI is UNWINDER; false
    otherwise.  */
 
diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index f0b8d5d..58ed112 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -22,25 +22,57 @@
 #include "selftest.h"
 #include "selftest-arch.h"
 #include "inferior.h"
+#include "gdbthread.h"
+#include "target.h"
 
 namespace selftests {
 
-/* A read-write regcache which doesn't write the target.  */
+/* A mock process_stratum target_ops that doesn't read/write registers
+   anywhere.  */
 
-class regcache_test : public regcache
+static int
+test_target_has_registers (target_ops *self)
 {
-public:
-  explicit regcache_test (struct gdbarch *gdbarch)
-    : regcache (gdbarch, NULL, false)
-  {
-    set_ptid (inferior_ptid);
+  return 1;
+}
 
-    current_regcache.push_front (this);
-  }
+static int
+test_target_has_stack (target_ops *self)
+{
+  return 1;
+}
+
+static int
+test_target_has_memory (target_ops *self)
+{
+  return 1;
+}
+
+static void
+test_target_prepare_to_store (target_ops *self, regcache *regs)
+{
+}
+
+static void
+test_target_store_registers (target_ops *self, regcache *regs, int regno)
+{
+}
 
-  void raw_write (int regnum, const gdb_byte *buf) override
+class test_target_ops : public target_ops
+{
+public:
+  test_target_ops ()
+    : target_ops {}
   {
-    raw_set_cached_value (regnum, buf);
+    to_magic = OPS_MAGIC;
+    to_stratum = process_stratum;
+    to_has_memory = test_target_has_memory;
+    to_has_stack = test_target_has_stack;
+    to_has_registers = test_target_has_registers;
+    to_prepare_to_store = test_target_prepare_to_store;
+    to_store_registers = test_target_store_registers;
+
+    complete_target_initialization (this);
   }
 };
 
@@ -84,15 +116,56 @@ register_to_value_test (struct gdbarch *gdbarch)
       builtin->builtin_char32,
     };
 
-  current_inferior()->gdbarch = gdbarch;
+  /* Error out if debugging something, because we're going to push the
+     test target, which would pop any existing target.  */
+  if (current_target.to_stratum >= process_stratum)
+    error (_("target already pushed"));
+
+  /* Create a mock environment.  An inferior with a thread, with a
+     process_stratum target pushed.  */
+
+  test_target_ops mock_target;
+  ptid_t mock_ptid (1, 1);
+  inferior mock_inferior (mock_ptid.pid ());
+  address_space mock_aspace {};
+  mock_inferior.gdbarch = gdbarch;
+  mock_inferior.aspace = &mock_aspace;
+  thread_info mock_thread (&mock_inferior, mock_ptid);
+
+  scoped_restore restore_thread_list
+    = make_scoped_restore (&thread_list, &mock_thread);
+
+  /* Add the mock inferior to the inferior list so that look ups by
+     target+ptid can find it.  */
+  scoped_restore restore_inferior_list
+    = make_scoped_restore (&inferior_list);
+  inferior_list = &mock_inferior;
+
+  /* Switch to the mock inferior.  */
+  scoped_restore_current_inferior restore_current_inferior;
+  set_current_inferior (&mock_inferior);
+
+  /* Push the process_stratum target so we can mock accessing
+     registers.  */
+  push_target (&mock_target);
+
+  /* Pop it again on exit (return/exception).  */
+  struct on_exit
+  {
+    ~on_exit ()
+    {
+      pop_all_targets_at_and_above (process_stratum);
+    }
+  } pop_targets;
+
+  /* Switch to the mock thread.  */
+  scoped_restore restore_inferior_ptid
+    = make_scoped_restore (&inferior_ptid, mock_ptid);
 
-  struct regcache *regcache = new regcache_test (gdbarch);
-  struct frame_info *frame = create_test_frame (regcache);
+  struct frame_info *frame = get_current_frame ();
   const int num_regs = (gdbarch_num_regs (gdbarch)
 			+ gdbarch_num_pseudo_regs (gdbarch));
 
-  SELF_CHECK (regcache == get_current_regcache ());
-
   /* Test gdbarch methods register_to_value and value_to_register with
      different combinations of register numbers and types.  */
   for (const auto &type : types)
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 41e8cd0..ea328c8 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -44,18 +44,6 @@ static int highest_address_space_num;
 
 DEFINE_REGISTRY (program_space, REGISTRY_ACCESS_FIELD)
 
-/* An address space.  It is used for comparing if pspaces/inferior/threads
-   see the same address space and for associating caches to each address
-   space.  */
-
-struct address_space
-{
-  int num;
-
-  /* Per aspace data-pointers required by other GDB modules.  */
-  REGISTRY_FIELDS;
-};
-
 /* Keep a registry of per-address_space data-pointers required by other GDB
    modules.  */
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index f679fe3..85c99a62 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -210,6 +210,17 @@ struct program_space
     REGISTRY_FIELDS;
   };
 
+/* An address space.  It is used for comparing if
+   pspaces/inferior/threads see the same address space and for
+   associating caches to each address space.  */
+struct address_space
+{
+  int num;
+
+  /* Per aspace data-pointers required by other GDB modules.  */
+  REGISTRY_FIELDS;
+};
+
 /* The object file that the main symbol table was loaded from (e.g. the
    argument to the "symbol-file" or "file" command).  */
 
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 3ecdb3b..6f42fb93 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -252,11 +252,6 @@ public:
 
   DISABLE_COPY_AND_ASSIGN (regcache);
 
-  /* class regcache is only extended in unit test, so only mark it
-     virtual when selftest is enabled.  */
-#if GDB_SELF_TEST
-  virtual
-#endif
   ~regcache ()
   {
     xfree (m_registers);
@@ -277,11 +272,6 @@ public:
 
   enum register_status raw_read (int regnum, gdb_byte *buf);
 
-  /* class regcache is only extended in unit test, so only mark it
-     virtual when selftest is enabled.  */
-#if GDB_SELF_TEST
-  virtual
-#endif
   void raw_write (int regnum, const gdb_byte *buf);
 
   template<typename T, typename = RequireLongest<T>>
-- 
2.5.5

Re: [PATCH 1/3] Redesign mock environment for gdbarch selftests

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> OK.  I propose we stick with something like I had, and still build
> an isolated inferior/aspace etc.  To me, that feels like more robust

I like the way that we can build an isolated inferior/aspace for unit test.

> mocking/isolation.  If I run the tests while debugging something
> with the updated patch (below), I get:
>
>  (gdb) maint selftest 
>  Running selftest array_view.
>  Running selftest copy_bitwise.
>  Running selftest copy_integer_to_size.
>  Running selftest current_regcache.
>  Running selftest execute_cfa_program.
>  Running selftest function_view.
>  Running selftest gdb_environ.
>  Running selftest gdb_realpath.
>  Running selftest memory_error.
>  Running selftest offset_type.
>  Running selftest optional.
>  Running selftest print_one_insn.
>  Running selftest producer-parser.
>  Running selftest register_to_value.
>  Self test failed: arch i386: target already pushed
>  Self test failed: arch i386:x86-64: target already pushed
>  Self test failed: arch i386:x64-32: target already pushed
>  Self test failed: arch i8086: target already pushed
>  Self test failed: arch i386:intel: target already pushed
>  Self test failed: arch i386:x86-64:intel: target already pushed
>  Self test failed: arch i386:x64-32:intel: target already pushed
>  Self test failed: arch i386:nacl: target already pushed
>  Self test failed: arch i386:x86-64:nacl: target already pushed
>  Self test failed: arch i386:x64-32:nacl: target already pushed
>  Self test failed: self-test failed at
> /home/pedro/gdb/mygit/src/gdb/selftest-arch.c:86
>  Running selftest rust-lex.
>  Running selftest scoped_restore.
>  Running selftest string_printf.
>  Running selftest string_vprintf.
>  Running selftest xml_escape_text.
>  Ran 19 unit tests, 1 failed
>
> So it ends up being best effort, with most things passing and
> the gdbarch test failing.
>
> WDYT?

That is fine by me.  Patch is good to me.

-- 
Yao (齐尧)