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

[Guinevere Larsen <guinevere@redhat.com>]

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 <map>
>> +
>> +/* 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
>