Re: [PATCH v7 15/18] [gdb/generic] corefile/bug: Add hook to control the use of target description notes from corefiles

[Andrew Burgess <aburgess@redhat.com>]

Luis Machado <luis.machado@arm.com> writes:

> On 9/20/23 16:26, Andrew Burgess wrote:
>> Andrew Burgess <aburgess@redhat.com> writes:
>> 
>>> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>
>>>> New entry in v7.
>>>>
>>>> ---
>>>>
>>>> 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 targets, it doesn't work correctly for AArch64 with SVE or SME
>>>> support.
>>>>
>>>> The correct approach for AArch64/Linux is to either have per-thread target
>>>> description notes in the corefiles or 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.
>>>>
>>>> The former, although more correct, doesn't address the case of existing gdb's
>>>> that only output a single target description note.
>>>
>>> Do those existing GDB's support per-thread target descriptions though?
>>> I thought this series was the where the per-thread target description
>>> feature was being added ... so shouldn't core files generated by
>>> previous GDB's only support a single target description?  Or am I
>>> missing something.
>>>
>>>>
>>>> This patch goes for the latter, and adds a new gdbarch hook to conditionalize
>>>> the use of the corefile target description note. The hook is called
>>>> use_target_description_from_corefile_notes.
>>>>
>>>> The hook defaults to returning true, meaning targets will use the corefile
>>>> target description note.  AArch64 Linux overrides the hook to return false
>>>> when it detects any of the SVE or SME register notes in the corefile.
>>>>
>>>> Otherwise it should be fine for AArch64 Linux to use the corefile target
>>>> description note.
>>>>
>>>> When we support per-thread target description notes, then we can augment
>>>> the AArch64 Linux hook to rely on those notes.
>>>
>>> I guess I was a little surprised that I couldn't see anywhere in this
>>> series that you _stop_ adding the NT_GDB_TDESC note to core files
>>> generated from within GDB.
>>>
>>> I guess I was expecting to see some logic in gcore_elf_make_tdesc_note
>>> that tried to figure out if we had per-thread tdesc, and if so, at a
>>> minimum, didn't add the NT_GDB_TDESC.
>>>
>>> If we did that, then, I'm thinking:
>>>
>>>   - Previous GDB's only supported a single tdesc, and so are correct,
>>>
>>>   - New GDB's support per-thread tdesc, but don't emit NT_GDB_TDESC, so
>>>     we don't need to guard against loading them
>>>
>>> Maybe I'm missing something though.
>> 
>> Luis,
>> 
>> After our discussion on IRC I think I understand this a bit more, so
>> thanks for that.
>
> You're welcome. Hopefully I managed to clarify most of it.
>
>> 
>> So, here's what I think is true:
>> 
>>   - AArch64 supports per-thread gdbarch since before this patch series
>>     (implements ::thread_architecture),
>> 
>>   - GDB by default writes out a single NT_GDB_TDESC which describes the
>>     "per-inferior" gdbarch, but this doesn't correctly represent the
>>     per-thread gdbarch/tdesc,
>> 
>>   - If a NT_GDB_TDESC is present then GDB is going to try and load it
>>     back in and use it,
>> 
>>   - This is a problem that has existed for a while, but you've only just
>>     become aware, and so you're fixing it here,
>> 
>>   - The new gdbarch hook allows an architecture to avoid loading a
>>     single NT_GDB_TDESC note, for AArch64 this is when we see registers
>>     that are (likely) going to require per-thread gdbarch/tdesc,
>> 
>
> All of the above are correct.
>
>> I'm hoping we both agree on the above.  So from there I think I has two
>> concerns:
>> 
>>  1. GDB should avoid writing out the NT_GDB_TDESC is the situation where
>>     it know that a single such note is not correct, and
>
> I see your point now. Not outputting NT_GDB_TDESC when it is incorrect sounds
> reasonable, even if the incorrect NT_GDB_TDESC note won't be used.
>
> It is also reasonable to do it until the point where we support per-thread
> NT_GDB_TDESC's, which is, I think, the proper solution.
>
>> 
>>  2. I wonder if there's a better solution than the new hook.
>> 
>> For point (1) the patch below is a rough draft of what I'm thinking
>> about: by looking at each of the gdbarch pointers we can spot if an
>> inferior uses multiple different gdbarches -- if it does then we know
>> that a single NT_GDB_TDESC is going to be wrong, so lets not write it
>> out.
>> 
>> As was mentioned on IRC, longer term, it might be better if we wrote out
>> one tdesc for each thread, but that feels like a bigger job, and I think
>> stopping GDB doing something that's just *wrong* is pretty easy, so why
>> wouldn't we do that?
>
> I think outputting per-thread NT_GDB_TDESC's is fairly easy. Probably a matter of
> calling the function that outputs NT_GDB_TDESC while we're dumping register sets for
> each thread.

