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-20T23:36:31Z

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.

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.

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

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?

By the way, thanks for the review.

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