Re: [PATCH v5 2/5] gdb: add "unwinder class" to frame unwinders

Guinevere Larsen
2024-10-08T18:22:33Z

On 10/3/24 3:46 PM, Simon Marchi wrote:
> On 10/1/24 2:42 PM, Guinevere Larsen wrote:
>> From: Guinevere Larsen <blarsen@redhat.com>
>>
>> A future patch will add a way to disable certain unwinders based on
>> different characteristics. This patch aims to make it more convenient
>> to disable related unwinders in bulk, such as architecture specific
>> ones, by indentifying all unwinders by which part of the code adds it.
> indentifying -> identifying
>
>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>> index b69ae8596a2..afc1258c6a9 100644
>> --- a/gdb/frame-unwind.c
>> +++ b/gdb/frame-unwind.c
>> @@ -30,6 +30,17 @@
>> #include "dwarf2/frame-tailcall.h"
>> #include "cli/cli-cmds.h"
>> #include "inferior.h"
>> +#include
>> +
>> +/* Conversion list between the enum for frame_unwind_class and
>> + string. */
>> +static std::map<enum frame_unwind_class, const char *> unwind_class_conversion =
>> +{
>> + {FRAME_UNWIND_GDB, "GDB"},
>> + {FRAME_UNWIND_ARCH, "ARCH"},
>> + {FRAME_UNWIND_EXTENSION, "EXTENSION"},
>> + {FRAME_UNWIND_DEBUGINFO, "DEBUGINFO"},
>> +};
> It's not reaaally a big deal, but using an std::map for this is kind of
> heavyweight. It could probably be a simple array, still indexed by the
> enum value. Personally, I used functions with switches to handle cases
> this (see target_waitkind_str), because compilers can warn you if you
> forget to handle an enumerator (which can easily happen when a new one
> is added).

Since I have 2 functions, one to convert from enum to string, and
another from string to enum, I'll go with a simple array that makes it
easy to do both.

This is my proposed change:

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index ef05b658f94..10d9f2e9f88 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -34,12 +34,12 @@

/* Conversion list between the enum for frame_unwind_class and
string. */
-static std::map<enum frame_unwind_class, const char *>
unwind_class_conversion =
+static const char * unwind_class_conversion[] =
{
-  {FRAME_UNWIND_GDB, "GDB"},
-  {FRAME_UNWIND_ARCH, "ARCH"},
-  {FRAME_UNWIND_EXTENSION, "EXTENSION"},
-  {FRAME_UNWIND_DEBUGINFO, "DEBUGINFO"},
+ "GDB",
+ "ARCH",
+ "EXTENSION",
+ "DEBUGINFO",
};

/* Default sniffers, that must always be the first in the unwinder list,
@@ -88,9 +88,8 @@ get_frame_unwind_table (struct gdbarch *gdbarch)
static const char *
frame_unwinder_class_str (frame_unwind_class uclass)
{
-  auto location = unwind_class_conversion.find (uclass);
-  gdb_assert (location != unwind_class_conversion.end ());
-  return location->second;
+  gdb_assert (uclass < UNWIND_CLASS_NUMBER);
+  return unwind_class_conversion[uclass];
}

/* Case insensitive search for a frame_unwind_class based on the given
@@ -102,10 +101,10 @@ str_to_frame_unwind_class (const char *class_str)
/* Skip the prefix if present. */
if (strncasecmp (class_str, prefix, strlen(prefix)) == 0)
class_str += strlen (prefix);
-  for (const auto &it : unwind_class_conversion)
+  for (int i = 0; i < UNWIND_CLASS_NUMBER; i++)
{
-      if (strcasecmp (it.second, class_str) == 0)
-       return it.first;
+      if (strcasecmp (unwind_class_conversion[i], class_str) == 0)
+       return (frame_unwind_class) i;
}
error (_("Unknown frame unwind class: %s"), class_str);
}
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index f5ea8d9f690..555a5d98d5b 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -167,6 +167,8 @@ enum frame_unwind_class
FRAME_UNWIND_DEBUGINFO,
/* The unwinder was created and handles target dependent things.  */
FRAME_UNWIND_ARCH,
+  /* Meta enum value, to ensure we're always sent a valid unwinder
class.  */
+ UNWIND_CLASS_NUMBER,
};

>
> Otherwise, a more medium-term but clean solution would be to integrate
> one of these libs in GDB:
>
> https://github.com/Neargye/magic_enum
> https://github.com/quicknir/wise_enum
>
> I use the latter in another project, and it works nicely.
>
> Otherwise, LGTM:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
Can I add the tag with that change? (just making sure you're ok with my
solution (: )

--
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Simon
>