[PATCH] gdbserver: try selecting a thread first to access memory

[PATCH] gdbserver: try selecting a thread first to access memory

[Tankut Baris Aktemur]

Since commit

  commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
  Author: Pedro Alves <pedro@palves.net>
  Date:   Tue Apr 5 13:57:11 2022 +0100

    gdbserver: track current process as well as current thread

gdbserver switches to a process, rather than a thread, before
processing a memory access request coming from the GDB side.  This
switch sets current_thread to null.  Some memory accesses on certain
targets, however, may require having a thread context.  Therefore, try
switching to the selected thread first.  If this fails for a reason
(e.g.  the thread has exited), switch to the process.

Regression-tested on X86_64 Linux using the unix, native-gdbserver,
and native-extended-gdbserver board files.
---
 gdbserver/server.cc | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..0344fba32a7 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1072,7 +1072,10 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
       /* (assume no half-trace half-real blocks for now) */
     }
 
-  if (set_desired_process ())
+  /* Some memory accesses may require having a thread context.
+     Attempt switching to the selected thread first, with a fall-back
+     option to a process.  */
+  if (set_desired_thread () || set_desired_process ())
     res = read_inferior_memory (memaddr, myaddr, len);
   else
     res = 1;
@@ -1093,7 +1096,10 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
     {
       int ret;
 
-      if (set_desired_process ())
+      /* Some memory accesses may require having a thread context.
+	 Attempt switching to the selected thread first, with a
+	 fall-back option to a process.  */
+      if (set_desired_thread () || set_desired_process ())
 	ret = target_write_memory (memaddr, myaddr, len);
       else
 	ret = EIO;
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdbserver: try selecting a thread first to access memory

[Pedro Alves]

On 2023-06-20 09:35, Tankut Baris Aktemur via Gdb-patches wrote:
> Since commit
> 
>   commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
>   Author: Pedro Alves <pedro@palves.net>
>   Date:   Tue Apr 5 13:57:11 2022 +0100
> 
>     gdbserver: track current process as well as current thread
> 
> gdbserver switches to a process, rather than a thread, before
> processing a memory access request coming from the GDB side.  This
> switch sets current_thread to null.  Some memory accesses on certain
> targets, however, may require having a thread context.  Therefore, try
> switching to the selected thread first.  If this fails for a reason
> (e.g.  the thread has exited), switch to the process.

The reason I liked always selecting a process (with thread == null), is
that it made the corner case be the case that runs all the time.  I.e., any
target backend that incorrectly assumes there's always a thread selected,
is immediately exposed to the problem scenario and is forced to fix it.
Trying to select the thread first loses that.

If the access is for a thread-specific address space, and the thread exited,
what is supposed to happen?  Is the backend supposed to error out in that
scenario?  Or will it misbehave?

How do you tell what address space the access is targeting?  Are you encoding
that in the address, or do you have more changes to gdbserver, and maybe
RSP extensions?

I'm wondering whether we should have something like:

  if ((aspace_thread_specific (...) && set_desired_thread ())
      || set_desired_process ())
     res = read_inferior_memory (memaddr, myaddr, len);
  else
     res = 1;

RE: [PATCH] gdbserver: try selecting a thread first to access memory

[Aktemur, Tankut Baris]

On Tuesday, July 18, 2023 3:09 PM, Pedro Alves wrote:
> On 2023-06-20 09:35, Tankut Baris Aktemur via Gdb-patches wrote:
> > Since commit
> >
> >   commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
> >   Author: Pedro Alves <pedro@palves.net>
> >   Date:   Tue Apr 5 13:57:11 2022 +0100
> >
> >     gdbserver: track current process as well as current thread
> >
> > gdbserver switches to a process, rather than a thread, before
> > processing a memory access request coming from the GDB side.  This
> > switch sets current_thread to null.  Some memory accesses on certain
> > targets, however, may require having a thread context.  Therefore, try
> > switching to the selected thread first.  If this fails for a reason
> > (e.g.  the thread has exited), switch to the process.
> 
> The reason I liked always selecting a process (with thread == null), is
> that it made the corner case be the case that runs all the time.  I.e., any
> target backend that incorrectly assumes there's always a thread selected,
> is immediately exposed to the problem scenario and is forced to fix it.
> Trying to select the thread first loses that.

I'll refer to this comment below as [1].

> If the access is for a thread-specific address space, and the thread exited,
> what is supposed to happen?  Is the backend supposed to error out in that
> scenario?  Or will it misbehave?

I'd expect the memory access at the target side to error out.  Gdbserver would
then convert this to an 'E' response.

> How do you tell what address space the access is targeting?  Are you encoding
> that in the address, or do you have more changes to gdbserver, and maybe
> RSP extensions?

Currently we're encoding the address space in the address (upper 3 bits).

> I'm wondering whether we should have something like:
> 
>   if ((aspace_thread_specific (...) && set_desired_thread ())
>       || set_desired_process ())
>      res = read_inferior_memory (memaddr, myaddr, len);
>   else
>      res = 1;

I believe `aspace_thread_specific` would have to be a target-specific
logic.  What you referred to in the comment [1] above, i.e.  "immediately
exposed to the problem scenario and is forced to fix it", would be caught
and the target would fix the problem by implementing `aspace_thread_specific`
appropriately.  This makes sense to me.  An alternative would be that
the target calls `set_desired_thread` itself, before accessing the memory,
if it detects that the request is thread-specific.  I don't see any low-target
using `set_desired_thread` ever in the gdbserver codebase, though.  So, it
might be considered a breakage of abstraction.  Hence, your suggestion looks
favorable.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdbserver: try selecting a thread first to access memory

[Pedro Alves]

On 2023-07-18 14:36, Aktemur, Tankut Baris wrote:
> On Tuesday, July 18, 2023 3:09 PM, Pedro Alves wrote:

[ snip answers; thanks! ]

>> I'm wondering whether we should have something like:
>>
>>   if ((aspace_thread_specific (...) && set_desired_thread ())
>>       || set_desired_process ())
>>      res = read_inferior_memory (memaddr, myaddr, len);
>>   else
>>      res = 1;
> 
> I believe `aspace_thread_specific` would have to be a target-specific
> logic.  

Right.

> What you referred to in the comment [1] above, i.e.  "immediately
> exposed to the problem scenario and is forced to fix it", would be caught
> and the target would fix the problem by implementing `aspace_thread_specific`
> appropriately.  This makes sense to me.  

Exactly.

> An alternative would be that
> the target calls `set_desired_thread` itself, before accessing the memory,
> if it detects that the request is thread-specific.  I don't see any low-target
> using `set_desired_thread` ever in the gdbserver codebase, though.  So, it
> might be considered a breakage of abstraction.  Hence, your suggestion looks
> favorable.

Yeah, that'd be an abstraction violation.  Ideally, the gdbserver backends would
have no RSP awareness at all, and would even be reusable by GDB.
Many years ago, the abstraction didn't exist as it does today, and the backends
would even peek into the remote protocol message buffer.  That was all separated
out with the non-stop and multi-process work, IIRC, where gdbserver migrated to a
target_ops interface similar to GDB's.

Pedro Alves

RE: [PATCH] gdbserver: try selecting a thread first to access memory

[Aktemur, Tankut Baris]

On Tuesday, July 18, 2023 4:03 PM, Pedro Alves wrote:
> On 2023-07-18 14:36, Aktemur, Tankut Baris wrote:
> > On Tuesday, July 18, 2023 3:09 PM, Pedro Alves wrote:
> 
> [ snip answers; thanks! ]
> 
> >> I'm wondering whether we should have something like:
> >>
> >>   if ((aspace_thread_specific (...) && set_desired_thread ())
> >>       || set_desired_process ())
> >>      res = read_inferior_memory (memaddr, myaddr, len);
> >>   else
> >>      res = 1;
> >
> > I believe `aspace_thread_specific` would have to be a target-specific
> > logic.
> 
> Right.
> 
> > What you referred to in the comment [1] above, i.e.  "immediately
> > exposed to the problem scenario and is forced to fix it", would be caught
> > and the target would fix the problem by implementing `aspace_thread_specific`
> > appropriately.  This makes sense to me.
> 
> Exactly.
> 
> > An alternative would be that
> > the target calls `set_desired_thread` itself, before accessing the memory,
> > if it detects that the request is thread-specific.  I don't see any low-target
> > using `set_desired_thread` ever in the gdbserver codebase, though.  So, it
> > might be considered a breakage of abstraction.  Hence, your suggestion looks
> > favorable.
> 
> Yeah, that'd be an abstraction violation.  Ideally, the gdbserver backends would
> have no RSP awareness at all, and would even be reusable by GDB.
> Many years ago, the abstraction didn't exist as it does today, and the backends
> would even peek into the remote protocol message buffer.  That was all separated
> out with the non-stop and multi-process work, IIRC, where gdbserver migrated to a
> target_ops interface similar to GDB's.

Ack. 

I'll revise the patch accordingly and send an update as soon as I can.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2] gdbserver: select a thread, if necessary, to access memory (was: [PATCH] gdbserver: try selecting a thread first to access memory)

[Tankut Baris Aktemur]

Since commit

  commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
  Author: Pedro Alves <pedro@palves.net>
  Date:   Tue Apr 5 13:57:11 2022 +0100

    gdbserver: track current process as well as current thread

gdbserver switches to a process, rather than a thread, before
processing a memory access request coming from the GDB side.  This
switch sets current_thread to null.  Some memory accesses on certain
targets, however, may require having a thread context.  Therefore,
switch to the selected thread, if the target would require a thread
context.
---
 gdbserver/server.cc | 16 ++++++++++++++--
 gdbserver/target.cc |  6 ++++++
 gdbserver/target.h  |  7 +++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..44642a64215 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1072,7 +1072,13 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
       /* (assume no half-trace half-real blocks for now) */
     }
 
