[PATCH 0/2] Fix gdbserver/linux memory access regression

[PATCH 0/2] Fix gdbserver/linux memory access regression

[Pedro Alves]

The recent change to make GDBserver always access memory via
/proc/pid/mem caused a regression in
gdb.threads/access-mem-running-thread-exit.exp that I somehow missed.
This is actually a pre-existing GDBserver issue being now exposed.

Patch #2 fixes the GDBserver bug.

Patch #1 fixes the gdb.threads/access-mem-running-thread-exit.exp
testcase itself -- it doesn't run properly against
--target_board=native-extended-gdbserver today.

Pedro Alves (2):
  Fix gdb.threads/access-mem-running-thread-exit.exp w/
    native-extended-gdbserver
  gdbserver: track current process as well as current thread

 .../access-mem-running-thread-exit.exp        | 30 ++++++++++++-
 gdbserver/gdbthread.h                         |  1 +
 gdbserver/inferiors.cc                        | 26 ++++++++---
 gdbserver/server.cc                           |  4 +-
 gdbserver/target.cc                           | 44 ++++++++++++++++++-
 gdbserver/target.h                            | 15 ++++++-
 6 files changed, 108 insertions(+), 12 deletions(-)


base-commit: 6ea673e2d643b7b2283aa73d35b02dfc9aa7115f
-- 
2.26.2

[PATCH 1/2] Fix gdb.threads/access-mem-running-thread-exit.exp w/ native-extended-gdbserver

[Pedro Alves]

When testing gdb.threads/access-mem-running-thread-exit.exp with
--target_board=native-extended-gdbserver, we get:

  Running gdb.threads/access-mem-running-thread-exit.exp ...
  FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: second inferior: runto: run to main
  WARNING: Timed out waiting for EOF in server after monitor exit

		  === gdb Summary ===

  # of expected passes            3
  # of unexpected failures        1
  # of unsupported tests          1

