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

  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 changes the registry in gdbarch
to instead store an std::vector, which doesn't require using an obstack
and doesn't rely on a linked list.

There should be no user-visible changes.

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
 gdb/frame-unwind.c | 107 +++++++++++++++------------------------------
 1 file changed, 36 insertions(+), 71 deletions(-)
  

Guinevere Larsen <guinevere@redhat.com> writes:

> 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 changes the registry in gdbarch
> to instead store an std::vector, which doesn't require using an obstack
> and doesn't rely on a linked list.
>
> There should be no user-visible changes.
>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  gdb/frame-unwind.c | 107 +++++++++++++++------------------------------
>  1 file changed, 36 insertions(+), 71 deletions(-)
>
> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index 352779fcdcc..e61f6244913 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -31,61 +31,42 @@
>  #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 =

I'm not a huge fan of `auto` when the type is well known.  My thinking
is write once, read many times.  So I'd rather have the type information
available when I read the code.  For me:

  static constexpr std::initializer_list<const frame_unwind *> standard_unwinders =

tells me what's going on...

>  {
> -  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;
> -};
> +/* 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 ();
>  
> -static const registry<gdbarch>::key<struct frame_unwind_table>
> +static const registry<gdbarch>::key<std::vector<const frame_unwind *>>
>       frame_unwind_data;
>  
> -/* 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)
> -{
> -  *link = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry);
> -  (*link)->unwinder = unwinder;
> -  return &(*link)->next;
> -}
> -
> -static struct frame_unwind_table *
> +/* Retrieve the list of frame unwinders available in GDBARCH.
> +   If this list is empty, it is initialized before being returned.  */
> +static std::vector<const frame_unwind *> *
>  get_frame_unwind_table (struct gdbarch *gdbarch)

Given you're changing this code anyway, how about returning a reference
rather than a pointer, i.e.:

  static std::vector<const frame_unwind *> &
  get_frame_unwind_table (struct gdbarch *gdbarch)
  { ... }

The users of get_frame_unwind_table will need to be updated to match.