-  if (set_desired_process ())
+  /* Some memory accesses may require having a thread context.
+     Attempt switching to the selected thread, if necessary.
+     Otherwise use the process as the context.  */
+  bool need_thread = the_target->memaddr_is_thread_specific (memaddr);
+
+  if ((need_thread && set_desired_thread ())
+      || (!need_thread && set_desired_process ()))
     res = read_inferior_memory (memaddr, myaddr, len);
   else
     res = 1;
@@ -1093,7 +1099,13 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
     {
       int ret;
 
-      if (set_desired_process ())
+      /* Some memory accesses may require having a thread context.
+	 Attempt switching to the selected thread, if necessary.
+	 Otherwise use the process as the context.  */
+      bool need_thread = the_target->memaddr_is_thread_specific (memaddr);
+
+      if ((need_thread && set_desired_thread ())
+	  || (!need_thread && set_desired_process ()))
 	ret = target_write_memory (memaddr, myaddr, len);
       else
 	ret = EIO;
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index f8e592d20c3..85efe6d88c6 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -832,3 +832,9 @@ process_stratum_target::get_ipa_tdesc_idx ()
 {
   return 0;
 }
+
+bool
+process_stratum_target::memaddr_is_thread_specific (CORE_ADDR memaddr)
+{
+  return false;
+}
diff --git a/gdbserver/target.h b/gdbserver/target.h
index d993e361b76..bde2fb2eb4c 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -508,6 +508,13 @@ class process_stratum_target
      Returns true if successful and false otherwise.  */
   virtual bool store_memtags (CORE_ADDR address, size_t len,
 			      const gdb::byte_vector &tags, int type);
+
+  /* Some memory addresses may require having a thread context.  E.g.:
+     an address whose upper bits encode a thread-specific address
+     space.  Let the target tell if MEMADDR is such an address, so
+     that the server can attempt switching the context before
+     accessing the memory.  */
+  virtual bool memaddr_is_thread_specific (CORE_ADDR memaddr);
 };
 
 extern process_stratum_target *the_target;
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2] gdbserver: select a thread, if necessary, to access memory (was: [PATCH] gdbserver: try selecting a thread first to access memory)