The problem is that the testcase spawns a second inferior with
-no-connection, and then runto_main does "run", which fails like so:

 (gdb) run
 Don't know how to run.  Try "help target".
 (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: second inferior: runto: run to main

That "run" above failed because native-extended-gdbserver forces "set
auto-connect-native-target off", to prevent testcases from mistakenly
running programs with the native target, which would exactly be the
case here.

Fix this by letting the second inferior share the first inferior's
connection everywhere except on targets that do reload on run (e.g.,
--target_board=native-gdbserver).

Change-Id: Ib57105a238cbc69c57220e71261219fa55d329ed
---
 .../access-mem-running-thread-exit.exp        | 30 ++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
index dddb35dabc0..8dc9af05c8e 100644
--- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
+++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
@@ -79,7 +79,35 @@ proc test { non_stop } {
 
     # Start the second inferior.
     with_test_prefix "second inferior" {
-	gdb_test "add-inferior -no-connection" "New inferior 2.*"
+	# With stub targets that do reload on run, if we let the new
+	# inferior share inferior 1's connection, runto_main would
+	# fail because GDB is already connected to something, like
+	# e.g. with --target_board=native-gdbserver:
+	#
+	#  (gdb) kill
+	#  ...
+	#  (gdb) target remote localhost:2348
+	#  Already connected to a remote target.  Disconnect? (y or n)
+	#
+	# Instead, start the inferior with no connection, and let
+	# gdb_load/runto_main spawn a new remote connection/gdbserver.
+	#
+	# OTOH, with extended-remote, we must let the new inferior
+	# reuse the current connection, so that runto_main below can
+	# issue the "run" command, and have the inferior run on the
+	# remote target.  If we forced no connection, then "run" would
+	# either fail if "set auto-connect-native-target" is on, like
+	# the native-extended-gdbserver board enforces, or it would
+	# run the inferior on the native target, which isn't what is
+	# being tested.
+	#
+	# Since it's reload_on_run targets that need special care, we
+	# default to reusing the connection on most targets.
+	if [target_info exists gdb,do_reload_on_run] {
+	    gdb_test "add-inferior -no-connection" "New inferior 2.*"
+	} else {
+	    gdb_test "add-inferior" "New inferior 2.*"
+	}
 	gdb_test "inferior 2" "Switching to inferior 2 .*"
 
 	gdb_load $binfile
-- 
2.26.2

[PATCH 2/2] gdbserver: track current process as well as current thread

[Pedro Alves]

The recent commit 421490af33bf ("gdbserver/linux: Access memory even
if threads are running") caused a regression in
gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I
somehow missed.  Like so:

 (gdb) print global_var
 Cannot access memory at address 0x555555558010
 (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem (print global_var after writing, inf=2, iter=1)

The problem starts with GDB telling GDBserver to select a thread, via
the Hg packet, which GDBserver complies with, then that thread exits,
and GDB, without knowing the thread is gone, tries to write to memory,
through the context of the previously selected Hg thread.

GDBserver's GDB-facing memory access routines, gdb_read_memory and
gdb_write_memory, call set_desired_thread to make GDBserver re-select
the thread that GDB has selected with the Hg packet.  Since the thread
is gone, set_desired_thread returns false, and the memory access
fails.

Now, to access memory, it doesn't really matter which thread is
selected.  All we should need is the target process.  Even if the
thread that GDB previously selected is gone, and GDB does not yet know
about that exit, it shouldn't matter, GDBserver should still know
which process that thread belonged to.

Fix this by making GDBserver track the current process separately,
like GDB also does.  Add a new set_desired_process routine that is
similar to set_desired_thread, but just sets the current process,
leaving the current thread as NULL.  Use it in the GDB-facing memory
read and write routines, to avoid failing if the selected thread is
gone, but the process is still around.

Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd
---
 gdbserver/gdbthread.h  |  1 +
 gdbserver/inferiors.cc | 26 +++++++++++++++++++------
 gdbserver/server.cc    |  4 ++--
 gdbserver/target.cc    | 44 ++++++++++++++++++++++++++++++++++++++++--
 gdbserver/target.h     | 15 +++++++++++++-
 5 files changed, 79 insertions(+), 11 deletions(-)

diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 10ae1cb7c87..8b897e73d33 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -252,6 +252,7 @@ class scoped_restore_current_thread
 
 private:
   bool m_dont_restore = false;
+  process_info *m_process;
   thread_info *m_thread;
 };
 
diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
index 678d93c77a1..3d0a8b0e716 100644
--- a/gdbserver/inferiors.cc
+++ b/gdbserver/inferiors.cc
@@ -26,6 +26,11 @@
 std::list<process_info *> all_processes;
 std::list<thread_info *> all_threads;
 
+/* The current process.  */
+static process_info *current_process_;
+
+/* The current thread.  This is either a thread of CURRENT_PROCESS, or
+   NULL.  */
 struct thread_info *current_thread;
 
 /* The current working directory used to start the inferior.
@@ -130,6 +135,7 @@ clear_inferiors (void)
   clear_dlls ();
 
   switch_to_thread (nullptr);
+  current_process_ = nullptr;
 }
 
 struct process_info *
@@ -153,6 +159,8 @@ remove_process (struct process_info *process)
   free_all_breakpoints (process);
   gdb_assert (find_thread_process (process) == NULL);
   all_processes.remove (process);
+  if (current_process () == process)
+    switch_to_process (nullptr);
   delete process;
 }
 
@@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread)
 struct process_info *
 current_process (void)
 {
-  gdb_assert (current_thread != NULL);
-  return get_thread_process (current_thread);
+  return current_process_;
 }
 
 /* See gdbsupport/common-gdbthread.h.  */
@@ -223,6 +230,10 @@ switch_to_thread (process_stratum_target *ops, ptid_t ptid)
 void
 switch_to_thread (thread_info *thread)
 {
+  if (thread != nullptr)
+    current_process_ = get_thread_process (thread);
+  else
+    current_process_ = nullptr;
   current_thread = thread;
 }
 
@@ -231,9 +242,8 @@ switch_to_thread (thread_info *thread)
 void
 switch_to_process (process_info *proc)
 {
-  int pid = pid_of (proc);
-
-  switch_to_thread (find_any_thread_of_pid (pid));
+  current_process_ = proc;
+  current_thread = nullptr;
 }
 
 /* See gdbsupport/common-inferior.h.  */
@@ -254,6 +264,7 @@ set_inferior_cwd (std::string cwd)
 
 scoped_restore_current_thread::scoped_restore_current_thread ()
 {
+  m_process = current_process_;
   m_thread = current_thread;
 }
 
@@ -262,5 +273,8 @@ scoped_restore_current_thread::~scoped_restore_current_thread ()
   if (m_dont_restore)
     return;
 
-  switch_to_thread (m_thread);
+  if (m_thread != nullptr)
+    switch_to_thread (m_thread);
+  else
+    switch_to_process (m_process);
 }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 33c42714e72..f9c02a9c6da 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1067,7 +1067,7 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
       /* (assume no half-trace half-real blocks for now) */
     }
 
-  if (set_desired_thread ())
+  if (set_desired_process ())
     res = read_inferior_memory (memaddr, myaddr, len);
   else
     res = 1;
@@ -1088,7 +1088,7 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
     {
       int ret;
 
-      if (set_desired_thread ())
+      if (set_desired_process ())
 	ret = target_write_memory (memaddr, myaddr, len);
       else
 	ret = EIO;
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index 883242377c0..adcfe6e7bcc 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -29,16 +29,56 @@
 
 process_stratum_target *the_target;
 
-int
+/* See target.h.  */
+
+bool
 set_desired_thread ()
 {
   client_state &cs = get_client_state ();
   thread_info *found = find_thread_ptid (cs.general_thread);
 
-  switch_to_thread (found);
+  if (found == nullptr)
+    {
+      process_info *proc = find_process_pid (cs.general_thread.pid ());
+      if (proc == nullptr)
+	{
+	  threads_debug_printf
+	    ("did not find thread nor process for general_thread %s",
+	     cs.general_thread.to_string ().c_str ());
+	}
+      else
+	{
+	  threads_debug_printf
+	    ("did not find thread for general_thread %s, but found process",
+	     cs.general_thread.to_string ().c_str ());
+	}
+      switch_to_process (proc);
+    }
+  else
+    switch_to_thread (found);
+
   return (current_thread != NULL);
 }
 
+/* See target.h.  */
+
+bool
+set_desired_process ()
+{
+  client_state &cs = get_client_state ();
+
+  process_info *proc = find_process_pid (cs.general_thread.pid ());
+  if (proc == nullptr)
+    {
+      threads_debug_printf
+	("did not find process for general_thread %s",
+	 cs.general_thread.to_string ().c_str ());
+    }
+  switch_to_process (proc);
+
+  return proc != nullptr;
+}
+
 int
 read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 {
diff --git a/gdbserver/target.h b/gdbserver/target.h
index f3172e2ed7e..6c536a30778 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -699,7 +699,20 @@ target_thread_pending_child (thread_info *thread)
 
 int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
-int set_desired_thread ();
+/* Set GDBserver's current thread to the thread the client requested
+   via Hg.  Also switches the current process to the requested
+   process.  If the requested thread is not found in the thread list,
+   then the current thread is set to NULL.  Likewise, if the requested
+   process is not found in the process list, then the current process
+   is set to NULL.  Returns true if the requested thread was found,
+   false otherwise.  */
+
+bool set_desired_thread ();
+
+/* Set GDBserver's current process to the process the client requested
+   via Hg.  The current thread is set to NULL.  */
+
+bool set_desired_process ();
 
 std::string target_pid_to_str (ptid_t);
 
-- 
2.26.2

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Pedro Alves]

On 2022-04-19 23:47, Pedro Alves wrote:
> The recent change to make GDBserver always access memory via
> /proc/pid/mem caused a regression in
> gdb.threads/access-mem-running-thread-exit.exp that I somehow missed.
> This is actually a pre-existing GDBserver issue being now exposed.
> 
> Patch #2 fixes the GDBserver bug.
> 
> Patch #1 fixes the gdb.threads/access-mem-running-thread-exit.exp
> testcase itself -- it doesn't run properly against
> --target_board=native-extended-gdbserver today.
> 

I'm pushing this in.

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Luis Machado]

On 5/3/22 15:24, Pedro Alves wrote:
> On 2022-04-19 23:47, Pedro Alves wrote:
>> The recent change to make GDBserver always access memory via
>> /proc/pid/mem caused a regression in
>> gdb.threads/access-mem-running-thread-exit.exp that I somehow missed.
>> This is actually a pre-existing GDBserver issue being now exposed.
>>
>> Patch #2 fixes the GDBserver bug.
>>
>> Patch #1 fixes the gdb.threads/access-mem-running-thread-exit.exp
>> testcase itself -- it doesn't run properly against
>> --target_board=native-extended-gdbserver today.
>>
> 
> I'm pushing this in.

Just a heads-up, this seems to have regressed a few gdb.multi/*.exp 
tests for aarch64-linux.

I see the following internal error for 
gdb.multi/multi-target-continue.exp for example:

Starting program: 
binutils-gdb/gdb/testsuite/outputs/gdb.multi/multi-target-continue/multi-target-continue 
^M
Error in re-setting breakpoint 2: Remote connection closed^M
../../../repos/binutils-gdb/gdb/thread.c:85: internal-error: 
inferior_thread: Assertion `current_thread_ != nullptr' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.

I haven't investigated this yet.

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Luis Machado]

On 5/4/22 10:11, Luis Machado via Gdb-patches wrote:
> On 5/3/22 15:24, Pedro Alves wrote:
>> On 2022-04-19 23:47, Pedro Alves wrote:
>>> The recent change to make GDBserver always access memory via
>>> /proc/pid/mem caused a regression in
>>> gdb.threads/access-mem-running-thread-exit.exp that I somehow missed.
>>> This is actually a pre-existing GDBserver issue being now exposed.
>>>
>>> Patch #2 fixes the GDBserver bug.
>>>
>>> Patch #1 fixes the gdb.threads/access-mem-running-thread-exit.exp
>>> testcase itself -- it doesn't run properly against
>>> --target_board=native-extended-gdbserver today.
>>>
>>
>> I'm pushing this in.
> 
> Just a heads-up, this seems to have regressed a few gdb.multi/*.exp 
> tests for aarch64-linux.
> 
> I see the following internal error for 
> gdb.multi/multi-target-continue.exp for example:
> 
> Starting program: 
> binutils-gdb/gdb/testsuite/outputs/gdb.multi/multi-target-continue/multi-target-continue 
> ^M
> Error in re-setting breakpoint 2: Remote connection closed^M
> ../../../repos/binutils-gdb/gdb/thread.c:85: internal-error: 
> inferior_thread: Assertion `current_thread_ != nullptr' failed.^M
> A problem internal to GDB has been detected,^M
> further debugging may prove unreliable.
> 
> I haven't investigated this yet.

Ok. I tracked this down to gdbserver crashing when trying to fetch the 
register cache using a nullptr for the thread pointer. This happens when 
trying to read memory, then the backend goes to try to figure out if it 
is 64-bit and then eventually tries to fetch the regcache with 
current_thread (nullptr).

I'm guessing we should really be using the process pointer if there 
isn't a valid thread pointer to fetch the register data.

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Pedro Alves]

On 2022-05-04 10:42, Luis Machado wrote:
> On 5/4/22 10:11, Luis Machado via Gdb-patches wrote:
>> On 5/3/22 15:24, Pedro Alves wrote:
>>> On 2022-04-19 23:47, Pedro Alves wrote:
>>>> The recent change to make GDBserver always access memory via
>>>> /proc/pid/mem caused a regression in
>>>> gdb.threads/access-mem-running-thread-exit.exp that I somehow missed.
>>>> This is actually a pre-existing GDBserver issue being now exposed.
>>>>
>>>> Patch #2 fixes the GDBserver bug.
>>>>
>>>> Patch #1 fixes the gdb.threads/access-mem-running-thread-exit.exp
>>>> testcase itself -- it doesn't run properly against
>>>> --target_board=native-extended-gdbserver today.
>>>>
>>>
>>> I'm pushing this in.
>>
>> Just a heads-up, this seems to have regressed a few gdb.multi/*.exp tests for aarch64-linux.
>>
>> I see the following internal error for gdb.multi/multi-target-continue.exp for example:
>>
>> Starting program: binutils-gdb/gdb/testsuite/outputs/gdb.multi/multi-target-continue/multi-target-continue ^M
>> Error in re-setting breakpoint 2: Remote connection closed^M
>> ../../../repos/binutils-gdb/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.^M
>> A problem internal to GDB has been detected,^M
>> further debugging may prove unreliable.
>>
>> I haven't investigated this yet.
> 
> Ok. I tracked this down to gdbserver crashing when trying to fetch the register cache using a nullptr for the thread pointer. This happens when trying to read memory, then the backend goes to try to figure out if it is 64-bit and then eventually tries to fetch the regcache with current_thread (nullptr).
> 
> I'm guessing we should really be using the process pointer if there isn't a valid thread pointer to fetch the register data.
> 

Can you show a backtrace?  If this is when reading memory, what code cares whether it's 64-bit?  Reading memory
out of /proc/pid/mem should not care about that.

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Luis Machado]

On 5/4/22 10:45, Pedro Alves wrote:
> On 2022-05-04 10:42, Luis Machado wrote:
>> On 5/4/22 10:11, Luis Machado via Gdb-patches wrote:
>>> On 5/3/22 15:24, Pedro Alves wrote:
>>>> On 2022-04-19 23:47, Pedro Alves wrote:
>>>>> The recent change to make GDBserver always access memory via
>>>>> /proc/pid/mem caused a regression in
>>>>> gdb.threads/access-mem-running-thread-exit.exp that I somehow missed.
>>>>> This is actually a pre-existing GDBserver issue being now exposed.
>>>>>
>>>>> Patch #2 fixes the GDBserver bug.
>>>>>
>>>>> Patch #1 fixes the gdb.threads/access-mem-running-thread-exit.exp
>>>>> testcase itself -- it doesn't run properly against
>>>>> --target_board=native-extended-gdbserver today.
>>>>>
>>>>
>>>> I'm pushing this in.
>>>
>>> Just a heads-up, this seems to have regressed a few gdb.multi/*.exp tests for aarch64-linux.
>>>
>>> I see the following internal error for gdb.multi/multi-target-continue.exp for example:
>>>
>>> Starting program: binutils-gdb/gdb/testsuite/outputs/gdb.multi/multi-target-continue/multi-target-continue ^M
>>> Error in re-setting breakpoint 2: Remote connection closed^M
>>> ../../../repos/binutils-gdb/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.^M
>>> A problem internal to GDB has been detected,^M
>>> further debugging may prove unreliable.
>>>
>>> I haven't investigated this yet.
>>
>> Ok. I tracked this down to gdbserver crashing when trying to fetch the register cache using a nullptr for the thread pointer. This happens when trying to read memory, then the backend goes to try to figure out if it is 64-bit and then eventually tries to fetch the regcache with current_thread (nullptr).
>>
>> I'm guessing we should really be using the process pointer if there isn't a valid thread pointer to fetch the register data.
>>
> 
> Can you show a backtrace?  If this is when reading memory, what code cares whether it's 64-bit?  Reading memory
> out of /proc/pid/mem should not care about that.

Here it is:

#0  thread_regcache_data (thread=thread@entry=0x0) at 
../../../repos/binutils-gdb/gdbserver/inferiors.cc:120
#1  0x0000aaaaaaabf0e8 in get_thread_regcache (thread=0x0, 
fetch=fetch@entry=0) at ../../../repos/binutils-gdb/gdbserver/regcache.cc:31
#2  0x0000aaaaaaad785c in is_64bit_tdesc () at 
../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:194
#3  0x0000aaaaaaad8a48 in aarch64_target::sw_breakpoint_from_kind 
(this=<optimized out>, kind=4, size=0xffffffffef04) at 
../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:3226
#4  0x0000aaaaaaabe220 in bp_size (bp=0xaaaaaab6f3d0) at 
../../../repos/binutils-gdb/gdbserver/mem-break.cc:226
#5  check_mem_read (mem_addr=187649984471104, 
buf=buf@entry=0xaaaaaab625d0 "\006", mem_len=mem_len@entry=56) at 
../../../repos/binutils-gdb/gdbserver/mem-break.cc:1862
#6  0x0000aaaaaaacc660 in read_inferior_memory (memaddr=<optimized out>, 
myaddr=0xaaaaaab625d0 "\006", len=56) at 
../../../repos/binutils-gdb/gdbserver/target.cc:93
#7  0x0000aaaaaaac3d9c in gdb_read_memory (len=56, myaddr=0xaaaaaab625d0 
"\006", memaddr=187649984471104) at 
../../../repos/binutils-gdb/gdbserver/server.cc:1071
#8  gdb_read_memory (memaddr=187649984471104, myaddr=0xaaaaaab625d0 
"\006", len=56) at ../../../repos/binutils-gdb/gdbserver/server.cc:1048
#9  0x0000aaaaaaac82a4 in process_serial_event () at 
../../../repos/binutils-gdb/gdbserver/server.cc:4307
#10 handle_serial_event (err=<optimized out>, client_data=<optimized 
out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4520
#11 0x0000aaaaaaafbcd0 in gdb_wait_for_event (block=block@entry=1) at 
../../../repos/binutils-gdb/gdbsupport/event-loop.cc:700
#12 0x0000aaaaaaafc0b0 in gdb_wait_for_event (block=1) at 
../../../repos/binutils-gdb/gdbsupport/event-loop.cc:596
#13 gdb_do_one_event () at 
../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
#14 0x0000aaaaaaacacb0 in start_event_loop () at 
../../../repos/binutils-gdb/gdbserver/server.cc:3518
#15 captured_main (argc=4, argv=<optimized out>) at 
../../../repos/binutils-gdb/gdbserver/server.cc:3998
#16 0x0000aaaaaaab66dc in main (argc=<optimized out>, argv=<optimized 
out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4084

--

This sequence of functions is invoked due to a series of conditions:

1 - The probe-based breakpoint mechanism failed (for some reason) so ...
2 - ... gdbserver has to know what type of architecture it is dealing 
with so it can pick the right breakpoint kind, so it wants to check if 
we have a 64-bit target
3 - To determine the size of a register, we need to fetch the register 
cache, and we do so through a thread point, which is now nullptr.

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Pedro Alves]

On 2022-05-04 10:52, Luis Machado wrote:
> On 5/4/22 10:45, Pedro Alves wrote:

>> Can you show a backtrace?  If this is when reading memory, what code cares whether it's 64-bit?  Reading memory
>> out of /proc/pid/mem should not care about that.
> 
> Here it is:
> 
> #0  thread_regcache_data (thread=thread@entry=0x0) at ../../../repos/binutils-gdb/gdbserver/inferiors.cc:120
> #1  0x0000aaaaaaabf0e8 in get_thread_regcache (thread=0x0, fetch=fetch@entry=0) at ../../../repos/binutils-gdb/gdbserver/regcache.cc:31
> #2  0x0000aaaaaaad785c in is_64bit_tdesc () at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:194
> #3  0x0000aaaaaaad8a48 in aarch64_target::sw_breakpoint_from_kind (this=<optimized out>, kind=4, size=0xffffffffef04) at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:3226
> #4  0x0000aaaaaaabe220 in bp_size (bp=0xaaaaaab6f3d0) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:226
> #5  check_mem_read (mem_addr=187649984471104, buf=buf@entry=0xaaaaaab625d0 "\006", mem_len=mem_len@entry=56) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:1862
> #6  0x0000aaaaaaacc660 in read_inferior_memory (memaddr=<optimized out>, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/target.cc:93
> #7  0x0000aaaaaaac3d9c in gdb_read_memory (len=56, myaddr=0xaaaaaab625d0 "\006", memaddr=187649984471104) at ../../../repos/binutils-gdb/gdbserver/server.cc:1071
> #8  gdb_read_memory (memaddr=187649984471104, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/server.cc:1048
> #9  0x0000aaaaaaac82a4 in process_serial_event () at ../../../repos/binutils-gdb/gdbserver/server.cc:4307
> #10 handle_serial_event (err=<optimized out>, client_data=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4520
> #11 0x0000aaaaaaafbcd0 in gdb_wait_for_event (block=block@entry=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:700
> #12 0x0000aaaaaaafc0b0 in gdb_wait_for_event (block=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:596
> #13 gdb_do_one_event () at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
> #14 0x0000aaaaaaacacb0 in start_event_loop () at ../../../repos/binutils-gdb/gdbserver/server.cc:3518
> #15 captured_main (argc=4, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:3998
> #16 0x0000aaaaaaab66dc in main (argc=<optimized out>, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4084
> 
> -- 
> 
> This sequence of functions is invoked due to a series of conditions:
> 
> 1 - The probe-based breakpoint mechanism failed (for some reason) so ...
> 2 - ... gdbserver has to know what type of architecture it is dealing with so it can pick the right breakpoint kind, so it wants to check if we have a 64-bit target
> 3 - To determine the size of a register, we need to fetch the register cache, and we do so through a thread point, which is now nullptr.
> 

Thanks.  I believe the patch below should fix that particular instance.

Note that the thread's tdesc is itself filled from the process's tdesc, so
this should be equivalent:

struct regcache *
get_thread_regcache (struct thread_info *thread, int fetch)
{
  struct regcache *regcache;

  regcache = thread_regcache_data (thread);

...
  if (regcache == NULL)
    {
      struct process_info *proc = get_thread_process (thread);

      gdb_assert (proc->tdesc != NULL);

      regcache = new_register_cache (proc->tdesc);
      set_thread_regcache_data (thread, regcache);
    }
...

There may be other spots that require similar treatments.

From 28f784b14bcd5b435de0a764a6a1e11df5e131c9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 4 May 2022 11:09:07 +0100
Subject: [PATCH] fix

Change-Id: Ibc809d7345e70a2f058b522bdc5cdbdca97e2cdc
---
 gdbserver/linux-aarch64-low.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 0091f998c63..c947f2bcac1 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -191,9 +191,9 @@ struct arch_process_info
 static int
 is_64bit_tdesc (void)
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-
-  return register_size (regcache->tdesc, 0) == 8;
+  /* We may not have a current thread at this point, so go straight to
+     the process's target description.  */
+  return register_size (current_process ()->tdesc) == 8;
 }
 
 static void

base-commit: 7f8acedeebe295fc8cc1d11ed971cbfc1942618c
-- 
2.36.0

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Pedro Alves]

