[1/4] gdb: make gdbarch store a vector of frame unwinders
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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 constructors.
Seeing as a future patch of this series wants to refactor the
frame_unwind struct to use inheritance, obstack storage would no longer
be viable. 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 | 111 +++++++++++++++++----------------------------
gdb/gdbarch.c | 3 ++
gdb/gdbarch.h | 5 ++
gdb/gdbarch.py | 3 ++
5 files changed, 61 insertions(+), 69 deletions(-)
Comments
>>>>> Guinevere Larsen <blarsen@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 constructors.
> Seeing as a future patch of this series wants to refactor the
> frame_unwind struct to use inheritance, obstack storage would no longer
> be viable. 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'm not really sure about this patch.
Like on the one hand, it is fine. The arch is going to store the
unwinder table.
On the other hand, the registry system is there to let modules be kind
of independent. The lines are blurry though.
> +std::vector<const frame_unwind*>&
Missing spaces in here.
> -static const registry<gdbarch>::key<struct frame_unwind_table>
> - frame_unwind_data;
An alternative approach would be to just use a different type in here.
This can use the default destruction approach and then it's just
allocated with 'new'. So then you can use any old C++ type.
FWIW I think the real issue with obstack allocation isn't constructors
but destruction. See https://sourceware.org/pipermail/gdb-patches/2024-February/206888.html
Tom
On 08/03/2024 17:34, Tom Tromey wrote:
>>>>>> Guinevere Larsen <blarsen@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 constructors.
>> Seeing as a future patch of this series wants to refactor the
>> frame_unwind struct to use inheritance, obstack storage would no longer
>> be viable. 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'm not really sure about this patch.
>
> Like on the one hand, it is fine. The arch is going to store the
> unwinder table.
>
> On the other hand, the registry system is there to let modules be kind
> of independent. The lines are blurry though.
>
>> +std::vector<const frame_unwind*>&
> Missing spaces in here.
>
>> -static const registry<gdbarch>::key<struct frame_unwind_table>
>> - frame_unwind_data;
> An alternative approach would be to just use a different type in here.
>
> This can use the default destruction approach and then it's just
> allocated with 'new'. So then you can use any old C++ type.
I guess you're right, but would it make any real difference to have the
gdbarch store a vector out right, or to store a vector in an obfuscated way?
I looked back through the git log and the latest change that could have
added a significant change to how unwinders are stored could have
happened in early 2014, so I don't really think the registry would
meaningfully save in complexity.
But if you feel strongly that the registry is better, I can rework this
patch to work with that instead... Or just drop it. Once Andrew informed
of obstack_new, this patch felt unnecessary to the series as a whole,
just a related cleanup that I forgot to reword.
>
> FWIW I think the real issue with obstack allocation isn't constructors
> but destruction. See https://sourceware.org/pipermail/gdb-patches/2024-February/206888.html
I see what you mean (though I don't necessarily understand why). I would
prefer if we could not restrict the destructors, but since GDB doesn't
ever dealloc gdbarches to begin with, I don't think that would be a big
issue either way.
> I guess you're right, but would it make any real difference to have
> the gdbarch store a vector out right, or to store a vector in an
> obfuscated way?
I'm not really sure how I feel about it. In this case it's probably
harmless.
>> FWIW I think the real issue with obstack allocation isn't constructors
>> but destruction. See https://sourceware.org/pipermail/gdb-patches/2024-February/206888.html
> I see what you mean (though I don't necessarily understand why). I
> would prefer if we could not restrict the destructors, but since GDB
> doesn't ever dealloc gdbarches to begin with, I don't think that would
> be a big issue either way.
That's true for things that happen to be allocated on the gdbarch
obstack right now, but not other obstacks, and perhaps not in the future
if we ever decide a gdbarch should be deleted.
FWIW the issue with non-trivial destructors is that when the obstack
itself is destroyed, there's no mechanism for calling any of these.
This could introduce leaks or whatever else.
Tom
@@ -1219,6 +1219,14 @@ obstack *gdbarch_obstack (gdbarch *arch)
/* See gdbarch.h. */
+std::vector<const frame_unwind*>&
+gdbarch_get_unwinder_list (struct gdbarch *arch)
+{
+ return arch->unwinders;
+}
+
+/* See gdbarch.h. */
+
char *
gdbarch_obstack_strdup (struct gdbarch *arch, const char *string)
{
@@ -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;
-static const registry<gdbarch>::key<struct frame_unwind_table>
- 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 *
-get_frame_unwind_table (struct gdbarch *gdbarch)
+/* 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)
{
- 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);
+ table.push_back (&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
@@ -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. */
+static std::vector<const frame_unwind*>&
+get_frame_unwind_table (struct gdbarch *gdbarch)
+{
+ std::vector<const frame_unwind*>& table = gdbarch_get_unwinder_list (gdbarch);
+ if (table.size () == 0)
+ initialize_frame_unwind_table (table);
return table;
}
@@ -95,27 +72,25 @@ 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);
+
+ gdb_assert (osabi_start >= 0);
+
+ table.push_back (unwinder);
+
+ /* Bubble the new entry up to where new unwinders are allowed to be
+ inserterd. */
+ for (int i = table.size() - 1; i > osabi_start; i--)
+ std::swap(table[i], table[i-1]);
}
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);
}
/* Call SNIFFER from UNWINDER. If it succeeded set UNWINDER for
@@ -188,9 +163,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 +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);
+ for (auto unwinder: table)
+ 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);
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)
{
- 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);
ui_out_emit_list tuple_emitter (uiout, nullptr);
uiout->field_string ("name", name);
@@ -27,6 +27,7 @@
/* Maintain the struct gdbarch object. */
+#include <vector>
struct gdbarch
{
/* Has this architecture been fully initialized? */
@@ -36,6 +37,8 @@ struct gdbarch
auto_obstack obstack;
/* Registry. */
registry<gdbarch> registry_fields;
+ /* list of frame unwinders. */
+ std::vector<const frame_unwind *> unwinders;
/* basic architectural information. */
const struct bfd_arch_info * bfd_arch_info;
@@ -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);
+
/* 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. */
@@ -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)
#
# 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)
+ print(" std::vector<const frame_unwind *> unwinders;", file=f)
print(file=f)
print(" /* basic architectural information. */", file=f)
for c in filter(info, components):