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:

> Hi Andrew,
>
> On 9/21/23 11:02, Andrew Burgess wrote:
>> 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?
>> 
>
> No, it doesn't get called multiple times. And to be honest, I don't think it would help at
> the moment, even if we did output multiple NT_GDB_TDESC entries.
>
> The reason being, IIRC, that we lack a thread_architecture hook. That's available for the
> AArch64 Linux native target and there have been attempts at having it for the remote
> target as well (which would fix the problem of communicating vector length changes over RSP).
>
> So, similarly, we'd need something like a thread_architecture implementation for core files,
> otherwise it wouldn't solve the problem completely.
>
> To clarify, the more generic issue of per-thread target descriptions (with or without NT_GDB_TDESC's)
> from threaded core files isn't completely solved by this patch either.  I haven't gone through that path
> yet, as it is a bit outside the scope of sme1/sme2 enablement. It is an orthogonal problem, that I happened
> to have a pass at improving (not completely fixing).
>
> With this patch, at least gdb can load the core file and display sane values for single-threaded
> core files with sme data.

OK, that makes so much sense -- I assumed multi-threaded stuff was
handled, but I couldn't figure out how it was supposed to work!

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

>
>> 
>> 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.
>> 
>
> Absolutely, that would be no problem. I can send you some of those. You will need more than
> one, as the different states might influence things. But, as I explained earlier, gdb won't
> setup the per-thread gdbarch in that case.
>
> If you'd still like to have a go, let me know which ones you want (no sve, sve,
> sve+sme, threads).

OK, that's probably not necessary, I was mostly trying to figure out how
the multi-threaded core-file support worked, and I guess the answer is
that right now, it doesn't in the general case, but, as you mention, if
all threads use the same tdesc, then we're probably fine.  Knowing that,
things make much more sense.

>
>>>
>>>>
>>>> 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.
>> 
>
> That's exactly what I did a few versions earlier. But Simon pointed out it would
> effectively be a conflicting situation given our choice of defaulting to reading the
> target description from the NT_GDB_TDESC. So I went with the hook, which, to be honest,
> seems cleaner.
>
>> 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?

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.

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.

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.

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

  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,

  2. If the patch above doesn't work, then we go with the dynamic
  gdbarch detection I proposed, which I think you said worked fine to
  prevent "broken" NT_GDB_TDESC from being emitted.

>
> I'd like to have the sme support in for soon-to-branch gdb 14. I don't know how long the community
> would be willing to wait for a potential fix for this problem before
> branching/releasing.

Understood.  I think we have a plan.

> I don't see it as a critical problem, to be honest. Most of the concern is around core files generated
> by threaded programs for which some threads have changed the vector length mid-execution. That is
> an uncommon case. The most common case is that of all threads having the same gdbarch, because they
> are all using the same vector length (for sve, sme etc).

Even signal threaded programs are going to emit the wrong NT_GDB_TDESC
right now though, so I'd really like to at least solve that problem.

I'm 100% NOT suggesting we need to solve the general, many threads with
many architectures problem right now.  I'm not even suggesting that you
need to promise to immediately start looking at it.  I just want GDB to
not emit wrong information in the simple case.  That's all.

---

commit 06546f13ffc5b71fdc65190b529b29575d4f4865
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 460f02e7dbb..1e99b2fbb37 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 b5eee5e108c..f82a3e44839 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;
 }