diff mbox series

[v6,14/17,gdb/generic] corefile/bug: Use thread-specific gdbarch when dumping register state to core files

Message ID 20230913101815.178154-15-luis.machado@arm.com
State New
Headers
Series SME support for AArch64 gdb/gdbserver on Linux |

Checks

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

Commit Message

Luis Machado Sept. 13, 2023, 10:18 a.m. UTC 
  When we have a core file generated by gdb (via the gcore command), gdb dumps
the target description to a note.  During loading of that core file, gdb will
first try to load that saved target description.

This works fine for almost all architectures. But AArch64 has a few
dynamically-generated target descriptions/gdbarch depending on the vector
length that was in use at the time the core file was generated.

The target description gdb dumps to the core file note is the one generated
at the time of attachment/startup.  If, for example, the SVE vector length
changed during execution, this would not reflect on the core file, as gdb
would still dump the initial target description.

Another issue is that the gdbarch potentially doesn't match the thread's
real gdbarch, and so things like the register cache may have different formats
and sizes.

To address this, fetch the thread's architecture before dumping its register
state.  That way we will always use the correct target description/gdbarch.
---
 gdb/linux-tdep.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Simon Marchi Sept. 13, 2023, 2:20 p.m. UTC | #1 
On 9/13/23 06:18, Luis Machado wrote:
> When we have a core file generated by gdb (via the gcore command), gdb dumps
> the target description to a note.  During loading of that core file, gdb will
> first try to load that saved target description.
> 
> This works fine for almost all architectures. But AArch64 has a few
> dynamically-generated target descriptions/gdbarch depending on the vector
> length that was in use at the time the core file was generated.
> 
> The target description gdb dumps to the core file note is the one generated
> at the time of attachment/startup.  If, for example, the SVE vector length
> changed during execution, this would not reflect on the core file, as gdb
> would still dump the initial target description.
> 
> Another issue is that the gdbarch potentially doesn't match the thread's
> real gdbarch, and so things like the register cache may have different formats
> and sizes.
> 
> To address this, fetch the thread's architecture before dumping its register
> state.  That way we will always use the correct target description/gdbarch.
> ---
>  gdb/linux-tdep.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index a4a86b01bdb..2885afd60c7 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -2078,15 +2078,30 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
>      stop_signal = GDB_SIGNAL_0;
>  
>    if (signalled_thr != nullptr)
> -    linux_corefile_thread (signalled_thr, gdbarch, obfd, note_data, note_size,
> -			   stop_signal);
> +    {
> +      /* On some architectures, like AArch64, each thread can have a distinct
> +	 gdbarch (due to scalable extensions), and using the inferior gdbarch
> +	 is incorrect.
> +
> +	 Fetch each thread's gdbarch and pass it down to the lower layers so
> +	 we can dump the right set of registers.  */
> +      linux_corefile_thread (signalled_thr,
> +			     target_thread_architecture (signalled_thr->ptid),
> +			     obfd, note_data, note_size, stop_signal);
> +    }
>    for (thread_info *thr : current_inferior ()->non_exited_threads ())
>      {
>        if (thr == signalled_thr)
>  	continue;
>  
> -      linux_corefile_thread (thr, gdbarch, obfd, note_data, note_size,
> -			     stop_signal);
> +      /* On some architectures, like AArch64, each thread can have a distinct
> +	 gdbarch (due to scalable extensions), and using the inferior gdbarch
> +	 is incorrect.
> +
> +	 Fetch each thread's gdbarch and pass it down to the lower layers so
> +	 we can dump the right set of registers.  */
> +      linux_corefile_thread (thr, target_thread_architecture (thr->ptid),
> +			     obfd, note_data, note_size, stop_signal);
>      }
>  
>    if (!note_data)
> -- 
> 2.25.1
> 

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
Tom Tromey Sept. 13, 2023, 3:11 p.m. UTC | #2 
>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

Luis> When we have a core file generated by gdb (via the gcore command), gdb dumps
Luis> the target description to a note.  During loading of that core file, gdb will
Luis> first try to load that saved target description.

Luis> This works fine for almost all architectures. But AArch64 has a few
Luis> dynamically-generated target descriptions/gdbarch depending on the vector
Luis> length that was in use at the time the core file was generated.

Luis> The target description gdb dumps to the core file note is the one generated
Luis> at the time of attachment/startup.  If, for example, the SVE vector length
Luis> changed during execution, this would not reflect on the core file, as gdb
Luis> would still dump the initial target description.

Luis> Another issue is that the gdbarch potentially doesn't match the thread's
Luis> real gdbarch, and so things like the register cache may have different formats
Luis> and sizes.

Luis> To address this, fetch the thread's architecture before dumping its register
Luis> state.  That way we will always use the correct target description/gdbarch.

FWIW this seems fine to me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
diff mbox series

Patch

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index a4a86b01bdb..2885afd60c7 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2078,15 +2078,30 @@  linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
     stop_signal = GDB_SIGNAL_0;
 
   if (signalled_thr != nullptr)
-    linux_corefile_thread (signalled_thr, gdbarch, obfd, note_data, note_size,
-			   stop_signal);
+    {
+      /* On some architectures, like AArch64, each thread can have a distinct
+	 gdbarch (due to scalable extensions), and using the inferior gdbarch
+	 is incorrect.
+
+	 Fetch each thread's gdbarch and pass it down to the lower layers so
+	 we can dump the right set of registers.  */
+      linux_corefile_thread (signalled_thr,
+			     target_thread_architecture (signalled_thr->ptid),
+			     obfd, note_data, note_size, stop_signal);
+    }
   for (thread_info *thr : current_inferior ()->non_exited_threads ())
     {
       if (thr == signalled_thr)
 	continue;
 
-      linux_corefile_thread (thr, gdbarch, obfd, note_data, note_size,
-			     stop_signal);
+      /* On some architectures, like AArch64, each thread can have a distinct
+	 gdbarch (due to scalable extensions), and using the inferior gdbarch
+	 is incorrect.
+
+	 Fetch each thread's gdbarch and pass it down to the lower layers so
+	 we can dump the right set of registers.  */
+      linux_corefile_thread (thr, target_thread_architecture (thr->ptid),
+			     obfd, note_data, note_size, stop_signal);
     }
 
   if (!note_data)