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

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

[Adrian Oltean]

Hi Tom,

Unfortunately, I could not find ways to adapt all the other components I have around GDB to 
work with target-async mode enabled. It is what it is...

Regarding the 'H' packet, I had a second look in the RSP specs where it's stated that '0' indicates
an arbitrary process/thread. So, as you mentioned, I changed the GDB server code to simply
ensure there's a valid thread stopped marked as 'current'. The original implementation incorrectly
assimilated '0' with 'first thread' (always).

Thank you,
Adrian

-----Original Message-----
From: Tom Tromey <tom@tromey.com> 
Sent: Wednesday, March 9, 2022 12:10 AM
To: Adrian Oltean via Gdb <gdb@sourceware.org>
Cc: Adrian Oltean <adrian.oltean@nxp.com>
Subject: [EXT] Re: Semihosting in GDB 11.1 | Proposed patch for interrupting in sync mode

Caution: EXT Email

>>>>> Adrian Oltean via Gdb <gdb@sourceware.org> writes:

> 1. I need to run GDB client in all-stop mode with target-async set to off.
> This seems to be a must in order to make some python scripts 
> (containing
> gdb.post_event('continue')) interact correctly with Eclipse CDT and 
> with some automation I have around the python scripts.

I think it would be better on the whole to figure out a way to make this work with target-async.  In the long run I'd prefer that gdb remove sync target wait entirely -- that the default be target-async everywhere.

> but I'd like to know if there's any chance I'll break any use case 
> with these changes.

I don't have the answer to that I'm afraid.

> Is the described behavior a known issue inside GDB? Or do I have to 
> change something in the server to correct the described behavior?

>   [remote] Sending packet: $Hg0#df

I think this request means to switch to any thread for subsequent non-continue/step requests.  So, ignoring it is almost the right thing to do, I think the idea is just to ensure that some stopped thread is selected.

Tom

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

[Tom Tromey]

Adrian> Unfortunately, I could not find ways to adapt all the other components I have around GDB to 
Adrian> work with target-async mode enabled. It is what it is...

I'm a little lost about this, because my mental model is that target
async shouldn't really change anything.  I think it is just an
implementation detail about where gdb waits for events -- is it in
wait() or in select()?

So, it would be interesting to learn what the differences are.  Perhaps
this can be patched up.

Adrian> Regarding the 'H' packet, I had a second look in the RSP specs
Adrian> where it's stated that '0' indicates an arbitrary
Adrian> process/thread. So, as you mentioned, I changed the GDB server
Adrian> code to simply ensure there's a valid thread stopped marked as
Adrian> 'current'. The original implementation incorrectly assimilated
Adrian> '0' with 'first thread' (always).

I'm a little surprised this matters as well, but anyway I think it's
fine to submit a patch like this.  Maybe others have some idea of what's
going on.

Tom

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

[Pedro Alves]

On 2022-03-09 19:49, Tom Tromey wrote:
> Adrian> Unfortunately, I could not find ways to adapt all the other components I have around GDB to 
> Adrian> work with target-async mode enabled. It is what it is...
> 
> I'm a little lost about this, because my mental model is that target
> async shouldn't really change anything.  I think it is just an
> implementation detail about where gdb waits for events -- is it in
> wait() or in select()?
> 
> So, it would be interesting to learn what the differences are.  Perhaps
> this can be patched up.
> 
> Adrian> Regarding the 'H' packet, I had a second look in the RSP specs
> Adrian> where it's stated that '0' indicates an arbitrary
> Adrian> process/thread. So, as you mentioned, I changed the GDB server
> Adrian> code to simply ensure there's a valid thread stopped marked as
> Adrian> 'current'. The original implementation incorrectly assimilated
> Adrian> '0' with 'first thread' (always).
> 
> I'm a little surprised this matters as well, but anyway I think it's
> fine to submit a patch like this.  Maybe others have some idea of what's
> going on.

I'm not sure that I get what Adrian is callilng "first thread", but I take
it to mean that the server was numbering threads starting at "0".
But, in the RSP, you can't use '0' as thread id, as it has a special meaning.
I.e., if the server is numbering threads like, 0,1,2,3,... then it will
run into problems, because when GDB says "Hg0", it doesn't mean "select
the first thread, the one with id "0".  The typical fix is to just start
numbering threads at 1 instead of 0, for example.

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

[Adrian Oltean]

Hi guys,

Unfortunately, my knowledge on GDB's internals is quite limited... I'm leaving below two old mail threads, [1] and [2],
that describe the GDB's behavior in a custom use case and what makes it difficult to handle when target-async
is enabled. GDB 8.3 is referred in those threads but behavior is mostly the same in GDB 11.1. The key is the execution
of post_event("cont").

As far as 'H' packet handling is concerned, note that the GDB server I'm referring is a proprietary implementation
that is used for JTAG debugging. Threads are numbered from 1. However, in the old implementation "Hg0" was always
switching to thread 1 ("first thread") internally, even though some other core/thread was suspended. The original
implementation assumed "Hg0" must *always* switch to "first thread" (thread 1 in my case) but this doesn't seem
to be enforced in the RSP specs. I updated the code and assimilate "arbitrary" (from spec) as "if there's no suspended
thread/core internally selected, find a valid one".

Thank you,
Adrian

[1] https://sourceware.org/pipermail/gdb/2020-February/048395.html
[2] https://www.eclipse.org/lists/cdt-dev/msg35130.html

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

Caution: EXT Email