On 2022-05-04 11:14, Pedro Alves wrote:
> On 2022-05-04 10:52, Luis Machado wrote:
>> On 5/4/22 10:45, Pedro Alves wrote:
> 
>>> Can you show a backtrace?  If this is when reading memory, what code cares whether it's 64-bit?  Reading memory
>>> out of /proc/pid/mem should not care about that.
>>
>> Here it is:
>>
>> #0  thread_regcache_data (thread=thread@entry=0x0) at ../../../repos/binutils-gdb/gdbserver/inferiors.cc:120
>> #1  0x0000aaaaaaabf0e8 in get_thread_regcache (thread=0x0, fetch=fetch@entry=0) at ../../../repos/binutils-gdb/gdbserver/regcache.cc:31
>> #2  0x0000aaaaaaad785c in is_64bit_tdesc () at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:194
>> #3  0x0000aaaaaaad8a48 in aarch64_target::sw_breakpoint_from_kind (this=<optimized out>, kind=4, size=0xffffffffef04) at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:3226
>> #4  0x0000aaaaaaabe220 in bp_size (bp=0xaaaaaab6f3d0) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:226
>> #5  check_mem_read (mem_addr=187649984471104, buf=buf@entry=0xaaaaaab625d0 "\006", mem_len=mem_len@entry=56) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:1862
>> #6  0x0000aaaaaaacc660 in read_inferior_memory (memaddr=<optimized out>, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/target.cc:93
>> #7  0x0000aaaaaaac3d9c in gdb_read_memory (len=56, myaddr=0xaaaaaab625d0 "\006", memaddr=187649984471104) at ../../../repos/binutils-gdb/gdbserver/server.cc:1071
>> #8  gdb_read_memory (memaddr=187649984471104, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/server.cc:1048
>> #9  0x0000aaaaaaac82a4 in process_serial_event () at ../../../repos/binutils-gdb/gdbserver/server.cc:4307
>> #10 handle_serial_event (err=<optimized out>, client_data=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4520
>> #11 0x0000aaaaaaafbcd0 in gdb_wait_for_event (block=block@entry=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:700
>> #12 0x0000aaaaaaafc0b0 in gdb_wait_for_event (block=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:596
>> #13 gdb_do_one_event () at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
>> #14 0x0000aaaaaaacacb0 in start_event_loop () at ../../../repos/binutils-gdb/gdbserver/server.cc:3518
>> #15 captured_main (argc=4, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:3998
>> #16 0x0000aaaaaaab66dc in main (argc=<optimized out>, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4084
>>
>> -- 
>>
>> This sequence of functions is invoked due to a series of conditions:
>>
>> 1 - The probe-based breakpoint mechanism failed (for some reason) so ...
>> 2 - ... gdbserver has to know what type of architecture it is dealing with so it can pick the right breakpoint kind, so it wants to check if we have a 64-bit target
>> 3 - To determine the size of a register, we need to fetch the register cache, and we do so through a thread point, which is now nullptr.
>>
> 
> Thanks.  I believe the patch below should fix that particular instance.
> 

Luis confirmed this looks good to him on IRC, so I've merged it, as below.

From 5890af36e5112bcbb8d7555e63570f68466e6944 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 4 May 2022 11:09:07 +0100
Subject: [PATCH] Fix GDBserver Aarch64 Linux regression

