Re: [EXT] Re: Semihosting in GDB 11.1 | Proposed patch for interrupting in sync mode

[Pedro Alves <pedro@palves.net>]

On 2022-03-10 11:30, Pedro Alves wrote:
> 
> HOWEVER.  Note that the File I/O packets, like "Fwrite,1,80026300,12" don't say
> at all which thread is doing the File I/O access!  IOW, this packet is not useable
> in multi-process scenarios as is...  It would need to be extended to also pass down
> an extra thread id field, like "Fwrite,1,80026300,12,pPID.TID".  FSF GDBserver doesn't
> support File I/O, so these File I/O packets didn't get attention when the multi-process
> extensions were devised.
> 
> So alright, the current "Hg0" packet you see in your logs is a bit spurious, but, it should
> be harmless, and you should be able to select any valid thread, as per the Hg0 packet's
> description, as all threads share the same address space, in GDB's view.
> 
>   [remote] Packet received: Fwrite,1,80026300,12
>   [remote] Sending packet: $Hg0#df
>   [remote] Received Ack
>   [remote] Packet received: OK
>   [remote] Sending packet: $m80026300,12#8f
>   [remote] Received Ack
>   [remote] Packet received: 48656c6c6f2066726f6d20436f726523370a
>   [remote] Sending packet: $F12#a9
> 
> What I think you should do is, when you get the reply to Fwrite -- the "$F12#a9"
> packet -- your stub should make sure that that resumes the same thread that initiated
> the Fwrite, not whatever Hg selected.  That $F12 reply is linked with the last Fwrite,
> there can only be one File I/O request ongoing at a given time.

Thinking a bit more about this, I guess GDB could just avoid changing the selected thread
when handling a File I/O request.  That seems quite easy to do, so I gave it a try.

However, a realized that that runs into a difficulty.  With multi-process, if the server
doesn't say which process the request is for, GDB's target_read_memory/target_write_memory
can end up reading/writing from the wrong dcache.  A workaround is to flush the dcache
around memory accesses that come from File I/O.  Not great, but at least it turns
a correctness issue into an optimization issue...

I don't have a target that does File I/O, so I can't really test this.

From c3093d0fa3ff527b542a141e93c0a8a580e7cce8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 10 Mar 2022 11:59:52 +0000
Subject: [PATCH] File I/O - avoid changing the server's current thread

Change-Id: I3a69e36988cb1fba746be5a4d8dd65002c8b7711
---
 gdb/remote-fileio.c | 52 +++++++++++++++++++++++++++++++++++----------
 gdb/remote.c        | 23 ++++++++++++++++----
 gdb/target-dcache.c | 23 ++++++++++++++++----
 gdb/target-dcache.h |  3 +++
 4 files changed, 82 insertions(+), 19 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 9963f1ebc01..3fbf5ec4090 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -29,6 +29,7 @@
 #include "target.h"
 #include "filenames.h"
 #include "gdbsupport/filestuff.h"
+#include "target-dcache.h"
 
 #include <fcntl.h>
 #include "gdbsupport/gdb_sys_time.h"
@@ -37,6 +38,35 @@
 #endif
 #include <signal.h>
 
