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

[Guinevere Larsen <blarsen@redhat.com>]

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
>