On 2022-03-09 19:49, Tom Tromey wrote:
> Adrian> Unfortunately, I could not find ways to adapt all the other 
> Adrian> components I have around GDB to work with target-async mode enabled. It is what it is...
>
> I'm a little lost about this, because my mental model is that target 
> async shouldn't really change anything.  I think it is just an 
> implementation detail about where gdb waits for events -- is it in
> wait() or in select()?
>
> So, it would be interesting to learn what the differences are.  
> Perhaps this can be patched up.
>
> Adrian> Regarding the 'H' packet, I had a second look in the RSP specs 
> Adrian> where it's stated that '0' indicates an arbitrary 
> Adrian> process/thread. So, as you mentioned, I changed the GDB server 
> Adrian> code to simply ensure there's a valid thread stopped marked as 
> Adrian> 'current'. The original implementation incorrectly assimilated 
> Adrian> '0' with 'first thread' (always).
>
> I'm a little surprised this matters as well, but anyway I think it's 
> fine to submit a patch like this.  Maybe others have some idea of 
> what's going on.

I'm not sure that I get what Adrian is callilng "first thread", but I take it to mean that the server was numbering threads starting at "0".
But, in the RSP, you can't use '0' as thread id, as it has a special meaning.
I.e., if the server is numbering threads like, 0,1,2,3,... then it will run into problems, because when GDB says "Hg0", it doesn't mean "select the first thread, the one with id "0".  The typical fix is to just start numbering threads at 1 instead of 0, for example.

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

[Pedro Alves]

On 2022-03-10 10:02, Adrian Oltean wrote:

> As far as 'H' packet handling is concerned, note that the GDB server I'm referring is a proprietary implementation
> that is used for JTAG debugging. Threads are numbered from 1. However, in the old implementation "Hg0" was always
> switching to thread 1 ("first thread") internally, even though some other core/thread was suspended. The original
> implementation assumed "Hg0" must *always* switch to "first thread" (thread 1 in my case) but this doesn't seem
> to be enforced in the RSP specs. I updated the code and assimilate "arbitrary" (from spec) as "if there's no suspended
> thread/core internally selected, find a valid one".

So going back to your original problem description, you said:

~~~~
2. File I/O (semihosting) when a multicore (ARMv8) target is involved seems
to exhibit an issue. My multicore debugging model assumes that each core is a
separate thread. If I have 'printf' calls executed on separate cores, I noticed
that the messages are duplicated on secondary cores (other than core 0). The
remote log snippet that highlights the root cause is pasted below. Long story
short, the 'H' packet that is sent to the server and instructs it to use
thread 0 for some further operations, actually causes troubles because breakpoint
handling and run control gets broken afterwards. Note that the 'Fwrite' is the
result of a 'printf' on a secondary core, thus instructing the server to use
thread 0 breaks lots of things inside the GDB server. The 'H' packet seems to
be sent as a result of the 'continue' action and it translates into a call to
'switch_to_no_thread'. If I ignore the 'H' packet, semihosting breakpoints and run
control are correctly handled, so the second 'Fwrite' RSP sequence would not
exist (this is the expected behavior). Is the described behavior a known issue
inside GDB? Or do I have to change something in the server to correct the
described behavior?

[remote] Sending packet: $vCont;c#a8
[remote] Received Ack
[remote] wait: enter
  [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
  [remote] Received Ack
  [remote] Packet received: Fwrite,1,80026300,12
  [remote] Sending packet: $m80026300,12#8f
  [remote] Received Ack
  [remote] Packet received: 48656c6c6f2066726f6d20436f726523370a
  [remote] Sending packet: $F12#a9
  [remote] Received Ack
  [remote] Packet received: T05thread:p1.8;core:7;
[remote] wait: exit
~~~~

The printf calls end up calling write under the hood, of course, and those
syscalls are intercepted and translated to File I/O calls into GDB.  That's the
"Fwrite,1,80026300,12" packets.  Now, GDB processes those and writes to its
own stdout.  What is written to its stdout is 12 bytes found at 0x80026300 in
the inferior memory, as per the Fwrite packet's arguments.

Depending on the kind of target object is being accessed, GDB makes sure
to select the proper remote thread, or at least any thread of the right remote process.
This is important for when the remote side understands the multi-process extensions,
for example, lest GDB reads something of the wrong process.

So in remote.c, you'll see calls to set_general_process, like this:

  /* Make sure the remote is pointing at the right process.  */
  set_general_process ();

... which translate to "HgPID.0" when multi-process extensions are supported by
the server, and "Hg0" when they're not.

For memory reads in particular, GDB instead makes sure that GDB's selected thread
is selected on the server side as well, not just any thread of the gdb-selected process.
This is done here:

 enum target_xfer_status
 remote_target::xfer_partial (enum target_object object,
 			     const char *annex, gdb_byte *readbuf,
 			     const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
 			     ULONGEST *xfered_len)
 {
 ...
   set_general_thread (inferior_ptid);


So what I think is happening is that inferior_ptid is null_ptid when you get here.
We always switch to null_ptid when waiting for events out of the target, exactly
to better uncover spots that would be relying on some stale non-null inferior_ptid
by accident.  This would be such a case.  The inferior is running, and we're waiting
for events, so whatever was the gdb-selected thread (per inferior_ptid) has no bearing
on what thread is reporting some event.

If the server supports multi-process extensions, then that set_general_thread call above
with inferior_ptid == null_ptid will end up sending "Hg0.0", meaning, select any thread of any process,
which means the subsequent memory read (m packet) will potentially read memory off of
the wrong process.  This is obviously bad.

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.

Pedro Alves

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

[Pedro Alves]

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

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

[Adrian Oltean]

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

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

[Pedro Alves]

Hi Adrian,

On 2022-03-14 07:33, Adrian Oltean wrote:
> 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.

I think we can consider the patch for inclusion, yes, but I'd like to hear whether it actually works for
you as intended before sending it to the gdb-patches list for proper review.