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

Simon Marchi
2024-10-03T18:34:00Z

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