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

Message ID 20230801132046.3465441-1-tankut.baris.aktemur@intel.com
State New
Headers
Series [v2] gdbserver: select a thread, if necessary, to access memory (was: [PATCH] gdbserver: try selecting a thread first to access memory) |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Aktemur, Tankut Baris Aug. 1, 2023, 1:20 p.m. UTC
  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(-)
  

Comments

Aktemur, Tankut Baris Nov. 21, 2023, 7:45 p.m. UTC | #1
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
  

Patch

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;