diff mbox series

[v3,13/16,gdb/generic] corefile/bug: Fixup (gcore) core file target description reading order

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

Commit Message

Luis Machado June 30, 2023, 1:46 p.m. UTC 
  Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread
can potentially have distinct target descriptions/gdbarches.

When loading a gcore-generated core file, at the moment GDB gives priority
to the target description dumped to NT_GDB_TDESC.  Though technically correct
for most target, it doesn't work correctly for AArch64 with SVE or SME
support.

The correct approach for AArch64/Linux is to rely on the
gdbarch_core_read_description hook, so it can figure out the proper target
description for a given thread based on the various available register notes.

I think this should work for other architectures as well. If not, we may
need to adjust things so all architectures get the information that they
need for discovering the target description of the core file.

Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
---
 gdb/corelow.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Thiago Jung Bauermann July 28, 2023, 3:12 a.m. UTC | #1 
Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread
> can potentially have distinct target descriptions/gdbarches.
>
> When loading a gcore-generated core file, at the moment GDB gives priority
> to the target description dumped to NT_GDB_TDESC.  Though technically correct
> for most target, it doesn't work correctly for AArch64 with SVE or SME
> support.
>
> The correct approach for AArch64/Linux is to rely on the
> gdbarch_core_read_description hook, so it can figure out the proper target
> description for a given thread based on the various available register notes.
>
> I think this should work for other architectures as well. If not, we may
> need to adjust things so all architectures get the information that they
> need for discovering the target description of the core file.

The code to load the target description from the core file was added in
commit 95ce627aeb9d "gdb: write target description into core file", by
Andrew Burgess. My understanding of that commit message is that it was
added before the gdbarch hook on purpose, and my reading of:

 “My primary motivation for adding this feature is that, in a future
  commit, I will be adding support for bare metal core dumps on some
  targets.  For RISC-V specifically, I want to be able to dump all the
  available control status registers.  As different targets will present
  different sets of register in their target description, including
  registers that are possibly not otherwise known to GDB I wanted a way
  to capture these registers in the core dump.”

is that it doesn't work if reading from the target description comes
last, though I'm not sure about that. I copied him in this email to see
if he would like to chime in.
Luis Machado July 31, 2023, 11:38 a.m. UTC | #2 
On 7/28/23 04:12, Thiago Jung Bauermann wrote:
> 
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread
>> can potentially have distinct target descriptions/gdbarches.
>>
>> When loading a gcore-generated core file, at the moment GDB gives priority
>> to the target description dumped to NT_GDB_TDESC.  Though technically correct
>> for most target, it doesn't work correctly for AArch64 with SVE or SME
>> support.
>>
>> The correct approach for AArch64/Linux is to rely on the
>> gdbarch_core_read_description hook, so it can figure out the proper target
>> description for a given thread based on the various available register notes.
>>
>> I think this should work for other architectures as well. If not, we may
>> need to adjust things so all architectures get the information that they
>> need for discovering the target description of the core file.
> 
> The code to load the target description from the core file was added in
> commit 95ce627aeb9d "gdb: write target description into core file", by
> Andrew Burgess. My understanding of that commit message is that it was
> added before the gdbarch hook on purpose, and my reading of:
> 
>  “My primary motivation for adding this feature is that, in a future
>   commit, I will be adding support for bare metal core dumps on some
>   targets.  For RISC-V specifically, I want to be able to dump all the
>   available control status registers.  As different targets will present
>   different sets of register in their target description, including
>   registers that are possibly not otherwise known to GDB I wanted a way
>   to capture these registers in the core dump.”
> 
> is that it doesn't work if reading from the target description comes
> last, though I'm not sure about that. I copied him in this email to see
> if he would like to chime in.
> 

I think the risc-v case should still work as it doesn't provide a core_read_description hook. If one is added though, then gdb will
give that hook preference.

Usually the core_read_description hook is provided when the target runs hosted on an OS. So for bare metal we wouldn't have that hook implemented.

Still, I agree we might want to make sure both of these features coexist. The assumption a tdesc is the same for all the threads of a process, and that
this tdesc is exposed through a single note in the core file, doesn't hold true for AArch64 unfortunately.

It would've been true if we had a sizeless vector type, in which case all the threads of a process for AArch64 would have the exact same tdesc.
Luis Machado Sept. 5, 2023, 8:28 a.m. UTC | #3 
Hi Andrew,

On 7/28/23 04:12, Thiago Jung Bauermann wrote:
> 
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> Due to the nature of the AArch64 SVE/SME extensions in GDB, each thread
>> can potentially have distinct target descriptions/gdbarches.
>>
>> When loading a gcore-generated core file, at the moment GDB gives priority
>> to the target description dumped to NT_GDB_TDESC.  Though technically correct
>> for most target, it doesn't work correctly for AArch64 with SVE or SME
>> support.
>>
>> The correct approach for AArch64/Linux is to rely on the
>> gdbarch_core_read_description hook, so it can figure out the proper target
>> description for a given thread based on the various available register notes.
>>
>> I think this should work for other architectures as well. If not, we may
>> need to adjust things so all architectures get the information that they
>> need for discovering the target description of the core file.
> 
> The code to load the target description from the core file was added in
> commit 95ce627aeb9d "gdb: write target description into core file", by
> Andrew Burgess. My understanding of that commit message is that it was
> added before the gdbarch hook on purpose, and my reading of:
> 
>  “My primary motivation for adding this feature is that, in a future
>   commit, I will be adding support for bare metal core dumps on some
>   targets.  For RISC-V specifically, I want to be able to dump all the
>   available control status registers.  As different targets will present
>   different sets of register in their target description, including
>   registers that are possibly not otherwise known to GDB I wanted a way
>   to capture these registers in the core dump.”
> 
> is that it doesn't work if reading from the target description comes
> last, though I'm not sure about that. I copied him in this email to see
> if he would like to chime in.
> 

It would be great to have your input on this, as the approaches seem to conflict somewhat.

Could you please take a look at this entry (and, if possible, in the other generic pieces too), to make
sure we can come up with something that works for all cases?

Thanks,
Luis
diff mbox series

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 0a7ff56848a..de750b6c039 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -1080,6 +1080,21 @@  core_target::thread_alive (ptid_t ptid)
 const struct target_desc *
 core_target::read_description ()
 {
+  /* If the architecture provides a corefile target description hook, use
+     it now.  Even if the core file contains a target description in a note
+     section, it is not useful for targets that can potentially have distinct
+     descriptions for each thread.  One example is AArch64's SVE/SME
+     extensions that allow per-thread vector length changes, resulting in
+     registers with different sizes.  */
+  if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
+    {
+      const struct target_desc *result;
+
+      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
+      if (result != nullptr)
+	return result;
+    }
+
   /* If the core file contains a target description note then we will use
      that in preference to anything else.  */
   bfd_size_type tdesc_note_size = 0;
@@ -1103,15 +1118,6 @@  core_target::read_description ()
 	}
     }
 
-  if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
-    {
-      const struct target_desc *result;
-
-      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
-      if (result != NULL)
-	return result;
-    }
-
   return this->beneath ()->read_description ();
 }