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

[Luis Machado <luis.machado@arm.com>]

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;