Luis noticed that the recent changes to gdbserver to make it track
process and threads independently regressed a few gdb.multi/*.exp
tests for aarch64-linux.

We started seeing the following internal error for
gdb.multi/multi-target-continue.exp for example:

 Starting program: binutils-gdb/gdb/testsuite/outputs/gdb.multi/multi-target-continue/multi-target-continue ^M
 Error in re-setting breakpoint 2: Remote connection closed^M
 ../../../repos/binutils-gdb/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.^M
 A problem internal to GDB has been detected,^M
 further debugging may prove unreliable.

A backtrace looks like:

 #0  thread_regcache_data (thread=thread@entry=0x0) at ../../../repos/binutils-gdb/gdbserver/inferiors.cc:120
 #1  0x0000aaaaaaabf0e8 in get_thread_regcache (thread=0x0, fetch=fetch@entry=0) at ../../../repos/binutils-gdb/gdbserver/regcache.cc:31
 #2  0x0000aaaaaaad785c in is_64bit_tdesc () at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:194
 #3  0x0000aaaaaaad8a48 in aarch64_target::sw_breakpoint_from_kind (this=<optimized out>, kind=4, size=0xffffffffef04) at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:3226
 #4  0x0000aaaaaaabe220 in bp_size (bp=0xaaaaaab6f3d0) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:226
 #5  check_mem_read (mem_addr=187649984471104, buf=buf@entry=0xaaaaaab625d0 "\006", mem_len=mem_len@entry=56) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:1862
 #6  0x0000aaaaaaacc660 in read_inferior_memory (memaddr=<optimized out>, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/target.cc:93
 #7  0x0000aaaaaaac3d9c in gdb_read_memory (len=56, myaddr=0xaaaaaab625d0 "\006", memaddr=187649984471104) at ../../../repos/binutils-gdb/gdbserver/server.cc:1071
 #8  gdb_read_memory (memaddr=187649984471104, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/server.cc:1048
 #9  0x0000aaaaaaac82a4 in process_serial_event () at ../../../repos/binutils-gdb/gdbserver/server.cc:4307
 #10 handle_serial_event (err=<optimized out>, client_data=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4520
 #11 0x0000aaaaaaafbcd0 in gdb_wait_for_event (block=block@entry=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:700
 #12 0x0000aaaaaaafc0b0 in gdb_wait_for_event (block=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:596
 #13 gdb_do_one_event () at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
 #14 0x0000aaaaaaacacb0 in start_event_loop () at ../../../repos/binutils-gdb/gdbserver/server.cc:3518
 #15 captured_main (argc=4, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:3998
 #16 0x0000aaaaaaab66dc in main (argc=<optimized out>, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4084

This sequence of functions is invoked due to a series of conditions:

 1 - The probe-based breakpoint mechanism failed (for some reason) so ...

 2 - ... gdbserver has to know what type of architecture it is dealing
     with so it can pick the right breakpoint kind, so it wants to
     check if we have a 64-bit target.

 3 - To determine the size of a register, we currently fetch the
     current thread's register cache, and the current thread pointer
     is now nullptr.

In #3, the current thread is nullptr because gdb_read_memory clears it
on purpose, via set_desired_process, exactly to expose code relying on
the current thread when it shouldn't.  It was always possible to end
up in this situation (when the current thread exits), but it was
harder to reproduce before.

This commit fixes it by tweaking is_64bit_tdesc to look at the current
process's tdesc instead of the current thread's tdesc.

Note that the thread's tdesc is itself filled from the process's
tdesc, so this should be equivalent:

 struct regcache *
 get_thread_regcache (struct thread_info *thread, int fetch)
 {
   struct regcache *regcache;

   regcache = thread_regcache_data (thread);

 ...
   if (regcache == NULL)
     {
       struct process_info *proc = get_thread_process (thread);

       gdb_assert (proc->tdesc != NULL);

       regcache = new_register_cache (proc->tdesc);
       set_thread_regcache_data (thread, regcache);
     }
 ...

Change-Id: Ibc809d7345e70a2f058b522bdc5cdbdca97e2cdc
---
 gdbserver/linux-aarch64-low.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index c924821c25c..d1e7acb7b4a 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -191,9 +191,9 @@ struct arch_process_info
 static int
 is_64bit_tdesc (void)
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-
-  return register_size (regcache->tdesc, 0) == 8;
+  /* We may not have a current thread at this point, so go straight to
+     the process's target description.  */
+  return register_size (current_process ()->tdesc) == 8;
 }
 
 static void

base-commit: 901e4e8d5c4f1d32c50e4a31b228bc7ef4210775
-- 
2.36.0

Re: [PATCH 0/2] Fix gdbserver/linux memory access regression

[Luis Machado]

On 5/4/22 14:44, Pedro Alves wrote:
> On 2022-05-04 11:14, Pedro Alves wrote:
>> On 2022-05-04 10:52, Luis Machado wrote:
>>> On 5/4/22 10:45, Pedro Alves wrote:
>>
>>>> Can you show a backtrace?  If this is when reading memory, what code cares whether it's 64-bit?  Reading memory
>>>> out of /proc/pid/mem should not care about that.
>>>
>>> Here it is:
>>>
>>> #0  thread_regcache_data (thread=thread@entry=0x0) at ../../../repos/binutils-gdb/gdbserver/inferiors.cc:120
>>> #1  0x0000aaaaaaabf0e8 in get_thread_regcache (thread=0x0, fetch=fetch@entry=0) at ../../../repos/binutils-gdb/gdbserver/regcache.cc:31
>>> #2  0x0000aaaaaaad785c in is_64bit_tdesc () at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:194
>>> #3  0x0000aaaaaaad8a48 in aarch64_target::sw_breakpoint_from_kind (this=<optimized out>, kind=4, size=0xffffffffef04) at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:3226
>>> #4  0x0000aaaaaaabe220 in bp_size (bp=0xaaaaaab6f3d0) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:226
>>> #5  check_mem_read (mem_addr=187649984471104, buf=buf@entry=0xaaaaaab625d0 "\006", mem_len=mem_len@entry=56) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:1862
>>> #6  0x0000aaaaaaacc660 in read_inferior_memory (memaddr=<optimized out>, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/target.cc:93
>>> #7  0x0000aaaaaaac3d9c in gdb_read_memory (len=56, myaddr=0xaaaaaab625d0 "\006", memaddr=187649984471104) at ../../../repos/binutils-gdb/gdbserver/server.cc:1071
>>> #8  gdb_read_memory (memaddr=187649984471104, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/server.cc:1048
>>> #9  0x0000aaaaaaac82a4 in process_serial_event () at ../../../repos/binutils-gdb/gdbserver/server.cc:4307
>>> #10 handle_serial_event (err=<optimized out>, client_data=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4520
>>> #11 0x0000aaaaaaafbcd0 in gdb_wait_for_event (block=block@entry=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:700
>>> #12 0x0000aaaaaaafc0b0 in gdb_wait_for_event (block=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:596
>>> #13 gdb_do_one_event () at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
>>> #14 0x0000aaaaaaacacb0 in start_event_loop () at ../../../repos/binutils-gdb/gdbserver/server.cc:3518
>>> #15 captured_main (argc=4, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:3998
>>> #16 0x0000aaaaaaab66dc in main (argc=<optimized out>, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4084
>>>
>>> -- 
>>>
>>> This sequence of functions is invoked due to a series of conditions:
>>>
>>> 1 - The probe-based breakpoint mechanism failed (for some reason) so ...
>>> 2 - ... gdbserver has to know what type of architecture it is dealing with so it can pick the right breakpoint kind, so it wants to check if we have a 64-bit target
>>> 3 - To determine the size of a register, we need to fetch the register cache, and we do so through a thread point, which is now nullptr.
>>>
>>
>> Thanks.  I believe the patch below should fix that particular instance.
>>
> 
> Luis confirmed this looks good to him on IRC, so I've merged it, as below.
> 
>  From 5890af36e5112bcbb8d7555e63570f68466e6944 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 4 May 2022 11:09:07 +0100
> Subject: [PATCH] Fix GDBserver Aarch64 Linux regression
> 
> Luis noticed that the recent changes to gdbserver to make it track
> process and threads independently regressed a few gdb.multi/*.exp
> tests for aarch64-linux.
> 
> We started seeing the following internal error for
> gdb.multi/multi-target-continue.exp for example:
> 
>   Starting program: binutils-gdb/gdb/testsuite/outputs/gdb.multi/multi-target-continue/multi-target-continue ^M
>   Error in re-setting breakpoint 2: Remote connection closed^M
>   ../../../repos/binutils-gdb/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.^M
>   A problem internal to GDB has been detected,^M
>   further debugging may prove unreliable.
> 
> A backtrace looks like:
> 
>   #0  thread_regcache_data (thread=thread@entry=0x0) at ../../../repos/binutils-gdb/gdbserver/inferiors.cc:120
>   #1  0x0000aaaaaaabf0e8 in get_thread_regcache (thread=0x0, fetch=fetch@entry=0) at ../../../repos/binutils-gdb/gdbserver/regcache.cc:31
>   #2  0x0000aaaaaaad785c in is_64bit_tdesc () at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:194
>   #3  0x0000aaaaaaad8a48 in aarch64_target::sw_breakpoint_from_kind (this=<optimized out>, kind=4, size=0xffffffffef04) at ../../../repos/binutils-gdb/gdbserver/linux-aarch64-low.cc:3226
>   #4  0x0000aaaaaaabe220 in bp_size (bp=0xaaaaaab6f3d0) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:226
>   #5  check_mem_read (mem_addr=187649984471104, buf=buf@entry=0xaaaaaab625d0 "\006", mem_len=mem_len@entry=56) at ../../../repos/binutils-gdb/gdbserver/mem-break.cc:1862
>   #6  0x0000aaaaaaacc660 in read_inferior_memory (memaddr=<optimized out>, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/target.cc:93
>   #7  0x0000aaaaaaac3d9c in gdb_read_memory (len=56, myaddr=0xaaaaaab625d0 "\006", memaddr=187649984471104) at ../../../repos/binutils-gdb/gdbserver/server.cc:1071
>   #8  gdb_read_memory (memaddr=187649984471104, myaddr=0xaaaaaab625d0 "\006", len=56) at ../../../repos/binutils-gdb/gdbserver/server.cc:1048
>   #9  0x0000aaaaaaac82a4 in process_serial_event () at ../../../repos/binutils-gdb/gdbserver/server.cc:4307
>   #10 handle_serial_event (err=<optimized out>, client_data=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4520
>   #11 0x0000aaaaaaafbcd0 in gdb_wait_for_event (block=block@entry=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:700
>   #12 0x0000aaaaaaafc0b0 in gdb_wait_for_event (block=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:596
>   #13 gdb_do_one_event () at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
>   #14 0x0000aaaaaaacacb0 in start_event_loop () at ../../../repos/binutils-gdb/gdbserver/server.cc:3518
>   #15 captured_main (argc=4, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:3998
>   #16 0x0000aaaaaaab66dc in main (argc=<optimized out>, argv=<optimized out>) at ../../../repos/binutils-gdb/gdbserver/server.cc:4084
> 
> This sequence of functions is invoked due to a series of conditions:
> 
>   1 - The probe-based breakpoint mechanism failed (for some reason) so ...
> 
>   2 - ... gdbserver has to know what type of architecture it is dealing
>       with so it can pick the right breakpoint kind, so it wants to
>       check if we have a 64-bit target.
> 
>   3 - To determine the size of a register, we currently fetch the
>       current thread's register cache, and the current thread pointer
>       is now nullptr.
> 
> In #3, the current thread is nullptr because gdb_read_memory clears it
> on purpose, via set_desired_process, exactly to expose code relying on
> the current thread when it shouldn't.  It was always possible to end
> up in this situation (when the current thread exits), but it was
> harder to reproduce before.
> 
> This commit fixes it by tweaking is_64bit_tdesc to look at the current
> process's tdesc instead of the current thread's tdesc.
> 
> Note that the thread's tdesc is itself filled from the process's
> tdesc, so this should be equivalent:
> 
>   struct regcache *
>   get_thread_regcache (struct thread_info *thread, int fetch)
>   {
>     struct regcache *regcache;
> 
>     regcache = thread_regcache_data (thread);
> 
>   ...
>     if (regcache == NULL)
>       {
>         struct process_info *proc = get_thread_process (thread);
> 
>         gdb_assert (proc->tdesc != NULL);
> 
>         regcache = new_register_cache (proc->tdesc);
>         set_thread_regcache_data (thread, regcache);
>       }
>   ...
> 
> Change-Id: Ibc809d7345e70a2f058b522bdc5cdbdca97e2cdc
> ---
>   gdbserver/linux-aarch64-low.cc | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index c924821c25c..d1e7acb7b4a 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -191,9 +191,9 @@ struct arch_process_info
>   static int
>   is_64bit_tdesc (void)
>   {
> -  struct regcache *regcache = get_thread_regcache (current_thread, 0);
> -
> -  return register_size (regcache->tdesc, 0) == 8;
> +  /* We may not have a current thread at this point, so go straight to
> +     the process's target description.  */
> +  return register_size (current_process ()->tdesc) == 8;

