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

[Luis Machado <luis.machado@arm.com>]

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