>  {
> -  struct frame_unwind_table *table = frame_unwind_data.get (gdbarch);
> +  std::vector<const frame_unwind *> *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;
> +  table = new std::vector<const frame_unwind *>;
> +  table->insert (table->begin (), standard_unwinders.begin (),
> +		 standard_unwinders.end ());
>  
> -  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);
>  
>    return table;
> @@ -95,27 +76,16 @@ void
>  frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
>  				const struct frame_unwind *unwinder)
>  {
> -  struct frame_unwind_table *table = get_frame_unwind_table (gdbarch);
> -  struct frame_unwind_table_entry *entry;
> -
> -  /* Insert the new entry at the start of the list.  */
> -  entry = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind_table_entry);
> -  entry->unwinder = unwinder;
> -  entry->next = (*table->osabi_head);
> -  (*table->osabi_head) = entry;
> +  std::vector<const frame_unwind *> *table = get_frame_unwind_table (gdbarch);
> +
> +  table->insert (table->begin () + prepend_unwinder_index, unwinder);
>  }
>  
>  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;
> -
> -  /* 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;
> +  get_frame_unwind_table (gdbarch)->push_back (unwinder);
>  }
>  
>  /* Call SNIFFER from UNWINDER.  If it succeeded set UNWINDER for
> @@ -188,9 +158,6 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>    FRAME_SCOPED_DEBUG_ENTER_EXIT;
>    frame_debug_printf ("this_frame=%d", frame_relative_level (this_frame));
>  
> -  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> -  struct frame_unwind_table *table = get_frame_unwind_table (gdbarch);
> -  struct frame_unwind_table_entry *entry;
>    const struct frame_unwind *unwinder_from_target;
>  
>    unwinder_from_target = target_get_unwinder ();
> @@ -205,8 +172,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);
> +  for (auto unwinder : *table)

I think you can use:

  for (const auto &unwinder : *table)

here.  The 'const' is just good style as you're not going to modify it.
The '&' is not really necessary as the type is actually a pointer.
Better still might be:

  for (const frame_unwind *unwinder : *table)

But I don't mind the 'auto' here too much as the type is on the line
immediately above.  But I do think, if you're sticking with 'auto' here
then throwing the '&' in is a good idea.

> +    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
>        return;
>  
>    internal_error (_("frame_unwind_find_by_frame failed"));
> @@ -347,7 +316,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);
>  
>    ui_out *uiout = current_uiout;
>    ui_out_emit_table table_emitter (uiout, 2, -1, "FrameUnwinders");
> @@ -355,15 +324,11 @@ 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)

See the comments above.

If you're happy making this changes I suggest then:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>      {
> -      const char *name = entry->unwinder->name;
> -      const char *type = frame_type_str (entry->unwinder->type);
> -
>        ui_out_emit_list tuple_emitter (uiout, nullptr);
> -      uiout->field_string ("name", name);
> -      uiout->field_string ("type", type);
> +      uiout->field_string ("name", unwinder->name);
> +      uiout->field_string ("type", frame_type_str (unwinder->type));
>        uiout->text ("\n");
>      }
>  }
> -- 
> 2.47.0
  

On 1/14/25 11:28 AM, Andrew Burgess wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> 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 changes the registry in gdbarch
>> to instead store an std::vector, which doesn't require using an obstack
>> and doesn't rely on a linked list.
>>
>> There should be no user-visible changes.
>>
>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> ---
>>   gdb/frame-unwind.c | 107 +++++++++++++++------------------------------
>>   1 file changed, 36 insertions(+), 71 deletions(-)
>>
>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>> index 352779fcdcc..e61f6244913 100644
>> --- a/gdb/frame-unwind.c
>> +++ b/gdb/frame-unwind.c
>> @@ -31,61 +31,42 @@
>>   #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 =
> I'm not a huge fan of `auto` when the type is well known.  My thinking
> is write once, read many times.  So I'd rather have the type information
> available when I read the code.  For me:
>
>    static constexpr std::initializer_list<const frame_unwind *> standard_unwinders =
>
> tells me what's going on...
>
>>   {
>> -  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;
>> -};
>> +/* 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 ();
>>   
>> -static const registry<gdbarch>::key<struct frame_unwind_table>
>> +static const registry<gdbarch>::key<std::vector<const frame_unwind *>>
>>        frame_unwind_data;
>>   
>> -/* 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)
>> -{
>> -  *link = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry);
>> -  (*link)->unwinder = unwinder;
>> -  return &(*link)->next;
>> -}
>> -
>> -static struct frame_unwind_table *
>> +/* Retrieve the list of frame unwinders available in GDBARCH.
>> +   If this list is empty, it is initialized before being returned.  */
>> +static std::vector<const frame_unwind *> *
>>   get_frame_unwind_table (struct gdbarch *gdbarch)
> Given you're changing this code anyway, how about returning a reference
> rather than a pointer, i.e.:
>
>    static std::vector<const frame_unwind *> &
>    get_frame_unwind_table (struct gdbarch *gdbarch)
>    { ... }
>
> The users of get_frame_unwind_table will need to be updated to match.
>
>>   {
>> -  struct frame_unwind_table *table = frame_unwind_data.get (gdbarch);
>> +  std::vector<const frame_unwind *> *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;
>> +  table = new std::vector<const frame_unwind *>;
>> +  table->insert (table->begin (), standard_unwinders.begin (),
>> +		 standard_unwinders.end ());
>>   
>> -  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);
>>   
>>     return table;
>> @@ -95,27 +76,16 @@ void
>>   frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
>>   				const struct frame_unwind *unwinder)
>>   {
>> -  struct frame_unwind_table *table = get_frame_unwind_table (gdbarch);
>> -  struct frame_unwind_table_entry *entry;
>> -
>> -  /* Insert the new entry at the start of the list.  */
>> -  entry = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind_table_entry);
>> -  entry->unwinder = unwinder;
>> -  entry->next = (*table->osabi_head);
>> -  (*table->osabi_head) = entry;
>> +  std::vector<const frame_unwind *> *table = get_frame_unwind_table (gdbarch);
>> +
>> +  table->insert (table->begin () + prepend_unwinder_index, unwinder);
>>   }
>>   
>>   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;
>> -
>> -  /* 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;
>> +  get_frame_unwind_table (gdbarch)->push_back (unwinder);
>>   }
>>   
>>   /* Call SNIFFER from UNWINDER.  If it succeeded set UNWINDER for
>> @@ -188,9 +158,6 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>>     FRAME_SCOPED_DEBUG_ENTER_EXIT;
>>     frame_debug_printf ("this_frame=%d", frame_relative_level (this_frame));
>>   
>> -  struct gdbarch *gdbarch = get_frame_arch (this_frame);
>> -  struct frame_unwind_table *table = get_frame_unwind_table (gdbarch);
>> -  struct frame_unwind_table_entry *entry;
>>     const struct frame_unwind *unwinder_from_target;
>>   
>>     unwinder_from_target = target_get_unwinder ();
>> @@ -205,8 +172,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);
>> +  for (auto unwinder : *table)
> I think you can use:
>
>    for (const auto &unwinder : *table)
>
> here.  The 'const' is just good style as you're not going to modify it.
> The '&' is not really necessary as the type is actually a pointer.
> Better still might be:
>
>    for (const frame_unwind *unwinder : *table)
>
> But I don't mind the 'auto' here too much as the type is on the line
> immediately above.  But I do think, if you're sticking with 'auto' here
> then throwing the '&' in is a good idea.
I decided to go with the "auto" version, since table is always declared 
close to the usage, and that doesn't use auto.
>
>> +    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
>>         return;
>>   
>>     internal_error (_("frame_unwind_find_by_frame failed"));
>> @@ -347,7 +316,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);
>>   
>>     ui_out *uiout = current_uiout;
>>     ui_out_emit_table table_emitter (uiout, 2, -1, "FrameUnwinders");
>> @@ -355,15 +324,11 @@ 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)
> See the comments above.
>
> If you're happy making this changes I suggest then:
>
> Approved-By: Andrew Burgess <aburgess@redhat.com>
Thanks for the review, I went along with the changes and added your 
approval!