Sorry, forgot to tell you I fixed a typo in the patch above before 
testing. It should be:

return register_size (current_process ()->tdesc, 0) == 8;

Re: [PATCH 2/2] gdbserver: track current process as well as current thread

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> The recent commit 421490af33bf ("gdbserver/linux: Access memory even
> if threads are running") caused a regression in
> gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I
> somehow missed.  Like so:
>
>  (gdb) print global_var
>  Cannot access memory at address 0x555555558010
>  (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem (print global_var after writing, inf=2, iter=1)
>
> The problem starts with GDB telling GDBserver to select a thread, via
> the Hg packet, which GDBserver complies with, then that thread exits,
> and GDB, without knowing the thread is gone, tries to write to memory,
> through the context of the previously selected Hg thread.
>
> GDBserver's GDB-facing memory access routines, gdb_read_memory and
> gdb_write_memory, call set_desired_thread to make GDBserver re-select
> the thread that GDB has selected with the Hg packet.  Since the thread
> is gone, set_desired_thread returns false, and the memory access
> fails.
>
> Now, to access memory, it doesn't really matter which thread is
> selected.  All we should need is the target process.  Even if the
> thread that GDB previously selected is gone, and GDB does not yet know
> about that exit, it shouldn't matter, GDBserver should still know
> which process that thread belonged to.
>
> Fix this by making GDBserver track the current process separately,
> like GDB also does.  Add a new set_desired_process routine that is
> similar to set_desired_thread, but just sets the current process,
> leaving the current thread as NULL.  Use it in the GDB-facing memory
> read and write routines, to avoid failing if the selected thread is
> gone, but the process is still around.
>
> Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd
> ---
>  gdbserver/gdbthread.h  |  1 +
>  gdbserver/inferiors.cc | 26 +++++++++++++++++++------
>  gdbserver/server.cc    |  4 ++--
>  gdbserver/target.cc    | 44 ++++++++++++++++++++++++++++++++++++++++--
>  gdbserver/target.h     | 15 +++++++++++++-
>  5 files changed, 79 insertions(+), 11 deletions(-)
>
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 10ae1cb7c87..8b897e73d33 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -252,6 +252,7 @@ class scoped_restore_current_thread
>  
>  private:
>    bool m_dont_restore = false;
> +  process_info *m_process;
>    thread_info *m_thread;
>  };
>  
> diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
> index 678d93c77a1..3d0a8b0e716 100644
> --- a/gdbserver/inferiors.cc
> +++ b/gdbserver/inferiors.cc
> @@ -26,6 +26,11 @@
>  std::list<process_info *> all_processes;
>  std::list<thread_info *> all_threads;
>  
> +/* The current process.  */
> +static process_info *current_process_;
> +
> +/* The current thread.  This is either a thread of CURRENT_PROCESS, or
> +   NULL.  */
>  struct thread_info *current_thread;
>  
>  /* The current working directory used to start the inferior.
> @@ -130,6 +135,7 @@ clear_inferiors (void)
>    clear_dlls ();
>  
>    switch_to_thread (nullptr);
> +  current_process_ = nullptr;
>  }
>  
>  struct process_info *
> @@ -153,6 +159,8 @@ remove_process (struct process_info *process)
>    free_all_breakpoints (process);
>    gdb_assert (find_thread_process (process) == NULL);
>    all_processes.remove (process);
> +  if (current_process () == process)
> +    switch_to_process (nullptr);
>    delete process;
>  }
>  
> @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread)
>  struct process_info *
>  current_process (void)
>  {
> -  gdb_assert (current_thread != NULL);
> -  return get_thread_process (current_thread);
> +  return current_process_;

A bit late I know, but I wonder if the assert, or something similar,
should have been retained here?

The comment on this function currently (though Baris has a patch
proposed to change this), says this function should only be called in a
context where the result will not be nullptr.  Given that, not of the
(many) existing uses check the return value of this function against
nullptr.

Happy to roll a patch to add the assert back, just wondered if there was
a reason for its removal.

Thanks,
Andrew



>  }
>  
>  /* See gdbsupport/common-gdbthread.h.  */
> @@ -223,6 +230,10 @@ switch_to_thread (process_stratum_target *ops, ptid_t ptid)
>  void
>  switch_to_thread (thread_info *thread)
>  {
> +  if (thread != nullptr)
> +    current_process_ = get_thread_process (thread);
> +  else
> +    current_process_ = nullptr;
>    current_thread = thread;
>  }
>  
> @@ -231,9 +242,8 @@ switch_to_thread (thread_info *thread)
>  void
>  switch_to_process (process_info *proc)
>  {
> -  int pid = pid_of (proc);
> -
> -  switch_to_thread (find_any_thread_of_pid (pid));
> +  current_process_ = proc;
> +  current_thread = nullptr;
>  }
>  
>  /* See gdbsupport/common-inferior.h.  */
> @@ -254,6 +264,7 @@ set_inferior_cwd (std::string cwd)
>  
>  scoped_restore_current_thread::scoped_restore_current_thread ()
>  {
> +  m_process = current_process_;
>    m_thread = current_thread;
>  }
>  
> @@ -262,5 +273,8 @@ scoped_restore_current_thread::~scoped_restore_current_thread ()
>    if (m_dont_restore)
>      return;
>  
> -  switch_to_thread (m_thread);
> +  if (m_thread != nullptr)
> +    switch_to_thread (m_thread);
> +  else
> +    switch_to_process (m_process);
>  }
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 33c42714e72..f9c02a9c6da 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1067,7 +1067,7 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>        /* (assume no half-trace half-real blocks for now) */
>      }
>  
> -  if (set_desired_thread ())
> +  if (set_desired_process ())
>      res = read_inferior_memory (memaddr, myaddr, len);
>    else
>      res = 1;
> @@ -1088,7 +1088,7 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
>      {
>        int ret;
>  
> -      if (set_desired_thread ())
> +      if (set_desired_process ())
>  	ret = target_write_memory (memaddr, myaddr, len);
>        else
>  	ret = EIO;
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index 883242377c0..adcfe6e7bcc 100644
> --- a/gdbserver/target.cc
> +++ b/gdbserver/target.cc
> @@ -29,16 +29,56 @@
>  
>  process_stratum_target *the_target;
>  
> -int
> +/* See target.h.  */
> +
> +bool
>  set_desired_thread ()
>  {
>    client_state &cs = get_client_state ();
>    thread_info *found = find_thread_ptid (cs.general_thread);
>  
> -  switch_to_thread (found);
> +  if (found == nullptr)
> +    {
> +      process_info *proc = find_process_pid (cs.general_thread.pid ());
> +      if (proc == nullptr)
> +	{
> +	  threads_debug_printf
> +	    ("did not find thread nor process for general_thread %s",
> +	     cs.general_thread.to_string ().c_str ());
> +	}
> +      else
> +	{
> +	  threads_debug_printf
> +	    ("did not find thread for general_thread %s, but found process",
> +	     cs.general_thread.to_string ().c_str ());
> +	}
> +      switch_to_process (proc);
> +    }
> +  else
> +    switch_to_thread (found);
> +
>    return (current_thread != NULL);
>  }
>  
> +/* See target.h.  */
> +
> +bool
> +set_desired_process ()
> +{
> +  client_state &cs = get_client_state ();
> +
> +  process_info *proc = find_process_pid (cs.general_thread.pid ());
> +  if (proc == nullptr)
> +    {
> +      threads_debug_printf
> +	("did not find process for general_thread %s",
> +	 cs.general_thread.to_string ().c_str ());
> +    }
> +  switch_to_process (proc);
> +
> +  return proc != nullptr;
> +}
> +
>  int
>  read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>  {
> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index f3172e2ed7e..6c536a30778 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -699,7 +699,20 @@ target_thread_pending_child (thread_info *thread)
>  
>  int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
>  
> -int set_desired_thread ();
> +/* Set GDBserver's current thread to the thread the client requested
> +   via Hg.  Also switches the current process to the requested
> +   process.  If the requested thread is not found in the thread list,
> +   then the current thread is set to NULL.  Likewise, if the requested
> +   process is not found in the process list, then the current process
> +   is set to NULL.  Returns true if the requested thread was found,
> +   false otherwise.  */
> +
> +bool set_desired_thread ();
> +
> +/* Set GDBserver's current process to the process the client requested
> +   via Hg.  The current thread is set to NULL.  */
> +
> +bool set_desired_process ();
>  
>  std::string target_pid_to_str (ptid_t);
>  
> -- 
> 2.26.2

RE: [PATCH 2/2] gdbserver: track current process as well as current thread

[Aktemur, Tankut Baris]

On Tuesday, April 25, 2023 3:57 PM, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
> > The recent commit 421490af33bf ("gdbserver/linux: Access memory even
> > if threads are running") caused a regression in
> > gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I
> > somehow missed.  Like so:
> >
> >  (gdb) print global_var
> >  Cannot access memory at address 0x555555558010
> >  (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem (print
> global_var after writing, inf=2, iter=1)
> >
> > The problem starts with GDB telling GDBserver to select a thread, via
> > the Hg packet, which GDBserver complies with, then that thread exits,
> > and GDB, without knowing the thread is gone, tries to write to memory,
> > through the context of the previously selected Hg thread.
> >
> > GDBserver's GDB-facing memory access routines, gdb_read_memory and
> > gdb_write_memory, call set_desired_thread to make GDBserver re-select
> > the thread that GDB has selected with the Hg packet.  Since the thread
> > is gone, set_desired_thread returns false, and the memory access
> > fails.
> >
> > Now, to access memory, it doesn't really matter which thread is
> > selected.  All we should need is the target process.  Even if the
> > thread that GDB previously selected is gone, and GDB does not yet know
> > about that exit, it shouldn't matter, GDBserver should still know
> > which process that thread belonged to.
> >
> > Fix this by making GDBserver track the current process separately,
> > like GDB also does.  Add a new set_desired_process routine that is
> > similar to set_desired_thread, but just sets the current process,
> > leaving the current thread as NULL.  Use it in the GDB-facing memory
> > read and write routines, to avoid failing if the selected thread is
> > gone, but the process is still around.
> >
> > Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd
> > ---
> >  gdbserver/gdbthread.h  |  1 +
> >  gdbserver/inferiors.cc | 26 +++++++++++++++++++------
> >  gdbserver/server.cc    |  4 ++--
> >  gdbserver/target.cc    | 44 ++++++++++++++++++++++++++++++++++++++++--
> >  gdbserver/target.h     | 15 +++++++++++++-
> >  5 files changed, 79 insertions(+), 11 deletions(-)
> >
> > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> > index 10ae1cb7c87..8b897e73d33 100644
> > --- a/gdbserver/gdbthread.h
> > +++ b/gdbserver/gdbthread.h
> > @@ -252,6 +252,7 @@ class scoped_restore_current_thread
> >
> >  private:
> >    bool m_dont_restore = false;
> > +  process_info *m_process;
> >    thread_info *m_thread;
> >  };
> >
> > diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
> > index 678d93c77a1..3d0a8b0e716 100644
> > --- a/gdbserver/inferiors.cc
> > +++ b/gdbserver/inferiors.cc
> > @@ -26,6 +26,11 @@
> >  std::list<process_info *> all_processes;
> >  std::list<thread_info *> all_threads;
> >
> > +/* The current process.  */
> > +static process_info *current_process_;
> > +
> > +/* The current thread.  This is either a thread of CURRENT_PROCESS, or
> > +   NULL.  */
> >  struct thread_info *current_thread;
> >
> >  /* The current working directory used to start the inferior.
> > @@ -130,6 +135,7 @@ clear_inferiors (void)
> >    clear_dlls ();
> >
> >    switch_to_thread (nullptr);
> > +  current_process_ = nullptr;
> >  }
> >
> >  struct process_info *
> > @@ -153,6 +159,8 @@ remove_process (struct process_info *process)
> >    free_all_breakpoints (process);
> >    gdb_assert (find_thread_process (process) == NULL);
> >    all_processes.remove (process);
> > +  if (current_process () == process)
> > +    switch_to_process (nullptr);
> >    delete process;
> >  }
> >
> > @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread)
> >  struct process_info *
> >  current_process (void)
> >  {
> > -  gdb_assert (current_thread != NULL);
> > -  return get_thread_process (current_thread);
> > +  return current_process_;
> 
> A bit late I know, but I wonder if the assert, or something similar,
> should have been retained here?
> 
> The comment on this function currently (though Baris has a patch
> proposed to change this), says this function should only be called in a
> context where the result will not be nullptr.  Given that, not of the
> (many) existing uses check the return value of this function against
> nullptr.
> 
> Happy to roll a patch to add the assert back, just wondered if there was
> a reason for its removal.
> 
> Thanks,
> Andrew

Hi Andrew,

I think the current process can in fact be null in some brief periods.
For instance, in 'handle_detach' there is

      if (extended_protocol || target_running ())
        {
          /* There is still at least one inferior remaining or
             we are in extended mode, so don't terminate gdbserver,
             and instead treat this like a normal program exit.  */
          cs.last_status.set_exited (0);
          cs.last_ptid = ptid_t (pid);

          switch_to_thread (nullptr);
        }

So, if the current process exits, gdbserver reports the event to GDB and
sets the current thread to nullptr, which also sets the current process
to nullptr.

I believe an invariant is this:

  (current_thread == nullptr) || (current_process () == get_thread_process (current_thread))

Thanks
-Baris


> >  }
> >
> >  /* See gdbsupport/common-gdbthread.h.  */
> > @@ -223,6 +230,10 @@ switch_to_thread (process_stratum_target *ops, ptid_t ptid)
> >  void
> >  switch_to_thread (thread_info *thread)
> >  {
> > +  if (thread != nullptr)
> > +    current_process_ = get_thread_process (thread);
> > +  else
> > +    current_process_ = nullptr;
> >    current_thread = thread;
> >  }
> >
> > @@ -231,9 +242,8 @@ switch_to_thread (thread_info *thread)
> >  void
> >  switch_to_process (process_info *proc)
> >  {
> > -  int pid = pid_of (proc);
> > -
> > -  switch_to_thread (find_any_thread_of_pid (pid));
> > +  current_process_ = proc;
> > +  current_thread = nullptr;
> >  }
> >
> >  /* See gdbsupport/common-inferior.h.  */
> > @@ -254,6 +264,7 @@ set_inferior_cwd (std::string cwd)
> >
> >  scoped_restore_current_thread::scoped_restore_current_thread ()
> >  {
> > +  m_process = current_process_;
> >    m_thread = current_thread;
> >  }
> >
> > @@ -262,5 +273,8 @@ scoped_restore_current_thread::~scoped_restore_current_thread ()
> >    if (m_dont_restore)
> >      return;
> >
> > -  switch_to_thread (m_thread);
> > +  if (m_thread != nullptr)
> > +    switch_to_thread (m_thread);
> > +  else
> > +    switch_to_process (m_process);
> >  }
> > diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> > index 33c42714e72..f9c02a9c6da 100644
> > --- a/gdbserver/server.cc
> > +++ b/gdbserver/server.cc
> > @@ -1067,7 +1067,7 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
> >        /* (assume no half-trace half-real blocks for now) */
> >      }
> >
> > -  if (set_desired_thread ())
> > +  if (set_desired_process ())
> >      res = read_inferior_memory (memaddr, myaddr, len);
> >    else
> >      res = 1;
> > @@ -1088,7 +1088,7 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
> int len)
> >      {
> >        int ret;
> >
> > -      if (set_desired_thread ())
> > +      if (set_desired_process ())
> >  	ret = target_write_memory (memaddr, myaddr, len);
> >        else
> >  	ret = EIO;
> > diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> > index 883242377c0..adcfe6e7bcc 100644
> > --- a/gdbserver/target.cc
> > +++ b/gdbserver/target.cc
> > @@ -29,16 +29,56 @@
> >
> >  process_stratum_target *the_target;
> >
> > -int
> > +/* See target.h.  */
> > +
> > +bool
> >  set_desired_thread ()
> >  {
> >    client_state &cs = get_client_state ();
> >    thread_info *found = find_thread_ptid (cs.general_thread);
> >
> > -  switch_to_thread (found);
> > +  if (found == nullptr)
> > +    {
> > +      process_info *proc = find_process_pid (cs.general_thread.pid ());
> > +      if (proc == nullptr)
> > +	{
> > +	  threads_debug_printf
> > +	    ("did not find thread nor process for general_thread %s",
> > +	     cs.general_thread.to_string ().c_str ());
> > +	}
> > +      else
> > +	{
> > +	  threads_debug_printf
> > +	    ("did not find thread for general_thread %s, but found process",
> > +	     cs.general_thread.to_string ().c_str ());
> > +	}
> > +      switch_to_process (proc);
> > +    }
> > +  else
> > +    switch_to_thread (found);
> > +
> >    return (current_thread != NULL);
> >  }
> >
> > +/* See target.h.  */
> > +
> > +bool
> > +set_desired_process ()
> > +{
> > +  client_state &cs = get_client_state ();
> > +
> > +  process_info *proc = find_process_pid (cs.general_thread.pid ());
> > +  if (proc == nullptr)
> > +    {
> > +      threads_debug_printf
> > +	("did not find process for general_thread %s",
> > +	 cs.general_thread.to_string ().c_str ());
> > +    }
> > +  switch_to_process (proc);
> > +
> > +  return proc != nullptr;
> > +}
> > +
> >  int
> >  read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
> >  {
> > diff --git a/gdbserver/target.h b/gdbserver/target.h
> > index f3172e2ed7e..6c536a30778 100644
> > --- a/gdbserver/target.h
> > +++ b/gdbserver/target.h
> > @@ -699,7 +699,20 @@ target_thread_pending_child (thread_info *thread)
> >
> >  int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
> >
> > -int set_desired_thread ();
> > +/* Set GDBserver's current thread to the thread the client requested
> > +   via Hg.  Also switches the current process to the requested
> > +   process.  If the requested thread is not found in the thread list,
> > +   then the current thread is set to NULL.  Likewise, if the requested
> > +   process is not found in the process list, then the current process
> > +   is set to NULL.  Returns true if the requested thread was found,
> > +   false otherwise.  */
> > +
> > +bool set_desired_thread ();
> > +
> > +/* Set GDBserver's current process to the process the client requested
> > +   via Hg.  The current thread is set to NULL.  */
> > +
> > +bool set_desired_process ();
> >
> >  std::string target_pid_to_str (ptid_t);
> >
> > --
> > 2.26.2

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 2/2] gdbserver: track current process as well as current thread

[Aktemur, Tankut Baris]

> > > The recent commit 421490af33bf ("gdbserver/linux: Access memory even
> > > if threads are running") caused a regression in
> > > gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I
> > > somehow missed.  Like so:
> > >
> > >  (gdb) print global_var
> > >  Cannot access memory at address 0x555555558010
> > >  (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem (print
> > global_var after writing, inf=2, iter=1)
> > >
> > > The problem starts with GDB telling GDBserver to select a thread, via
> > > the Hg packet, which GDBserver complies with, then that thread exits,
> > > and GDB, without knowing the thread is gone, tries to write to memory,
> > > through the context of the previously selected Hg thread.
> > >
> > > GDBserver's GDB-facing memory access routines, gdb_read_memory and
> > > gdb_write_memory, call set_desired_thread to make GDBserver re-select
> > > the thread that GDB has selected with the Hg packet.  Since the thread
> > > is gone, set_desired_thread returns false, and the memory access
> > > fails.
> > >
> > > Now, to access memory, it doesn't really matter which thread is
> > > selected.  All we should need is the target process.  Even if the
> > > thread that GDB previously selected is gone, and GDB does not yet know
> > > about that exit, it shouldn't matter, GDBserver should still know
> > > which process that thread belonged to.
> > >
> > > Fix this by making GDBserver track the current process separately,
> > > like GDB also does.  Add a new set_desired_process routine that is
> > > similar to set_desired_thread, but just sets the current process,
> > > leaving the current thread as NULL.  Use it in the GDB-facing memory
> > > read and write routines, to avoid failing if the selected thread is
> > > gone, but the process is still around.
> > >
> > > Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd
> > > ---
> > >  gdbserver/gdbthread.h  |  1 +
> > >  gdbserver/inferiors.cc | 26 +++++++++++++++++++------
> > >  gdbserver/server.cc    |  4 ++--
> > >  gdbserver/target.cc    | 44 ++++++++++++++++++++++++++++++++++++++++--
> > >  gdbserver/target.h     | 15 +++++++++++++-
> > >  5 files changed, 79 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> > > index 10ae1cb7c87..8b897e73d33 100644
> > > --- a/gdbserver/gdbthread.h
> > > +++ b/gdbserver/gdbthread.h
> > > @@ -252,6 +252,7 @@ class scoped_restore_current_thread
> > >
> > >  private:
> > >    bool m_dont_restore = false;
> > > +  process_info *m_process;
> > >    thread_info *m_thread;
> > >  };
> > >
> > > diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
> > > index 678d93c77a1..3d0a8b0e716 100644
> > > --- a/gdbserver/inferiors.cc
> > > +++ b/gdbserver/inferiors.cc
> > > @@ -26,6 +26,11 @@
> > >  std::list<process_info *> all_processes;
> > >  std::list<thread_info *> all_threads;
> > >
> > > +/* The current process.  */
> > > +static process_info *current_process_;
> > > +
> > > +/* The current thread.  This is either a thread of CURRENT_PROCESS, or
> > > +   NULL.  */
> > >  struct thread_info *current_thread;
> > >
> > >  /* The current working directory used to start the inferior.
> > > @@ -130,6 +135,7 @@ clear_inferiors (void)
> > >    clear_dlls ();
> > >
> > >    switch_to_thread (nullptr);
> > > +  current_process_ = nullptr;
> > >  }
> > >
> > >  struct process_info *
> > > @@ -153,6 +159,8 @@ remove_process (struct process_info *process)
> > >    free_all_breakpoints (process);
> > >    gdb_assert (find_thread_process (process) == NULL);
> > >    all_processes.remove (process);
> > > +  if (current_process () == process)
> > > +    switch_to_process (nullptr);
> > >    delete process;
> > >  }
> > >
> > > @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread)
> > >  struct process_info *
> > >  current_process (void)
> > >  {
> > > -  gdb_assert (current_thread != NULL);
> > > -  return get_thread_process (current_thread);
> > > +  return current_process_;
> >
> > A bit late I know, but I wonder if the assert, or something similar,
> > should have been retained here?
> >
> > The comment on this function currently (though Baris has a patch
> > proposed to change this), says this function should only be called in a
> > context where the result will not be nullptr.  Given that, not of the
> > (many) existing uses check the return value of this function against
> > nullptr.
> >
> > Happy to roll a patch to add the assert back, just wondered if there was
> > a reason for its removal.
> >
> > Thanks,
> > Andrew
> 
> Hi Andrew,
> 
> I think the current process can in fact be null in some brief periods.
> For instance, in 'handle_detach' there is
> 
>       if (extended_protocol || target_running ())
>         {
>           /* There is still at least one inferior remaining or
>              we are in extended mode, so don't terminate gdbserver,
>              and instead treat this like a normal program exit.  */
>           cs.last_status.set_exited (0);
>           cs.last_ptid = ptid_t (pid);
> 
>           switch_to_thread (nullptr);
>         }
> 
> So, if the current process exits, gdbserver reports the event to GDB and
> sets the current thread to nullptr, which also sets the current process
> to nullptr.
> 
> I believe an invariant is this:
> 
>   (current_thread == nullptr) || (current_process () == get_thread_process (current_thread))

Kindly pinging for/in the context of
https://sourceware.org/pipermail/gdb-patches/2023-April/199116.html

-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 2/2] gdbserver: track current process as well as current thread

