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/27/23 18:56, Andrew Burgess wrote:
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>>> Hi Andrew,
>>>
>>> On 9/25/23 10:57, Andrew Burgess wrote:
>>>> Luis Machado <luis.machado@arm.com> writes:
>>>>> Sure, that makes sense, and I don't disagree with that. But as I mentioned above, it is an
>>>>> orthogonal problem that might be better suited as its own patch/series. A more complete patch
>>>>> addressing this particular situation I stumbled upon.
>>>>
>>>> That's fine.  I understand the urgency you have.  For me, the only issue
>>>> I have here is that the new hook you add is to prevent GDB loading a
>>>> "broken" NT_GDB_TDESC. I'd also like this patch to include code that
>>>> prevents GDB from writing out "broken" NT_GDB_TDESC.
>>>>
>>>> I understand we're likely stuck with the "don't load the NT_GDB_TDESC"
>>>> hook due to broken GDB's already existing in the wild.  That sucks, but
>>>> it is what it is.
>>>>
>>>> However, having identified some broken behaviour I think we really
>>>> should fix that at the same time as adding the work around.
>>>>
>>>
>>> Yes, I see your point and think it's reasonable.
>>>
>>>>>> 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.
>>>>>
>>>>> That's true. With the introduction of dynamic target descriptions like sve and sme, this
>>>>> no longer holds true.
>>>>
>>>> OK. But is it really the case that the assumption is wrong, or is it
>>>> that the current code (that I added) is just using the wrong tdesc?
>>>
>>> For the general case, it is the wrong assumption because you can have threads
>>> with different tdesc's/gdbarches, and using a single NT_GDB_TDESC doesn't cover
>>> that part.
>>>
>>> Putting aside the threaded case, it can also be incorrect for the single-threaded
>>> case.
>>>
>>> Imagine you start up with a vector length of 256 bits (vg == 4). At this point the
>>> gdbarch/target description that gets stored in the inferior (the tdesc is still a
>>> per-inferior entry IMO) says the vector length is 256 bits.
>>>
>>> Now imagine we executed a few instructions and now the vector length is 128 bits
>>> (vg == 2). Though gdb will cope with it just fine during debugging, because
>>> aarch64/linux implements the thread_architecture hook, if we attempt to output
>>> a gcore core file, the current code (without your proposed patch) will dump
>>> the target description/gdbarch cached in the inferior. And that one says we
>>> have a vector length of 256 bits.
>>>
>>> So the tdesc is incorrect when we try to load the gcore file, if we trust the
>>> NT_GDB_TDESC note. The register data in the notes has the correct information
>>> though.
>> 
>> I think we agree on the above.
>> 
>>> Your attached patch fixes this because it uses the signalled thread's
>>> gdbarch/tdesc instead of the cached inferior gdbarch/tdesc.
>>>
>>> target_gdbarch can be a bit dangerous unfortunately, but we still have to use
>>> it in some places.
>> 
>> That's unfortunate.  I guess that might be because in some places we
>> have an inferior selected but no thread?
>> 
>> It feels like the right solution is to transition GDB to only having a
>> per-thread gdbarch/tdesc, which would avoid problems like this in the
>> future.
>> 
>>>
>>>>
>>>> I know you described the idea of updating gcore_elf_make_tdesc_note to
>>>> take a gdbarch* as orthogonal, but I really think this might be the way
>>>> to solve this problem.
>>>>
>>>> I've attached a patch at the end of this email that can be applied to
>>>> head of master BEFORE your patch series.  This patch updates the
>>>> NT_GDB_TDESC generation to use the gdbarch of the signalled thread.
>>>>
>>>> Now, if I've understood how everything works, I believe that the
>>>> NT_GDB_TDESC that is emitted _with_ my patch should be good enough -- it
>>>> should be the correct tdesc for the signalled thread, which, if all
>>>> threads use the same tdesc, should be good enough.
>>>>
>>>> As a test, you can apply my patch, and then TEMPORARILY update
>>>> aarch64_use_target_description_from_corefile_notes to always return
>>>> true.  Using such a GDB you should be able to create a core file, and
>>>> then load it back in correctly.
>>>
>>> Yes, that works correctly from testing on my end. So at a minimum that's
>>> already an improvement, as changing the vector length for one (or all threads,
>>> but with all of them still using the same vector length) during execution is a
>>> useful use case. Like a program setting up for some sve operation, and all of a
>>> someone wants to dump a core file.
>>>
>>>>
>>>> As I already mentioned, I acknowledge that we are stuck with the
>>>> aarch64_use_target_description_from_corefile_notes hook -- there are
>>>> broken GDBs in the wild.
>>>
>>> Though unfortunate, the most common core files are likely coming from crashes
>>> outside a debugging session (so not gcore).
>>>
>>>>
>>>> But, if I'm correct, then I think this is the best solution for now,
>>>> we're emitting a NT_GDB_TDESC that is mostly correct, but AArch64 will
>>>> continue to ignore it, just as you originally proposed.
>>>>
>>>
>>> Sorry, it took me a little bit to get some tests running on the simulator. Happy to
>>> report that I see no regressions in terms of testing with the changes you proposed for
>>> handling the signalled thread.
>>>
>>> Also, I checked the case I described above about a program starting with vg == 4 and then
>>> changing it to vg == 2 before we output a gcore file.
>>>
>>> When I load the corefile back in a patched gdb (and forcing gdb to use the tdesc, by always
>>> returning true in the aarch64-linux hook), I can see that the tdesc is the correct
>>> one (vector length indicating vg == 2).
>>>
>>> Without your patch, gdb used the inferior's cached gdbarch/tdesc to dump the NT_GDB_TDESC
>>> note, which means the vector length was vg == 4, when in reality it should've been
>>> vg == 2, as we had changed it before dumping the core file.
>> 
>> This all sounds positive.
>> 
>>>
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> That was my assessment back in the earlier versions of this series.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Got it.
>>>>>
>>>>> I'm afraid I'm going to need some clarification on ways forward with regards to this particular
>>>>> series. Do you still want this (or other) potential changes as part of this series or would you
>>>>> be ok with a follow-up patch/series to (try to, haven't gone in-depth yet) address this particular
>>>>> core file/gdbarch/threads situation?
>>>>
>>>> Here's what I propose:to output the cached gdbarch/
>>>>
>>>>   1. Try the patch I've supplied here, if this works for you (see above)
>>>>   then I think this is the best solution, we merge this, then go with v7
>>>>   of this patch unmodified,
>>>
>>> It did work. Thanks!
>>>
>>> But should we keep your suggested changes to check for distinct gdbarches in the threaded case? And
>>> not output NT_GDB_TDESC if we find multiple distinct gdbarch entries?
>> 
>> I don't think so.  As you've already said, in the core file case we
>> already don't support per-thread tdesc, even if we use the
>> aarch64_linux_core_read_description hook - that's only called once, and
>> will just generate a tdesc based on the main (signalled) thread as far
>> as I can tell.
>> 
>> Telling GDB to dump per-thread NT_GDB_TDESC is trivial, but I haven't
>> looked at updating the core file code to read in per-thread tdesc, but I
>> suspect you'll (eventually) need to solve that problem even when using
>> the aarch64_linux_core_read_description hook.
>> 
>> I know Simon has mentioned changing GDB to not write out NT_GDB_TDESC,
>> and I think that would be fine, however, I think we would still want my
>> proposed patch -- we should always write out the thread tdesc, not the
>> cached global one, as that is always going to be more accurate.
>> 
>> My proposal then would be: we merge my patch to write NT_GDB_TDESC using
>> the per-thread tdesc, then you merge your original V7 version of this
>> patch with no other changes.
>> 
>> Later, if we want, we can add a new hook to avoid writing the
>> NT_GDB_TDESC at all -- but that can come later, and would be in addition
>> to my proposed patch, not instead of it.
>> 
>> If you and Simon are happy with this plan then I'll go ahead and push my
>> patch, this should unblock you, and you'll be able to merge this series.
>> 
>> Let me know what you think,
>
> That sounds good to me. Thanks for going over this.
>
> Please let me know when you have pushed the NT_GDB_TDESC fixup.

I've pushed the patch.

I've included it again below, just for the record.

Thanks,
Andrew

---

commit c14993e9dc598a5ca126121f7d2a0898c26908bc
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Sep 25 10:32:14 2023 +0100

    gdb/corefile: write NT_GDB_TDESC based on signalled thread
    
    When creating a core file from within GDB we include a NT_GDB_TDESC
    that includes the target description of the architecture in use.
    
    For architectures with dynamic architectures (e.g. AArch64 with
    sve/sme) the original architecture, calculated from the original
    target description, might not match the per-thread architecture.
    
    In the general case, where each thread has a different architecture,
    then we really need a separate NT_GDB_TDESC for each thread, however,
    there's currently no way to read in multiple NT_GDB_TDESC.
    
    This commit is a step towards per-thread NT_GDB_TDESC.  In this commit
    I have updated the function that writes the NT_GDB_TDESC to accept a
    gdbarch (rather than calling target_gdbarch() to find a gdbarch), and
    I now pass in the gdbarch of the signalled thread.
    
    In many cases (though NOT all) targets with dynamic architectures
    really only use a single architecture, even when there are multiple
    threads, so in the common case, this should ensure that GDB emits an
    architecture that is more likely to be correct.
    
    Additional work will be needed in order to support corefiles with
    truly per-thread architectures, but that will need to be done in the
    future.

diff --git a/gdb/elf-none-tdep.c b/gdb/elf-none-tdep.c
index ce2db02aa9d..cae66004a26 100644
--- a/gdb/elf-none-tdep.c
+++ b/gdb/elf-none-tdep.c
@@ -110,8 +110,12 @@ elf_none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd,
     }
 
 
