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

[Simon Marchi]

On 7/3/24 1:08 PM, Guinevere Larsen wrote:
> 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.

I'll try to review this series with a time I have, it won't be a
complete review right away, but hopefully we can make some progress.

> ---
>  gdb/arch-utils.c   |   8 ++++
>  gdb/frame-unwind.c | 111 +++++++++++++++++----------------------------
>  gdb/gdbarch.c      |   3 ++
>  gdb/gdbarch.h      |   5 ++
>  gdb/gdbarch.py     |   3 ++
>  5 files changed, 61 insertions(+), 69 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 83e29470bf7..99fc3a07ac3 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1219,6 +1219,14 @@ obstack *gdbarch_obstack (gdbarch *arch)
>  
>  /* See gdbarch.h.  */
>  
> +std::vector<const frame_unwind *>&

Space before &.

> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index e5f108d3257..0bb47deea3a 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -31,49 +31,16 @@
>  #include "cli/cli-cmds.h"
>  #include "inferior.h"
>  
> -struct frame_unwind_table_entry
> -{
> -  const struct frame_unwind *unwinder;
> -  struct frame_unwind_table_entry *next;
> -};
> -
> -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;
> -};
> +/* If an unwinder should be prepended to the list, this is the
> +   index in which it should be inserted.  */
> +static int osabi_start = -1;

I find it a bit odd to have this one global variable, given that we have
many lists (one per gdbarch).  I understand that the set of "fixed"
unwinders will be the same for all gdbarches, so ultimately osabi_start
will have value 3, and it will stay like this for all its lifetime.  But
it shows that it doesn't really need to be a runtime thing.

Here's a proposal.  Make a global constexpr list of "standard" unwinders
(or whatever we want to call them):

  static constexpr auto standard_unwinders =
  {
    &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,
  };

Then compute the index where custom arch-specific unwinders start:

  /* The index in an undwinder list where osabi unwinders start (after the
     standard unwinders).  */
  static constexpr auto osabi_unwinders_start_index = standard_unwinders.size ();

(I don't think that "osabi" is a good name here, because it's not only
osabis who register additional unwinders, perhaps think of a better
name)

Then, initialize_frame_unwind_table can look like this:

  static void
  initialize_frame_unwind_table (std::vector<const frame_unwind *>& table)
  {
    gdb_assert (table.empty ());

    table.insert (table.begin (), standard_unwinders.begin (),
  		standard_unwinders.end ());
  }

For frame_unwind_prepend_unwinder, you can insert the element at the
right place with insert, instead of doing the bubbling up:

  table.insert (table.begin () + osabi_unwinders_start_index, unwinder);

> @@ -81,12 +48,22 @@ get_frame_unwind_table (struct gdbarch *gdbarch)
>       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);
> +  table.push_back (&dwarf2_tailcall_frame_unwind);
> +  table.push_back (&inline_frame_unwind);
>  
>    /* The insertion point for OSABI sniffers.  */
> -  table->osabi_head = link;
> -  frame_unwind_data.set (gdbarch, table);
> +  osabi_start = table.size ();
> +}
> +
> +/* This function call retrieves the list of frame unwinders available in
> +   GDBARCH.  If this list is empty, it is initialized before being
> +   returned.  */

No need to say "This function call".  Just go with "Retrieve the list of
frame unwinders...".

>  void
>  frame_unwind_append_unwinder (struct gdbarch *gdbarch,
>  			      const struct frame_unwind *unwinder)
>  {
> -  struct frame_unwind_table *table = get_frame_unwind_table (gdbarch);
> -  struct frame_unwind_table_entry **ip;
> +  std::vector<const frame_unwind *>& table = get_frame_unwind_table (gdbarch);
>  
> -  /* Find the end of the list and insert the new entry there.  */
> -  for (ip = table->osabi_head; (*ip) != NULL; ip = &(*ip)->next);
> -  (*ip) = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind_table_entry);
> -  (*ip)->unwinder = unwinder;
> +  table.push_back (unwinder);

