Re: [PATCH v7 15/18] [gdb/generic] corefile/bug: Add hook to control the use of target description notes from corefiles

Andrew Burgess via Gdb-patches
2023-09-20T14:23:21Z

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> New entry in v7.
>
> ---
>
> 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 targets, it doesn't work correctly for AArch64 with SVE or SME
> support.
>
> The correct approach for AArch64/Linux is to either have per-thread target
> description notes in the corefiles or 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.
>
> The former, although more correct, doesn't address the case of existing gdb's
> that only output a single target description note.

Do those existing GDB's support per-thread target descriptions though?
I thought this series was the where the per-thread target description
feature was being added ... so shouldn't core files generated by
previous GDB's only support a single target description? Or am I
missing something.

>
> This patch goes for the latter, and adds a new gdbarch hook to conditionalize
> the use of the corefile target description note. The hook is called
> use_target_description_from_corefile_notes.
>
> The hook defaults to returning true, meaning targets will use the corefile
> target description note. AArch64 Linux overrides the hook to return false
> when it detects any of the SVE or SME register notes in the corefile.
>
> Otherwise it should be fine for AArch64 Linux to use the corefile target
> description note.
>
> When we support per-thread target description notes, then we can augment
> the AArch64 Linux hook to rely on those notes.

I guess I was a little surprised that I couldn't see anywhere in this
series that you _stop_ adding the NT_GDB_TDESC note to core files
generated from within GDB.

I guess I was expecting to see some logic in gcore_elf_make_tdesc_note
that tried to figure out if we had per-thread tdesc, and if so, at a
minimum, didn't add the NT_GDB_TDESC.

If we did that, then, I'm thinking:

- Previous GDB's only supported a single tdesc, and so are correct,

- New GDB's support per-thread tdesc, but don't emit NT_GDB_TDESC, so
we don't need to guard against loading them

Maybe I'm missing something though.

Thanks,
Andrew

