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

Simon Marchi via Gdb-patches
2023-09-08T17:11:12Z

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