[Aktemur, Tankut Baris]

Kindly pinging. 

Regards
-Baris

On Tuesday, August 1, 2023 3:21 PM, Aktemur, Tankut Baris wrote:
> 
> Since commit
> 
>   commit 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
>   Author: Pedro Alves <pedro@palves.net>
>   Date:   Tue Apr 5 13:57:11 2022 +0100
> 
>     gdbserver: track current process as well as current thread
> 
> gdbserver switches to a process, rather than a thread, before
> processing a memory access request coming from the GDB side.  This
> switch sets current_thread to null.  Some memory accesses on certain
> targets, however, may require having a thread context.  Therefore,
> switch to the selected thread, if the target would require a thread
> context.
> ---
>  gdbserver/server.cc | 16 ++++++++++++++--
>  gdbserver/target.cc |  6 ++++++
>  gdbserver/target.h  |  7 +++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index c57270175b4..44642a64215 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1072,7 +1072,13 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int
> len)
>        /* (assume no half-trace half-real blocks for now) */
>      }
> 
> -  if (set_desired_process ())
> +  /* Some memory accesses may require having a thread context.
> +     Attempt switching to the selected thread, if necessary.
> +     Otherwise use the process as the context.  */
> +  bool need_thread = the_target->memaddr_is_thread_specific (memaddr);
> +
> +  if ((need_thread && set_desired_thread ())
> +      || (!need_thread && set_desired_process ()))
>      res = read_inferior_memory (memaddr, myaddr, len);
>    else
>      res = 1;
> @@ -1093,7 +1099,13 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char
> *myaddr, int len)
>      {
>        int ret;
> 
> -      if (set_desired_process ())
> +      /* Some memory accesses may require having a thread context.
> +	 Attempt switching to the selected thread, if necessary.
> +	 Otherwise use the process as the context.  */
> +      bool need_thread = the_target->memaddr_is_thread_specific (memaddr);
> +
> +      if ((need_thread && set_desired_thread ())
> +	  || (!need_thread && set_desired_process ()))
>  	ret = target_write_memory (memaddr, myaddr, len);
>        else
>  	ret = EIO;
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index f8e592d20c3..85efe6d88c6 100644
> --- a/gdbserver/target.cc
> +++ b/gdbserver/target.cc
> @@ -832,3 +832,9 @@ process_stratum_target::get_ipa_tdesc_idx ()
>  {
>    return 0;
>  }
> +
> +bool
> +process_stratum_target::memaddr_is_thread_specific (CORE_ADDR memaddr)
> +{
> +  return false;
> +}
> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index d993e361b76..bde2fb2eb4c 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -508,6 +508,13 @@ class process_stratum_target
>       Returns true if successful and false otherwise.  */
>    virtual bool store_memtags (CORE_ADDR address, size_t len,
>  			      const gdb::byte_vector &tags, int type);
> +
> +  /* Some memory addresses may require having a thread context.  E.g.:
> +     an address whose upper bits encode a thread-specific address
> +     space.  Let the target tell if MEMADDR is such an address, so
> +     that the server can attempt switching the context before
> +     accessing the memory.  */
> +  virtual bool memaddr_is_thread_specific (CORE_ADDR memaddr);
>  };
> 
>  extern process_stratum_target *the_target;
> --
> 2.34.1


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928