Re: [PATCH v5 13/16] [gdb/generic] corefile/bug: Fixup (gcore) core file target description reading order

Luis Machado via Gdb-patches
2023-09-12T08:50:25Z

On 9/8/23 18:10, Simon Marchi wrote:
> On 9/7/23 11:20, Luis Machado via Gdb-patches wrote:
>> 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 target, it doesn't work correctly for AArch64 with SVE or SME
>> support.
>>
>> The correct approach for AArch64/Linux is 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.
>>
>> I think this should work for other architectures as well. If not, we may
>> need to adjust things so all architectures get the information that they
>> need for discovering the target description of the core file.
>>
>> Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
>> ---
>> gdb/corelow.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 439270f5559..ae1641fe5d2 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -1229,6 +1229,21 @@ core_target::thread_alive (ptid_t ptid)
>> const struct target_desc *
>> core_target::read_description ()
>> {
>> + /* If the architecture provides a corefile target description hook, use
>> + it now. Even if the core file contains a target description in a note
>> + section, it is not useful for targets that can potentially have distinct
>> + descriptions for each thread. One example is AArch64's SVE/SME
>> + extensions that allow per-thread vector length changes, resulting in
>> + registers with different sizes. */
>> + if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
>> + {
>> + const struct target_desc *result;
>> +
>> + result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
>> + if (result != nullptr)
>> + return result;
>> + }
>> +
>> /* If the core file contains a target description note then we will use
>> that in preference to anything else. */
>> bfd_size_type tdesc_note_size = 0;
>> @@ -1252,15 +1267,6 @@ core_target::read_description ()
>> }
>> }
>>
>> - if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
>> - {
>> - const struct target_desc *result;
>> -
>> - result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
>> - if (result != NULL)
>> - return result;
>> - }
>> -
>> return this->beneath ()->read_description ();
>> }
>
> I'm not convinced that this is right. The current role of
> gdbarch_core_read_description (AFAIU) is to provide a fallback to the
> note method. Usually, the note method is preferred, because it's
> precise, but if there's no note, maybe the gdbarch can derive a tdesc
> from what it sees in the core. Naturally, this use of
> gdbarch_core_read_description (as a fallback) has to go after trying the
> note method.
>
> Now, you want to to use gdbarch_core_read_description as an override to
> the note method, which is why you want to call it before trying the note
> method. I don't think the same gdbarch method can be used for both
> fallback and override. With your change, with an arch that defines a
> gdbarch_core_read_description hook, where we would have used the note
> before, we will now always use the hook. Not what we want.

Yeah, it seems that way unfortunately. Looking back, maybe it would've been better to define the gdb tdesc note as a per-thread entry instead.

I suppose we can still do it from now on, but the previous core files generated by gdb would
still need to be handled in a special way for AArch64.

>
> Some options I see:
>
> - Add another gdbarch hook, so one is called before trying the note,
> and one after.
> - Add another gdbarch hook that allows the arch to modify the target
> desc read from the note. So the flow would be:
> core_target::read_description creates a target from the note, then
> calls the gdbarch hook. The latter could return the same tdesc, or a
> new tdesc. The AArch64 hook could then create a new tdesc based on
> the one read from the note, but with the SVE/SME bits tweaked.

I think the first solution would work. But I see it as a temporary measure until we update the core file target description note to be per-thread.

After that, I believe the gdbarch hook wouldn't be too useful.

>
> Simon