>
> Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
> ---
> gdb/aarch64-linux-tdep.c | 33 ++++++++++++++++++++++++++
> gdb/arch-utils.c | 10 ++++++++
> gdb/arch-utils.h | 6 +++++
> gdb/corelow.c | 50 ++++++++++++++++++++++++---------------
> gdb/gdbarch-gen.h | 14 +++++++++++
> gdb/gdbarch.c | 22 +++++++++++++++++
> gdb/gdbarch_components.py | 19 +++++++++++++++
> 7 files changed, 135 insertions(+), 19 deletions(-)
>
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 47e5e1db641..21ac7ebdc56 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2199,6 +2199,33 @@ aarch64_linux_decode_memtag_section (struct gdbarch *gdbarch,
> return tags;
> }
>
> +/* AArch64 Linux implementation of the
> + gdbarch_use_target_description_from_corefile_notes hook. */
> +
> +static bool
> +aarch64_use_target_description_from_corefile_notes (gdbarch *gdbarch,
> + bfd *obfd)
> +{
> + /* Sanity check. */
> + gdb_assert (obfd != nullptr);
> +
> + /* If the corefile contains any SVE or SME register data, we don't want to
> + use the target description note, as it may be incorrect.
> +
> + Currently the target description note contains a potentially incorrect
> + target description if the originating program changed the SVE or SME
> + vector lengths mid-execution.
> +
> + Once we support per-thread target description notes in the corefiles, we
> + can always trust those notes whenever they are available. */
> + if (bfd_get_section_by_name (obfd, ".reg-aarch-sve") != nullptr
> + || bfd_get_section_by_name (obfd, ".reg-aarch-za") != nullptr
> + || bfd_get_section_by_name (obfd, ".reg-aarch-zt") != nullptr)
> + return false;
> +
> + return true;
> +}
> +
> static void
> aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> {
> @@ -2469,6 +2496,12 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> aarch64_displaced_step_hw_singlestep);
>
> set_gdbarch_gcc_target_options (gdbarch, aarch64_linux_gcc_target_options);
> +
> + /* Hook to decide if the target description should be obtained from
> + corefile target description note(s) or inferred from the corefile
> + sections. */
> + set_gdbarch_use_target_description_from_corefile_notes (gdbarch,
> + aarch64_use_target_description_from_corefile_notes);
> }
>
> #if GDB_SELF_TEST
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index ee34fc07d33..c1d2c939eb9 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1092,6 +1092,16 @@ default_read_core_file_mappings
> {
> }
>
> +/* See arch-utils.h. */
> +bool
> +default_use_target_description_from_corefile_notes (struct gdbarch *gdbarch,
> + struct bfd *obfd)
> +{
> + /* Always trust the corefile target description contained in the target
> + description note. */
> + return true;
> +}
> +
> CORE_ADDR
> default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
> {
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 2bdc3251c9c..5df3de7b5d9 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -305,6 +305,12 @@ extern void default_read_core_file_mappings
> read_core_file_mappings_pre_loop_ftype pre_loop_cb,
> read_core_file_mappings_loop_ftype loop_cb);
>
> +/* Default implementation of gdbarch
> + use_target_description_from_corefile_notes. */
> +extern bool default_use_target_description_from_corefile_notes
> + (struct gdbarch *gdbarch,
> + struct bfd *obfd);
> +
> /* Default implementation of gdbarch default_get_return_buf_addr method. */
> extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
> frame_info_ptr cur_frame);
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 439270f5559..114ce3054d5 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -1229,35 +1229,47 @@ core_target::thread_alive (ptid_t ptid)
> const struct target_desc *
> core_target::read_description ()
> {
> - /* 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;
> - struct bfd_section *tdesc_note_section
> - = bfd_get_section_by_name (core_bfd, ".gdb-tdesc");
> - if (tdesc_note_section != nullptr)
> - tdesc_note_size = bfd_section_size (tdesc_note_section);
> - if (tdesc_note_size > 0)
> + /* First check whether the target wants us to use the corefile target
> + description notes. */
> + if (gdbarch_use_target_description_from_corefile_notes (m_core_gdbarch,
> + core_bfd))
> {
> - gdb::char_vector contents (tdesc_note_size + 1);
> - if (bfd_get_section_contents (core_bfd, tdesc_note_section,
> - contents.data (), (file_ptr) 0,
> - tdesc_note_size))
> + /* If the core file contains a target description note then go ahead and
> + use that. */
> + bfd_size_type tdesc_note_size = 0;
> + struct bfd_section *tdesc_note_section
> + = bfd_get_section_by_name (core_bfd, ".gdb-tdesc");
> + if (tdesc_note_section != nullptr)
> + tdesc_note_size = bfd_section_size (tdesc_note_section);
> + if (tdesc_note_size > 0)
> {
> - /* Ensure we have a null terminator. */
> - contents[tdesc_note_size] = '\0';
> - const struct target_desc *result
> - = string_read_description_xml (contents.data ());
> - if (result != nullptr)
> - return result;
> + gdb::char_vector contents (tdesc_note_size + 1);
> + if (bfd_get_section_contents (core_bfd, tdesc_note_section,
> + contents.data (), (file_ptr) 0,
> + tdesc_note_size))
> + {
> + /* Ensure we have a null terminator. */
> + contents[tdesc_note_size] = '\0';
> + const struct target_desc *result
> + = string_read_description_xml (contents.data ());
> + if (result != nullptr)
> + return result;
> + }
> }
> }
>
> + /* 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 != NULL)
> + if (result != nullptr)
> return result;
> }
>
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index d62eefa1c5b..33276aa1c43 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1717,3 +1717,17 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
> typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
> extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
> extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
> +
> +/* Return true if the target description for all threads should be read from the
> + target description core file note(s). Return false if the target description
> + for all threads should be inferred from the core file contents/sections.
> +
> + The corefile's bfd is passed through OBFD.
> +
> + This hook should be used by targets that can have distinct target descriptions
> + for each thread when the core file only holds a single target description
> + note. */
> +
> +typedef bool (gdbarch_use_target_description_from_corefile_notes_ftype) (struct gdbarch *gdbarch, struct bfd *obfd);
> +extern bool gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, struct bfd *obfd);
> +extern void set_gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes);
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 1fc254d3d6e..ee868908598 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -256,6 +256,7 @@ struct gdbarch
> gdbarch_type_align_ftype *type_align = default_type_align;
> gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
> gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
> + gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes = default_use_target_description_from_corefile_notes;
> };
>
> /* Create a new ``struct gdbarch'' based on information provided by
> @@ -523,6 +524,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
> /* Skip verify of type_align, invalid_p == 0 */
> /* Skip verify of get_pc_address_flags, invalid_p == 0 */
> /* Skip verify of read_core_file_mappings, invalid_p == 0 */
> + /* Skip verify of use_target_description_from_corefile_notes, invalid_p == 0 */
> if (!log.empty ())
> internal_error (_("verify_gdbarch: the following are invalid ...%s"),
> log.c_str ());
> @@ -1373,6 +1375,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
> gdb_printf (file,
> "gdbarch_dump: read_core_file_mappings = <%s>\n",
> host_address_to_string (gdbarch->read_core_file_mappings));
> + gdb_printf (file,
> + "gdbarch_dump: use_target_description_from_corefile_notes = <%s>\n",
> + host_address_to_string (gdbarch->use_target_description_from_corefile_notes));
> if (gdbarch->dump_tdep != NULL)
> gdbarch->dump_tdep (gdbarch, file);
> }
> @@ -5409,3 +5414,20 @@ set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch,
> {
> gdbarch->read_core_file_mappings = read_core_file_mappings;
> }
> +
> +bool
> +gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch, struct bfd *obfd)
> +{
> + gdb_assert (gdbarch != NULL);
> + gdb_assert (gdbarch->use_target_description_from_corefile_notes != NULL);
> + if (gdbarch_debug >= 2)
> + gdb_printf (gdb_stdlog, "gdbarch_use_target_description_from_corefile_notes called\n");
> + return gdbarch->use_target_description_from_corefile_notes (gdbarch, obfd);
> +}
> +
> +void
> +set_gdbarch_use_target_description_from_corefile_notes (struct gdbarch *gdbarch,
> + gdbarch_use_target_description_from_corefile_notes_ftype use_target_description_from_corefile_notes)
> +{
> + gdbarch->use_target_description_from_corefile_notes = use_target_description_from_corefile_notes;
> +}
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 846467b8d83..bbb9b188286 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -2732,3 +2732,22 @@ Read core file mappings
> predefault="default_read_core_file_mappings",
> invalid=False,
> )
> +
> +Method(
> + comment="""
> +Return true if the target description for all threads should be read from the
> +target description core file note(s). Return false if the target description
> +for all threads should be inferred from the core file contents/sections.
> +
> +The corefile's bfd is passed through OBFD.
> +
> +This hook should be used by targets that can have distinct target descriptions
> +for each thread when the core file only holds a single target description
> +note.
> +""",
> + type="bool",
> + name="use_target_description_from_corefile_notes",
> + params=[("struct bfd *", "obfd")],
> + predefault="default_use_target_description_from_corefile_notes",
> + invalid=False,
> +)
> --
> 2.25.1