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

[Andrew Burgess <aburgess@redhat.com>]

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