-  /* Target description.  */
-  gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
+  /* Include the target description when possible.  Some architectures
+     allow for per-thread gdbarch so we should really be emitting a tdesc
+     per-thread, however, we don't currently support reading in a
+     per-thread tdesc, so just emit the tdesc for the signalled thread.  */
+  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..d166d785736 100644
--- a/gdb/fbsd-tdep.c
+++ b/gdb/fbsd-tdep.c
@@ -773,8 +773,12 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
 	return NULL;
     }
 
-  /* Include the target description when possible.  */
-  gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
+  /* Include the target description when possible.  Some architectures
+     allow for per-thread gdbarch so we should really be emitting a tdesc
+     per-thread, however, we don't currently support reading in a
+     per-thread tdesc, so just emit the tdesc for the signalled thread.  */
+  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..826e3bebcee 100644
--- a/gdb/gcore-elf.h
+++ b/gdb/gcore-elf.h
@@ -37,11 +37,12 @@ extern void gcore_elf_build_thread_register_notes
    bfd *obfd, gdb::unique_xmalloc_ptr<char> *note_data, int *note_size);
 
 /* Add content to *NOTE_DATA (and update *NOTE_SIZE) to include a note
-   containing the current target's target description.  The core file is
+   containing the target description for GDBARCH.  The core file is
    being written to OBFD.  If something goes wrong then *NOTE_DATA can be
    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 22bb9686e8d..98658fa5a3f 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2128,8 +2128,12 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
   /* File mappings.  */
   linux_make_mappings_corefile_notes (gdbarch, obfd, note_data, note_size);
 
-  /* Target description.  */
-  gcore_elf_make_tdesc_note (obfd, &note_data, note_size);
+  /* Include the target description when possible.  Some architectures
+     allow for per-thread gdbarch so we should really be emitting a tdesc
+     per-thread, however, we don't currently support reading in a
+     per-thread tdesc, so just emit the tdesc for the signalled thread.  */
+  gdbarch = target_thread_architecture (signalled_thr->ptid);
+  gcore_elf_make_tdesc_note (gdbarch, obfd, &note_data, note_size);
 
   return note_data;
 }