+/* Wrappers around target_read_memory/target_write_memory that
+   invalidate the dcache on entry and exit.  This is needed because
+   File I/O packets don't tell us which process initiated the request,
+   so we don't know whether the cache for the current process is the
+   right one.  */
+
+static int
+fileio_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
+{
+  target_dcache_invalidate ();
+  SCOPE_EXIT { target_dcache_invalidate (); };
+
+  return target_read_memory (memaddr, myaddr, len);
+}
+
+static int
+fileio_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
+{
+  target_dcache_invalidate ();
+  SCOPE_EXIT
+    {
+      /* After writing, the cache of some other inferior may be stale,
+	 so we need to invalidate all.  */
+      target_dcache_invalidate_all ();
+    };
+
+  return target_write_memory (memaddr, myaddr, len);
+}
+
 static struct {
   int *fd_map;
   int fd_map_size;
@@ -401,7 +431,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
 
   /* Request pathname.  */
   pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  if (fileio_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -574,7 +604,7 @@ remote_fileio_func_read (remote_target *remote, char *buf)
 
   if (ret > 0)
     {
-      errno = target_write_memory (ptrval, buffer, ret);
+      errno = fileio_write_memory (ptrval, buffer, ret);
       if (errno != 0)
 	ret = -1;
     }
@@ -625,7 +655,7 @@ remote_fileio_func_write (remote_target *remote, char *buf)
   length = (size_t) num;
     
   buffer = (gdb_byte *) xmalloc (length);
-  if (target_read_memory (ptrval, buffer, length) != 0)
+  if (fileio_read_memory (ptrval, buffer, length) != 0)
     {
       xfree (buffer);
       remote_fileio_ioerror (remote);
@@ -740,7 +770,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
   
   /* Request oldpath using 'm' packet */
   oldpath = (char *) alloca (old_len);
-  if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
+  if (fileio_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -748,7 +778,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
   
   /* Request newpath using 'm' packet */
   newpath = (char *) alloca (new_len);
-  if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
+  if (fileio_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -825,7 +855,7 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
     }
   /* Request pathname using 'm' packet */
   pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  if (fileio_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -874,7 +904,7 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
   
   /* Request pathname using 'm' packet */
   pathname = (char *) alloca (namelength);
-  if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
+  if (fileio_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -898,7 +928,7 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
       host_to_fileio_stat (&st, &fst);
       host_to_fileio_uint (0, fst.fst_dev);
 
-      errno = target_write_memory (statptr, (gdb_byte *) &fst, sizeof fst);
+      errno = fileio_write_memory (statptr, (gdb_byte *) &fst, sizeof fst);
       if (errno != 0)
 	{
 	  remote_fileio_return_errno (remote, -1);
@@ -975,7 +1005,7 @@ remote_fileio_func_fstat (remote_target *remote, char *buf)
     {
       host_to_fileio_stat (&st, &fst);
 
-      errno = target_write_memory (ptrval, (gdb_byte *) &fst, sizeof fst);
+      errno = fileio_write_memory (ptrval, (gdb_byte *) &fst, sizeof fst);
       if (errno != 0)
 	{
 	  remote_fileio_return_errno (remote, -1);
@@ -1026,7 +1056,7 @@ remote_fileio_func_gettimeofday (remote_target *remote, char *buf)
     {
       remote_fileio_to_fio_timeval (&tv, &ftv);
 
-      errno = target_write_memory (ptrval, (gdb_byte *) &ftv, sizeof ftv);
+      errno = fileio_write_memory (ptrval, (gdb_byte *) &ftv, sizeof ftv);
       if (errno != 0)
 	{
 	  remote_fileio_return_errno (remote, -1);
@@ -1071,7 +1101,7 @@ remote_fileio_func_system (remote_target *remote, char *buf)
     {
       /* Request commandline using 'm' packet */
       cmdline = (char *) alloca (length);
-      if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
+      if (fileio_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
 	{
 	  remote_fileio_ioerror (remote);
 	  return;
diff --git a/gdb/remote.c b/gdb/remote.c
index aa6a67a96e0..e9230060927 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -274,6 +274,13 @@ class remote_state
      because we allow GDB commands while the target is running.  */
   bool waiting_for_stop_reply = false;
 
+  /* True if handling a File I/O request.  When this is true, we make
+     sure to skip telling the server to change its selected thread to
+     match the GDB-selected thread, which has no relation to the
+     thread that initiated the File I/O request.  Note the File I/O
+     packet doesn't tell us which thread or process that was.  */
+  bool handling_fileio_request = false;
+
   /* The status of the stub support for the various vCont actions.  */
   vCont_action_support supports_vCont;
   /* Whether vCont support was probed already.  This is a workaround
@@ -8232,11 +8239,13 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 	     for a stop reply.  See the comments in putpkt_binary.  Set
 	     waiting_for_stop_reply to 0 temporarily.  */
 	  rs->waiting_for_stop_reply = 0;
+	  rs->handling_fileio_request = true;
 	  remote_fileio_request (this, buf, rs->ctrlc_pending_p);
 	  rs->ctrlc_pending_p = 0;
 	  /* GDB handled the File-I/O request, and the target is running
 	     again.  Keep waiting for events.  */
 	  rs->waiting_for_stop_reply = 1;
+	  rs->handling_fileio_request = false;
 	  break;
 	case 'N': case 'T': case 'S': case 'X': case 'W':
 	  {
@@ -11208,16 +11217,22 @@ remote_target::xfer_partial (enum target_object object,
 			     const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
 			     ULONGEST *xfered_len)
 {
-  struct remote_state *rs;
   int i;
   char *p2;
   char query_type;
   int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());
 
-  set_remote_traceframe ();
-  set_general_thread (inferior_ptid);
+  remote_state *rs = get_remote_state ();
 
-  rs = get_remote_state ();
+  /* If we're handling a File I/O read/write request, we'll want to
+     access memory off of the thread that initiated the File I/O
+     request, not from whatever thread GDB had selected.  We don't
+     know what thread that is, but we don't need to know.  */
+  if (!rs->handling_fileio_request)
+    {
+      set_remote_traceframe ();
+      set_general_thread (inferior_ptid);
+    }
 
   /* Handle memory using the standard memory routines.  */
   if (object == TARGET_OBJECT_MEMORY)
diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
index 96d4611ee02..0e10e42218f 100644
--- a/gdb/target-dcache.c
+++ b/gdb/target-dcache.c
@@ -38,18 +38,33 @@ target_dcache_init_p (void)
   return (dcache != NULL);
 }
 
-/* Invalidate the target dcache.  */
+/* Invalidate the target dcache of PSPACE.  */
 
-void
-target_dcache_invalidate (void)
+static void
+target_dcache_invalidate_pspace (program_space *pspace)
 {
   DCACHE *dcache
-    = target_dcache_aspace_key.get (current_program_space->aspace);
+    = target_dcache_aspace_key.get (pspace->aspace);
 
   if (dcache != NULL)
     dcache_invalidate (dcache);
 }
 
+/* Invalidate the target dcache.  */
+
+void
+target_dcache_invalidate (void)
+{
+  target_dcache_invalidate_pspace (current_program_space);
+}
+
+void
+target_dcache_invalidate_all ()
+{
+  for (program_space *pspace : program_spaces)
+    target_dcache_invalidate_pspace (pspace);
+}
+
 /* Return the target dcache.  Return NULL if target dcache is not
    initialized yet.  */
 
diff --git a/gdb/target-dcache.h b/gdb/target-dcache.h
index 6f31a3d4482..2def8a2ee30 100644
--- a/gdb/target-dcache.h
+++ b/gdb/target-dcache.h
@@ -22,6 +22,9 @@
 
 extern void target_dcache_invalidate (void);
 
+/* Invalidate the dcache of all program spaces.  */
+extern void target_dcache_invalidate_all ();
+
 extern DCACHE *target_dcache_get (void);
 
 extern DCACHE *target_dcache_get_or_init (void);

base-commit: c6479f8b2ab32c8a1659054104f2d0176a466cb3
-- 
2.26.2