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

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

Could a global maintainer please go through this change and let me know if it is OK? It touches a generic part of gdb as well.

This has the potential to cause some issues for some targets relying on the gdb xml core file note.

On 9/7/23 16: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 ();
>  }
>
  

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.

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.

Simon
  

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
  

>> 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.
Well, as you mentioned above, there will still be the case of supporting
old cores that only have one global target description note, right?

Simon
  

On 9/13/23 14:50, Simon Marchi wrote:
>>> 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.
> Well, as you mentioned above, there will still be the case of supporting
> old cores that only have one global target description note, right?

Yes, that's true. Though I'd expect the most common case of generated
core files to be ones generated by a crash as opposed to gdb's gcore command. And
the core files generated by the Linux kernel, for instance, wouldn't carry the target
description note.

But, as I mentioned on IRC, even with the introduction of such a gdbarch hook, we'd still
run into other issues that need to be fixed before threaded core files will work correctly
for AArch64's SVE and SME. It is something that is broken currently. But those would be
best addressed with a different patch set.

> 
> Simon
>