You could get rid of the `table` local variable here.

> @@ -205,8 +177,10 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>  				   unwinder_from_target))
>      return;
>  
> -  for (entry = table->list; entry != NULL; entry = entry->next)
> -    if (frame_unwind_try_unwinder (this_frame, this_cache, entry->unwinder))
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  std::vector<const frame_unwind *> table = get_frame_unwind_table (gdbarch);

Missing & to make `table` a reference?

> +  for (auto unwinder: table)

Space before colon.

> +    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
>        return;
>  
>    internal_error (_("frame_unwind_find_by_frame failed"));
> @@ -347,7 +321,7 @@ static void
>  maintenance_info_frame_unwinders (const char *args, int from_tty)
>  {
>    gdbarch *gdbarch = current_inferior ()->arch ();
> -  struct frame_unwind_table *table = get_frame_unwind_table (gdbarch);
> +  std::vector<const frame_unwind *> table = get_frame_unwind_table (gdbarch);

Missing &?

>  
>    ui_out *uiout = current_uiout;
>    ui_out_emit_table table_emitter (uiout, 2, -1, "FrameUnwinders");
> @@ -355,11 +329,10 @@ maintenance_info_frame_unwinders (const char *args, int from_tty)
>    uiout->table_header (25, ui_left, "type", "Type");
>    uiout->table_body ();
>  
> -  for (struct frame_unwind_table_entry *entry = table->list; entry != NULL;
> -       entry = entry->next)
> +  for (const struct frame_unwind *unwinder: table)

Let's use `auto` like you did above.  Space before colon.

>      {
> -      const char *name = entry->unwinder->name;
> -      const char *type = frame_type_str (entry->unwinder->type);
> +      const char *name = unwinder->name;
> +      const char *type = frame_type_str (unwinder->type);

I think you can get rid of the local vars.

> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 5175ef79e5b..17030e6e081 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -30,6 +30,7 @@
>  #include "displaced-stepping.h"
>  #include "gdbsupport/gdb-checked-static-cast.h"
>  #include "registry.h"
> +#include "frame-unwind.h"
>  
>  struct floatformat;
>  struct ui_file;
> @@ -310,6 +311,10 @@ extern obstack *gdbarch_obstack (gdbarch *arch);
>  
>  #define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE)   obstack_zalloc<TYPE> (gdbarch_obstack ((GDBARCH)))
>  
> +/* Return the vector of unwinders stored in a gdbarch object.  */
> +
> +std::vector<const frame_unwind*>& gdbarch_get_unwinder_list (struct gdbarch *arch);

We typically put the & on the right of the space (just like for
pointers).

I would suggest naming the function gdbarch_unwinder_list, avoiding
"get".

> +
>  /* Duplicate STRING, returning an equivalent string that's allocated on the
>     obstack associated with GDBARCH.  The string is freed when the corresponding
>     architecture is also freed.  */
> diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
> index 4b4db667ca4..bff66cc39d3 100755
> --- a/gdb/gdbarch.py
> +++ b/gdb/gdbarch.py
> @@ -125,6 +125,7 @@ with open("gdbarch.c", "w") as f:
>      print(file=f)
>      print("/* Maintain the struct gdbarch object.  */", file=f)
>      print(file=f)
> +    print("#include <vector>", file=f)

Print an empty line after the #include.

>      #
>      # The struct definition body.
>      #
> @@ -137,6 +138,8 @@ with open("gdbarch.c", "w") as f:
>      print("  auto_obstack obstack;", file=f)
>      print("  /* Registry.  */", file=f)
>      print("  registry<gdbarch> registry_fields;", file=f)
> +    print("  /* list of frame unwinders.  */", file=f)

list -> List

Print an empty line before the new field (there should also be one
before the "Registry" line...).

Simon