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>]

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.

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,

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

 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?

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.

Thanks,
Andrew

---

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 2885afd60c7..f1f7675eb37 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2077,6 +2077,8 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
   else
     stop_signal = GDB_SIGNAL_0;
 
+  bool multiple_arches = false;
+  struct gdbarch *some_arch = nullptr;
   if (signalled_thr != nullptr)
     {
       /* On some architectures, like AArch64, each thread can have a distinct
@@ -2085,8 +2087,8 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
 
 	 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),
+      some_arch = target_thread_architecture (signalled_thr->ptid);
+      linux_corefile_thread (signalled_thr, some_arch,
 			     obfd, note_data, note_size, stop_signal);
     }
   for (thread_info *thr : current_inferior ()->non_exited_threads ())
@@ -2100,7 +2102,10 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
 
 	 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),
+      struct gdbarch *tmp = target_thread_architecture (thr->ptid);
+      if (tmp != some_arch)
+	multiple_arches = true;
+      linux_corefile_thread (thr, tmp,
 			     obfd, note_data, note_size, stop_signal);
     }
 
@@ -2125,7 +2130,8 @@ 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 (!multiple_arches)
+    gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
 
   return note_data;
 }