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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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