[Andrew Burgess]

"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> On Tuesday, April 25, 2023 3:57 PM, Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>> 
>> > The recent commit 421490af33bf ("gdbserver/linux: Access memory even
>> > if threads are running") caused a regression in
>> > gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I
>> > somehow missed.  Like so:
>> >
>> >  (gdb) print global_var
>> >  Cannot access memory at address 0x555555558010
>> >  (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem (print
>> global_var after writing, inf=2, iter=1)
>> >
>> > The problem starts with GDB telling GDBserver to select a thread, via
>> > the Hg packet, which GDBserver complies with, then that thread exits,
>> > and GDB, without knowing the thread is gone, tries to write to memory,
>> > through the context of the previously selected Hg thread.
>> >
>> > GDBserver's GDB-facing memory access routines, gdb_read_memory and
>> > gdb_write_memory, call set_desired_thread to make GDBserver re-select
>> > the thread that GDB has selected with the Hg packet.  Since the thread
>> > is gone, set_desired_thread returns false, and the memory access
>> > fails.
>> >
>> > Now, to access memory, it doesn't really matter which thread is
>> > selected.  All we should need is the target process.  Even if the
>> > thread that GDB previously selected is gone, and GDB does not yet know
>> > about that exit, it shouldn't matter, GDBserver should still know
>> > which process that thread belonged to.
>> >
>> > Fix this by making GDBserver track the current process separately,
>> > like GDB also does.  Add a new set_desired_process routine that is
>> > similar to set_desired_thread, but just sets the current process,
>> > leaving the current thread as NULL.  Use it in the GDB-facing memory
>> > read and write routines, to avoid failing if the selected thread is
>> > gone, but the process is still around.
>> >
>> > Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd
>> > ---
>> >  gdbserver/gdbthread.h  |  1 +
>> >  gdbserver/inferiors.cc | 26 +++++++++++++++++++------
>> >  gdbserver/server.cc    |  4 ++--
>> >  gdbserver/target.cc    | 44 ++++++++++++++++++++++++++++++++++++++++--
>> >  gdbserver/target.h     | 15 +++++++++++++-
>> >  5 files changed, 79 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
>> > index 10ae1cb7c87..8b897e73d33 100644
>> > --- a/gdbserver/gdbthread.h
>> > +++ b/gdbserver/gdbthread.h
>> > @@ -252,6 +252,7 @@ class scoped_restore_current_thread
>> >
>> >  private:
>> >    bool m_dont_restore = false;
>> > +  process_info *m_process;
>> >    thread_info *m_thread;
>> >  };
>> >
>> > diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
>> > index 678d93c77a1..3d0a8b0e716 100644
>> > --- a/gdbserver/inferiors.cc
>> > +++ b/gdbserver/inferiors.cc
>> > @@ -26,6 +26,11 @@
>> >  std::list<process_info *> all_processes;
>> >  std::list<thread_info *> all_threads;
>> >
>> > +/* The current process.  */
>> > +static process_info *current_process_;
>> > +
>> > +/* The current thread.  This is either a thread of CURRENT_PROCESS, or
>> > +   NULL.  */
>> >  struct thread_info *current_thread;
>> >
>> >  /* The current working directory used to start the inferior.
>> > @@ -130,6 +135,7 @@ clear_inferiors (void)
>> >    clear_dlls ();
>> >
>> >    switch_to_thread (nullptr);
>> > +  current_process_ = nullptr;
>> >  }
>> >
>> >  struct process_info *
>> > @@ -153,6 +159,8 @@ remove_process (struct process_info *process)
>> >    free_all_breakpoints (process);
>> >    gdb_assert (find_thread_process (process) == NULL);
>> >    all_processes.remove (process);
>> > +  if (current_process () == process)
>> > +    switch_to_process (nullptr);
>> >    delete process;
>> >  }
>> >
>> > @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread)
>> >  struct process_info *
>> >  current_process (void)
>> >  {
>> > -  gdb_assert (current_thread != NULL);
>> > -  return get_thread_process (current_thread);
>> > +  return current_process_;
>> 
>> A bit late I know, but I wonder if the assert, or something similar,
>> should have been retained here?
>> 
>> The comment on this function currently (though Baris has a patch
>> proposed to change this), says this function should only be called in a
>> context where the result will not be nullptr.  Given that, not of the
>> (many) existing uses check the return value of this function against
>> nullptr.
>> 
>> Happy to roll a patch to add the assert back, just wondered if there was
>> a reason for its removal.
>> 
>> Thanks,
>> Andrew
>
> Hi Andrew,
>
> I think the current process can in fact be null in some brief periods.
> For instance, in 'handle_detach' there is
>
>       if (extended_protocol || target_running ())
>         {
>           /* There is still at least one inferior remaining or
>              we are in extended mode, so don't terminate gdbserver,
>              and instead treat this like a normal program exit.  */
>           cs.last_status.set_exited (0);
>           cs.last_ptid = ptid_t (pid);
>
>           switch_to_thread (nullptr);
>         }
>
> So, if the current process exits, gdbserver reports the event to GDB and
> sets the current thread to nullptr, which also sets the current process
> to nullptr.
>
> I believe an invariant is this:
>
>   (current_thread == nullptr) || (current_process () == get_thread_process (current_thread))
>

