Re: [PATCH v5 1/5] gdb: make gdbarch store a vector of frame unwinders

[Simon Marchi <simark@simark.ca>]

On 10/1/24 2:42 PM, Guinevere Larsen wrote:
> From: Guinevere Larsen <blarsen@redhat.com>
> 
> Before this commit, all frame unwinders would be stored in the obstack
> of a gdbarch and accessed by using the registry system. This made for
> unwieldy code, and unnecessarily complex logic in the frame_unwinder
> implementation, along with making frame_unwind structs be unable to have
> non-trivial destructors.
> 
> Seeing as a future patch of this series wants to refactor the
> frame_unwind struct to use inheritance, and we'd like to not restrict
> the future derived classes on what destructors are allowed. In
> preparation for that change, this commit adds an std::vector to gdbarch
> to store the unwinders in.
> 
> There should be no user-visible changes.

Just some nits, but with those addressed:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> ---
>  gdb/arch-utils.c   |   8 ++++
>  gdb/frame-unwind.c | 113 +++++++++++++++++----------------------------
>  gdb/gdbarch-gen.c  |   6 +++
>  gdb/gdbarch.h      |   5 ++
>  gdb/gdbarch.py     |   6 +++
>  5 files changed, 67 insertions(+), 71 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 6ffa4109765..5a4ed097321 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1225,6 +1225,14 @@ obstack *gdbarch_obstack (gdbarch *arch)
>  
>  /* See gdbarch.h.  */
>  
> +std::vector<const frame_unwind *> &
> +gdbarch_unwinder_list (struct gdbarch *arch)
> +{
> +  return arch->unwinders;
> +}
> +
> +/* See gdbarch.h.  */
> +
>  char *
>  gdbarch_obstack_strdup (struct gdbarch *arch, const char *string)
>  {
> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index e5f108d3257..b69ae8596a2 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -31,62 +31,46 @@
>  #include "cli/cli-cmds.h"
>  #include "inferior.h"
>  
> -struct frame_unwind_table_entry
> +/* Default sniffers, that must always be the first in the unwinder list,
> +   no matter the architecture.  */
> +static constexpr auto standard_unwinders =
>  {
> -  const struct frame_unwind *unwinder;
> -  struct frame_unwind_table_entry *next;
> -};
> +  &dummy_frame_unwind,
> +  /* The DWARF tailcall sniffer must come before the inline sniffer.
> +     Otherwise, we can end up in a situation where a DWARF frame finds
> +     tailcall information, but then the inline sniffer claims a frame
> +     before the tailcall sniffer, resulting in confusion.  This is
> +     safe to do always because the tailcall sniffer can only ever be
> +     activated if the newer frame was created using the DWARF
> +     unwinder, and it also found tailcall information.  */
> +  &dwarf2_tailcall_frame_unwind,
> +  &inline_frame_unwind,
>  
> -struct frame_unwind_table
> -{
> -  struct frame_unwind_table_entry *list = nullptr;
> -  /* The head of the OSABI part of the search list.  */
> -  struct frame_unwind_table_entry **osabi_head = nullptr;
>  };
>  
> -static const registry<gdbarch>::key<struct frame_unwind_table>
> -     frame_unwind_data;
> +/* If an unwinder should be prepended to the list, this is the
> +   index in which it should be inserted.  */
> +static constexpr int prepend_unwinder_index = standard_unwinders.size ();
>  
> -/* A helper function to add an unwinder to a list.  LINK says where to
> -   install the new unwinder.  The new link is returned.  */
> -
> -static struct frame_unwind_table_entry **
> -add_unwinder (struct obstack *obstack, const struct frame_unwind *unwinder,
> -	      struct frame_unwind_table_entry **link)
> +/* Start the table out with a few default sniffers.  OSABI code
> +   can't override this.  */
> +static void
> +initialize_frame_unwind_table (std::vector<const frame_unwind *>& table)
>  {
> -  *link = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry);
> -  (*link)->unwinder = unwinder;
> -  return &(*link)->next;
> +  gdb_assert (table.empty ());
> +
> +  table.insert(table.begin (), standard_unwinders.begin (), standard_unwinders.end ());

Line a bit too long and missing space.

>  }
>  
> -static struct frame_unwind_table *
> +/* This function call retrieves the list of frame unwinders available in
> +   GDBARCH.  If this list is empty, it is initialized before being
> +   returned.  */

For brevity, I think you should remove "This function call", just start
with "Retrieve" or "Retrieves", depending of which school you are.

> +static std::vector<const frame_unwind *> &
>  get_frame_unwind_table (struct gdbarch *gdbarch)
>  {
> -  struct frame_unwind_table *table = frame_unwind_data.get (gdbarch);
> -  if (table != nullptr)
> -    return table;
> -
> -  table = new frame_unwind_table;
> -
> -  /* Start the table out with a few default sniffers.  OSABI code
> -     can't override this.  */
> -  struct frame_unwind_table_entry **link = &table->list;
> -
> -  struct obstack *obstack = gdbarch_obstack (gdbarch);
> -  link = add_unwinder (obstack, &dummy_frame_unwind, link);
> -  /* The DWARF tailcall sniffer must come before the inline sniffer.
> -     Otherwise, we can end up in a situation where a DWARF frame finds
> -     tailcall information, but then the inline sniffer claims a frame
> -     before the tailcall sniffer, resulting in confusion.  This is
> -     safe to do always because the tailcall sniffer can only ever be
> -     activated if the newer frame was created using the DWARF
> -     unwinder, and it also found tailcall information.  */
> -  link = add_unwinder (obstack, &dwarf2_tailcall_frame_unwind, link);
> -  link = add_unwinder (obstack, &inline_frame_unwind, link);
> -
> -  /* The insertion point for OSABI sniffers.  */
> -  table->osabi_head = link;
> -  frame_unwind_data.set (gdbarch, table);
> +  std::vector<const frame_unwind *> &table = gdbarch_unwinder_list (gdbarch);
> +  if (table.size () == 0)

table.empty ()

Simon