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

Luis Machado via Gdb-patches
2023-09-21T10:45:21Z

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.

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

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

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

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

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.

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

Thanks!
Luis

>
> 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;
> }
>
> ---
>