I don't think this really addresses my question.  Here's how I
understand things:

  1. Before this patch:
     a. Comment on 'current_process' says: it's an error to call this
        function when no thread is selected, and
     b. The function asserted that a process was selected,

  2. This patch removed the assert, but left the comment unchanged,

  3. A patch was proposed to updated the comment,

  4. I couldn't see any reason in this patch _why_ the assert was
     removed.

I agree that the process _could_ be nullptr, but it _could_ have been
nullptr before.

My question is: did something change such that there is now a location
where we might choose to call current_process when no thread is
selected?

Given the description of the original patch my guess was no, but maybe I
should just add the assert back locally and do a test run to find out?

Thanks,
Andrew

RE: [PATCH 2/2] gdbserver: track current process as well as current thread

[Aktemur, Tankut Baris]

On Thursday, June 22, 2023 7:50 PM, Andrew Burgess wrote:
> "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:
> 
> > On Tuesday, April 25, 2023 3:57 PM, Andrew Burgess wrote:
> >> Pedro Alves <pedro@palves.net> writes:
> >>
> >> > The recent commit 421490af33bf ("gdbserver/linux: Access memory even
> >> > if threads are running") caused a regression in
> >> > gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which I
> >> > somehow missed.  Like so:
> >> >
> >> >  (gdb) print global_var
> >> >  Cannot access memory at address 0x555555558010
> >> >  (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-stop: access mem
> (print
> >> global_var after writing, inf=2, iter=1)
> >> >
> >> > The problem starts with GDB telling GDBserver to select a thread, via
> >> > the Hg packet, which GDBserver complies with, then that thread exits,
> >> > and GDB, without knowing the thread is gone, tries to write to memory,
> >> > through the context of the previously selected Hg thread.
> >> >
> >> > GDBserver's GDB-facing memory access routines, gdb_read_memory and
> >> > gdb_write_memory, call set_desired_thread to make GDBserver re-select
> >> > the thread that GDB has selected with the Hg packet.  Since the thread
> >> > is gone, set_desired_thread returns false, and the memory access
> >> > fails.
> >> >
> >> > Now, to access memory, it doesn't really matter which thread is
> >> > selected.  All we should need is the target process.  Even if the
> >> > thread that GDB previously selected is gone, and GDB does not yet know
> >> > about that exit, it shouldn't matter, GDBserver should still know
> >> > which process that thread belonged to.
> >> >
> >> > Fix this by making GDBserver track the current process separately,
> >> > like GDB also does.  Add a new set_desired_process routine that is
> >> > similar to set_desired_thread, but just sets the current process,
> >> > leaving the current thread as NULL.  Use it in the GDB-facing memory
> >> > read and write routines, to avoid failing if the selected thread is
> >> > gone, but the process is still around.
> >> >
> >> > Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd
> >> > ---
> >> >  gdbserver/gdbthread.h  |  1 +
> >> >  gdbserver/inferiors.cc | 26 +++++++++++++++++++------
> >> >  gdbserver/server.cc    |  4 ++--
> >> >  gdbserver/target.cc    | 44 ++++++++++++++++++++++++++++++++++++++++--
> >> >  gdbserver/target.h     | 15 +++++++++++++-
> >> >  5 files changed, 79 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> >> > index 10ae1cb7c87..8b897e73d33 100644
> >> > --- a/gdbserver/gdbthread.h
> >> > +++ b/gdbserver/gdbthread.h
> >> > @@ -252,6 +252,7 @@ class scoped_restore_current_thread
> >> >
> >> >  private:
> >> >    bool m_dont_restore = false;
> >> > +  process_info *m_process;
> >> >    thread_info *m_thread;
> >> >  };
> >> >
> >> > diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
> >> > index 678d93c77a1..3d0a8b0e716 100644
> >> > --- a/gdbserver/inferiors.cc
> >> > +++ b/gdbserver/inferiors.cc
> >> > @@ -26,6 +26,11 @@
> >> >  std::list<process_info *> all_processes;
> >> >  std::list<thread_info *> all_threads;
> >> >
> >> > +/* The current process.  */
> >> > +static process_info *current_process_;
> >> > +
> >> > +/* The current thread.  This is either a thread of CURRENT_PROCESS, or
> >> > +   NULL.  */
> >> >  struct thread_info *current_thread;
> >> >
> >> >  /* The current working directory used to start the inferior.
> >> > @@ -130,6 +135,7 @@ clear_inferiors (void)
> >> >    clear_dlls ();
> >> >
> >> >    switch_to_thread (nullptr);
> >> > +  current_process_ = nullptr;
> >> >  }
> >> >
> >> >  struct process_info *
> >> > @@ -153,6 +159,8 @@ remove_process (struct process_info *process)
> >> >    free_all_breakpoints (process);
> >> >    gdb_assert (find_thread_process (process) == NULL);
> >> >    all_processes.remove (process);
> >> > +  if (current_process () == process)
> >> > +    switch_to_process (nullptr);
> >> >    delete process;
> >> >  }
> >> >
> >> > @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *thread)
> >> >  struct process_info *
> >> >  current_process (void)
> >> >  {
> >> > -  gdb_assert (current_thread != NULL);
> >> > -  return get_thread_process (current_thread);
> >> > +  return current_process_;
> >>
> >> A bit late I know, but I wonder if the assert, or something similar,
> >> should have been retained here?
> >>
> >> The comment on this function currently (though Baris has a patch
> >> proposed to change this), says this function should only be called in a
> >> context where the result will not be nullptr.  Given that, not of the
> >> (many) existing uses check the return value of this function against
> >> nullptr.
> >>
> >> Happy to roll a patch to add the assert back, just wondered if there was
> >> a reason for its removal.
> >>
> >> Thanks,
> >> Andrew
> >
> > Hi Andrew,
> >
> > I think the current process can in fact be null in some brief periods.
> > For instance, in 'handle_detach' there is
> >
> >       if (extended_protocol || target_running ())
> >         {
> >           /* There is still at least one inferior remaining or
> >              we are in extended mode, so don't terminate gdbserver,
> >              and instead treat this like a normal program exit.  */
> >           cs.last_status.set_exited (0);
> >           cs.last_ptid = ptid_t (pid);
> >
> >           switch_to_thread (nullptr);
> >         }
> >
> > So, if the current process exits, gdbserver reports the event to GDB and
> > sets the current thread to nullptr, which also sets the current process
> > to nullptr.
> >
> > I believe an invariant is this:
> >
> >   (current_thread == nullptr) || (current_process () == get_thread_process
> (current_thread))
> >
> 
> I don't think this really addresses my question.  Here's how I
> understand things:
> 
>   1. Before this patch:
>      a. Comment on 'current_process' says: it's an error to call this
>         function when no thread is selected, and
>      b. The function asserted that a process was selected,
> 
>   2. This patch removed the assert, but left the comment unchanged,
> 
>   3. A patch was proposed to updated the comment,
> 
>   4. I couldn't see any reason in this patch _why_ the assert was
>      removed.
> 
> I agree that the process _could_ be nullptr, but it _could_ have been
> nullptr before.
> 
> My question is: did something change such that there is now a location
> where we might choose to call current_process when no thread is
> selected?

With Pedro's patch above, before reading/writing memory upon GDB's request,
gdbserver does a `switch_to_process`.  This sets current_thread to nullptr
while setting current_process_ to non-null.  At that moment, calling
current_process would have violated the assertion.

-Baris

> 
> Given the description of the original patch my guess was no, but maybe I
> should just add the assert back locally and do a test run to find out?
> 
> Thanks,
> Andrew


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