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

[Adrian Oltean <adrian.oltean@nxp.com>]

Hi Pedro,

Thanks a lot for the extra details provided. I did not have a chance to test yet the patch you sent below. At first sight, the 
changes are straight-forward and make sense (even with my knowledge in GDB's internals). I also changed the 'H' packet
handling in the stub per previous emails.

Are you planning to commit the patch you proposed? I'm asking this because I have a GDB fork that I use for a custom
build but I'd rather not maintain a patch that won't be on the mainline.

Thank you,
Adrian

-----Original Message-----
From: Pedro Alves <pedro@palves.net> 
Sent: Thursday, March 10, 2022 2:33 PM
To: Adrian Oltean <adrian.oltean@nxp.com>; Tom Tromey <tom@tromey.com>
Cc: Adrian Oltean via Gdb <gdb@sourceware.org>
Subject: Re: [EXT] Re: Semihosting in GDB 11.1 | Proposed patch for interrupting in sync mode

Caution: EXT Email

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