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

  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.
---
 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(-)
  

Hello Guinevere,

Just a couple of nits. Regardless:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

Guinevere Larsen <guinevere@redhat.com> writes:

> 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,
>

Nit: this empty line should be removed.

> -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;
>  };

> @@ -355,11 +327,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 (auto unwinder : table)
>      {
> -      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);

Did you try getting rid of the local vars here, as Simon suggested?
I agree with him that seem unnecessary.

>        ui_out_emit_list tuple_emitter (uiout, nullptr);
>        uiout->field_string ("name", name);
  

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
  

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

Guinevere> Seeing as a future patch of this series wants to refactor the
Guinevere> frame_unwind struct to use inheritance, and we'd like to not restrict
Guinevere> the future derived classes on what destructors are allowed. In
Guinevere> preparation for that change, this commit adds an std::vector to gdbarch
Guinevere> to store the unwinders in.

Using a vector is fine, but I still think that this approach
makes gdbarch mildly less opaque.  That is, now code anywhere can modify
the unwinder list in any way; whereas currently this is constrained to
the interface provided by frame-unwind.c.

Tom
  

Thanks for the review!

On 10/2/24 6:49 PM, Thiago Jung Bauermann wrote:
>> @@ -355,11 +327,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 (auto unwinder : table)
>>       {
>> -      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);
> Did you try getting rid of the local vars here, as Simon suggested?
> I agree with him that seem unnecessary.
I must have overlooked Simon's original suggestion. Considering he has 
approved with minor nits fixed, I'll send a follow up patch removing 
these, once this is merged.