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

[Simon Marchi <simark@simark.ca>]

On 2024-10-08 14:22, Guinevere Larsen wrote:
> 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[] =

I think we don't want a space after the `*`.

>  {
> -  {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;

I'd suggest using static_cast instead of a C-style cast, since we use
C++.  Otherwise this diff LGTM.

> }
>    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,
>  };

I thought I made the suggestion to use "enum class" for new stuff, but
now I can't find it.

>> 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 (: )

Yes, I'm fine with you addressing these comments without a further
review (although I guess it would make sense to wait until the whole
series is approved before pushing, in case this needs to change):

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon