Re: [PATCH 4/4] GDB: introduce ability to disable frame unwinders

Guinevere Larsen
2024-03-11T14:10:19Z

On 08/03/2024 18:22, Tom Tromey wrote:
>>>>>> Guinevere Larsen <blarsen@redhat.com> writes:
>
>> +static enum frame_unwind_class
>> +str_to_frame_unwind_class (const char **c_str)
> Comment.
>
>> +{
>> + std::string full_name = "FRAME_UNWIND_";
>> + const int start_length = full_name.length ();
>> + if (strncasecmp (*c_str, full_name.c_str (), start_length) == 0)
>> + full_name = *c_str;
> In an earlier patch I was wondering how useful those FRAME_UNWIND_
> strings were for the maint output. Maybe just using human names here
> would be overall better.
>
> Then this could just call streq.

I don't want to call streq because I want case insensitive matching, but
I will drop the FRAME_UNWIND_ prefix from printing.

>
> Anyway if you want to keep this approach, it's weird to allocate a
> string and then use the C API. Perhaps a string_view would be better.
I used the string because at that moment it felt safer than dealing with
strcat, and originally I was going to use the operator equals rather
than strcasecmp. I'll drop it
>
>
>> +/* Helper function to both enable and disable frame unwinders.
>> + if ENABLE is true, this call will be enabling unwinders,
>> + otherwise the unwinders will be disabled. */
>> +static void
>> +enable_disable_frame_unwinders (const char *args, int from_tty, bool enable)
>> +{
>> +
>> + reinit_frame_cache ();
> Stray blank line.
>
>> + if (args == nullptr)
>> + {
>> + error (_("specify which frame unwinder(s) should be %s"),
>> + (enable)? "enabled" : "disabled");
>> + }
> No braces.
>
>> + /* First see if the user wants to change all unwinders. */
>> + if (check_for_argument (&args, "-all"))
>> + {
>> + for (const frame_unwind *u : unwinder_list)
>> + {
>> + u->set_enabled (enable);
>> + }
> This also looks over-braced.
>
>> + add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders,
>> + _("\
>> +Disable one or more frame unwinder(s).\n\
>> +Usage: maint frame-unwinder disable [OPTION] UNWINDER\n\
>> +\n\
>> +The meaning of UNWINDER depends on the OPTION given. These are the possibilities:\n\
>> +\t-all - UNWINDER is ignored. All available unwinders will be disabled\n\
>> +\t-name - UNWINDER is the exact name of the frame unwinder is to be disabled\n\
>> +\t-class - UNWINDER is the class of unwinders to be disabled.\n\
> I guess I'd write that more like
>
> disable [-all | -name UNWINDER | -class NAME]
>
> or something like that, rather than spelling out that UNWINDER is
> ignored in one case.
That sounds like a much better solution to what I came up with hahaha.
Thank you!
>
>
> I think there's probably a bug in bugzilla about disabling unwinders, so
> this should probably have a Bug: trailer.
Is there? :o I'll go looking for it.

--
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Tom
>