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

Simon Marchi
2024-10-08T18:37:59Z

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