Indeed, I have this bit done locally.  What isn't clear to me is how
having multiple NT_GDB_TDESC will help -- does
core_file::read_description get called multiple times when using AArch64
with sve/sme?

>
> I haven't investigated in-depth how to use that information when loading a core file, but
> there might be some complexity because we read the target description before we load the threads.
>
> If the target description we read first is *always* the one for the signalled thread, then we might
> be ok and it should make the implementation easier. Anyway, that's a
> bit off-topic for this patch.

I mean, maybe this is a better solution?  What if
gcore_elf_make_tdesc_note took a 'struct gdbarch *' argument, and it was
the callers to chose which gdbarch to pass.  We would then spec in the
comments of gcore_elf_make_tdesc_note that the passed in gdbarch should
be the gdbarch of the signalled thread.  See the patch below.

Could I ask: would you mind sending me a small application and corefile
for AArch64 with sve/sme please.  I'd love to have a play to better
understand how GDB sets up the per-thread gdbarch in that case.  I'd be
very grateful.

>
>> 
>> For point (2) I agree with the premise that we need a mechanism to stop
>> AArch64 loading the incorrect single NT_GDB_TDESC.  This is a slight
>> change in stance from what I originally wrote, but our IRC conversation
>> showed me I was wrong originally.  I don't have time this evening to
>> look at this, but will follow up again tomorrow with more thoughts.

I wonder, instead of adding the new hook, maybe we should just reorder
the checks in core_target::read_description -- ask the gdbarch to grok a
tdesc from the .regs (etc) sections, and if that fails, check for an
encoded tdesc.

When I originally added this code my problem case was RISC-V, where the
CSRs (control regs) for embedded targets can be different for each
target.  The CSRs are just encoded as a blob, so unless you already know
which regs are present you're stuck.  For real h/w the controller
(e.g. openocd) provides a tdesc (via target.xml), but for a core file we
needed a way to reacquire the tdesc.

My assumption at the time that the tdesc we wrote would always be
correct, but clearly this isn't the case.

Right now RISC-V doesn't event override gdbarch_core_read_description,
so if we read the encoded tdesc after checking that gdbarch hook nothing
would change.

For other architectures, if gdbarch_core_read_description is implemented
then we'll be back to using that, and that worked fine before.

... And if, in some future world we do implement
gdbarch_core_read_description for RISC-V then the solution seems
obvious, if there's a section containing CSRs, and there's also a
section containing a tdesc, then the RISC-V
gdbarch_core_read_description can just return nullptr, safe in the
knowledge the generic GDB code will load the encoded tdesc.

>
> Except for some different names and tweaks, your proposed approach works correctly.
>
> I tested this and noticed the lack of NT_GDB_TDESC for a AArch64 Linux target
> with sve/sme support and one thread with a differing gdbarch. I also saw the note for
> a AArch64 Linux target without sve/sme support, or with sve/sme support but all threads
> having the exact same gdbarch. So that looks good to me.
>
> Would you like this extra code to be included as part of this
> particular patch?

Yeah I think something like this should be added to this patch, we
should stop GDB creating incorrect core files I think.

Thanks,
Andrew

----

diff --git a/gdb/elf-none-tdep.c b/gdb/elf-none-tdep.c
index 460f02e7dbb..ad31d3c820a 100644
--- a/gdb/elf-none-tdep.c
+++ b/gdb/elf-none-tdep.c
@@ -111,7 +111,9 @@ elf_none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd,
 
 
   /* Target description.  */
-  gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
+  if (signalled_thr != nullptr)
+    gdbarch = target_thread_architecture (signalled_thr->ptid);
+  gcore_elf_make_tdesc_note (gdbarch, obfd, &note_data, note_size);
 
   return note_data;
 }
diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
index dc5020d92d2..c44dc0871c6 100644
--- a/gdb/fbsd-tdep.c
+++ b/gdb/fbsd-tdep.c
@@ -774,7 +774,9 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
     }
 
   /* Include the target description when possible.  */
-  gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
+  if (signalled_thr != nullptr)
+    gdbarch = target_thread_architecture (signalled_thr->ptid);
+  gcore_elf_make_tdesc_note (gdbarch, obfd, &note_data, note_size);
 
   return note_data;
 }
diff --git a/gdb/gcore-elf.c b/gdb/gcore-elf.c
index 643426d911b..0142db82670 100644
--- a/gdb/gcore-elf.c
+++ b/gdb/gcore-elf.c
@@ -139,12 +139,12 @@ gcore_elf_build_thread_register_notes
 /* See gcore-elf.h.  */
 
 void
-gcore_elf_make_tdesc_note (bfd *obfd,
+gcore_elf_make_tdesc_note (struct gdbarch *gdbarch, bfd *obfd,
 			   gdb::unique_xmalloc_ptr<char> *note_data,
 			   int *note_size)
 {
   /* Append the target description to the core file.  */
-  const struct target_desc *tdesc = gdbarch_target_desc (target_gdbarch ());
+  const struct target_desc *tdesc = gdbarch_target_desc (gdbarch);
   const char *tdesc_xml
     = tdesc == nullptr ? nullptr : tdesc_get_features_xml (tdesc);
   if (tdesc_xml != nullptr && *tdesc_xml != '\0')
diff --git a/gdb/gcore-elf.h b/gdb/gcore-elf.h
index 2cfeb3e8550..e23fe5e7162 100644
--- a/gdb/gcore-elf.h
+++ b/gdb/gcore-elf.h
@@ -42,6 +42,7 @@ extern void gcore_elf_build_thread_register_notes
    set to nullptr.  */
 
 extern void gcore_elf_make_tdesc_note
-  (bfd *obfd, gdb::unique_xmalloc_ptr<char> *note_data, int *note_size);
+  (struct gdbarch *gdbarch, bfd *obfd,
+   gdb::unique_xmalloc_ptr<char> *note_data, int *note_size);
 
 #endif /* GCORE_ELF_H */
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 2885afd60c7..24ae1d711d1 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1836,6 +1836,10 @@ linux_corefile_thread (struct thread_info *info,
      such a core file is useless.  */
   if (note_data != nullptr)
     {
+      /* If we want per-thread NT_GDB_TDESC then at this point we should
+	 do this:  */
+      // gcore_elf_make_tdesc_note (gdbarch, obfd, &note_data, note_size);
+
       gdb::byte_vector siginfo_data
 	= linux_get_siginfo_data (info, gdbarch);
       if (!siginfo_data.empty ())
@@ -2125,7 +2129,9 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
   linux_make_mappings_corefile_notes (gdbarch, obfd, note_data, note_size);
 
   /* Target description.  */
-  gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
+  if (signalled_thr != nullptr)
+    gdbarch = target_thread_architecture (signalled_thr->ptid);
+  gcore_elf_make_tdesc_note (gdbarch, obfd, &note_data, note_size);
 
   return note_data;
 }

---