Re: [PATCH v5 4/5] gdb: introduce ability to disable frame unwinders

[Guinevere Larsen <guinevere@redhat.com>]

On 10/5/24 11:51 PM, Simon Marchi wrote:
>
> On 2024-10-01 14:42, Guinevere Larsen wrote:
>> From: Guinevere Larsen <blarsen@redhat.com>
>>
>> Sometimes, in the GDB testsuite, we want to test the ability of specific
>> unwinders to handle some piece of code. Usually this is done by trying
>> to outsmart GDB, or by coercing the compiler to remove information that
>> GDB would rely on.  Both approaches have problems as GDB gets smarter
>> with time, and that compilers might differ in version and behavior, or
>> simply introduce new useful information. This was requested back in 2003
>> in PR backtrace/8434.
>>
>> To improve our ability to thoroughly test GDB, this patch introduces a
>> new maintenance command that allows a user to disable some unwinders,
>> based on either the name of the unwinder or on its class. With this
>> change, it will now be possible for GDB to not find any frame unwinders
>> for a given frame, which would previously cause GDB to assert. GDB will
>> now check if any frame unwinder has been disabled, and if some has, it
>> will just error out instead of asserting.
>>
>> Unwinders can be disabled or re-enabled in 3 different ways:
>> * Disabling/enabling all at once (using '-all').
>> * By specifying an unwinder class to be disabled (option '-class').
>> * By specifying the name of an unwinder (option '-name').
>>
>> If you give no options to the command, GDB assumes the input is an
>> unwinder class. '-class' would make no difference if used, is just here
>> for completeness.
> Heh, it's funny, to me the natural way would have been to make -name the
> default case.
Heh, I guess thinking about the next test where I want to disable all 
debuginfo unwinders and it just felt like the obvious choice.
>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>> index 7be01ec2203..f66f233c781 100644
>> --- a/gdb/frame-unwind.c
>> +++ b/gdb/frame-unwind.c
>> @@ -94,6 +94,23 @@ frame_unwinder_class_str (frame_unwind_class uclass)
>>     return location->second;
>>   }
>>   
>> +/* Case insensitive search for a frame_unwind_class based on the given
>> +   string.  */
>> +static enum frame_unwind_class
>> +str_to_frame_unwind_class (const char *class_str)
>> +{
>> +  const char *prefix = "FRAME_UNWIND_";
>> +  /* Skip the prefix if present.  */
>> +  if (strncasecmp (class_str, prefix, strlen(prefix)) == 0)
> Missing space.
Fixed
> OTOH, I don't think accepting user inputs FRAME_UNWIND_ is really
> necessary.
That's a fair question, but I don't think this is too costly (for 
performance, or maintenance) to keep it, in case someone reads the code 
instead of documentation (not that I do that or anything o.o'). If you 
disagree, I am fine with removing it honestly, especially with 
documenting the classes that you suggested later.
>
>> +    class_str += strlen (prefix);
>> +  for (const auto &it : unwind_class_conversion)
>> +    {
>> +      if (strcasecmp (it.second, class_str) == 0)
>> +	return it.first;
>> +    }
> I would suggest spacing things out a bit, addking blank lines after the
> declaration of `prefix`, after the if and after the for.  I have a hard
> time following things when they're all packed.
done
>
>
>> +  error (_("Unknown frame unwind class: %s"), class_str);
>> +}
>> +
>>   void
>>   frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
>>   				const struct frame_unwind *unwinder)
>> @@ -182,25 +199,47 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>>   
>>     const struct frame_unwind *unwinder_from_target;
>>   
>> +  /* If we see a disabled unwinder, we assume some test is being run on
>> +     GDB, and we don't want to assert at the end of this function.  */
>> +  bool seen_disabled_unwinder = false;
>> +
>>     unwinder_from_target = target_get_unwinder ();
>>     if (unwinder_from_target != NULL
>> +      && unwinder_from_target->enabled ()
>>         && frame_unwind_try_unwinder (this_frame, this_cache,
>>   				   unwinder_from_target))
>>       return;
>> +  else if (unwinder_from_target != nullptr
>> +	   && !unwinder_from_target->enabled ())
>> +    seen_disabled_unwinder = true;
> There would be less repetition like this:
>
>    if (unwinder_from_target != nullptr)
>      {
>        if (unwinder_from_target->enabled ())
> 	{
> 	  if (frame_unwind_try_unwinder (this_frame, this_cache,
> 					 unwinder_from_target))
> 	    return;
> 	}
>        else
> 	seen_disabled_unwinder = true;
>      }
>
> You could also factor out this logic to a lambda and use it at the
> multiple spots in the function where this is done.
Good idea. I factored into a lambda that receives the unwinder as a 
parameter, so I can use it every time frame_unwind_try_unwinder.
>
>> @@ -405,15 +445,97 @@ maintenance_info_frame_unwinders (const char *args, int from_tty)
>>         const char *name = unwinder->name ();
>>         const char *type = frame_type_str (unwinder->type ());
>>         const char *uclass = frame_unwinder_class_str (unwinder->unwinder_class ());
>> +      const char *enabled = unwinder->enabled () ? "Y" : "N";
>>   
>>         ui_out_emit_list tuple_emitter (uiout, nullptr);
>>         uiout->field_string ("name", name);
>>         uiout->field_string ("type", type);
>>         uiout->field_string ("class", uclass);
>> +      uiout->field_string ("enabled", enabled);
>>         uiout->text ("\n");
>>       }
>>   }
>>   
>> +/* 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 ();
>> +  if (args == nullptr)
>> +    {
>> +      if (enable)
>> +	error (_("specify which frame unwinder(s) should be enabled"));
>> +      else
>> +	error (_("specify which frame unwinder(s) should be disabled"));
>> +    }
> Should the error messages be capitalized?
>
> Not a big deal, but I would put the reinit_frame_cache a bit later, when
> the unwinder list actually gets modified.  If we error out before, we
> technically don't need to reinit the frame cache.
Fixed both.
>
>> +  struct gdbarch* gdbarch = current_inferior ()->arch ();
>> +  std::vector<const frame_unwind *> unwinder_list
>> +    = gdbarch_unwinder_list (gdbarch);
>> +
>> +  /* 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);
>> +      return;
>> +    }
> Did you consider using the gdb::option framework for this (found in
> cli/cli-option.h)?  It's been a while since I touched it, I don't recall
> if it would be a good fit for this, but it would be worth checking.

I hadn't looked into using this framework when I first implemented it. 
However, looking at it now, it looks like overkill for what I need. The 
gdb::option path need the option array (with all the stuff that goes 
along with it), to end up with a very similar setup in the function itself.

Now, if you think I'm overselling the complexity or that we are likely 
to add more options to this command, I can add it for the next version.

>
>> @@ -425,4 +547,37 @@ _initialize_frame_unwind ()
>>   	   _("List the frame unwinders currently in effect, "
>>   	     "starting with the highest priority."),
>>   	   &maintenanceinfolist);
>> +
>> +  /* Add "maint frame-unwinder disable/enable".  */
>> +  static struct cmd_list_element *maint_frame_unwinder;
>> +
>> +  add_basic_prefix_cmd ("frame-unwinder", class_maintenance,
>> +			_("Commands handling frame unwinders."),
>> +			&maint_frame_unwinder, 0, &maintenancelist);
>> +
>> +  add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders,
>> +	   _("\
>> +Disable one or more frame unwinder(s).\n\
>> +Usage: maint frame-unwinder disable [-all | -name NAME | [-class] CLASS]\n\
>> +\n\
>> +These are the meanings of the options:\n\
>> +\t-all    - All available unwinders will be disabled\n\
>> +\t-name   - NAME is the exact name of the frame unwinder to be disabled\n\
>> +\t-class  - CLASS is the class of unwinders to be disabled.\n\
> Should the help maybe list the valid classes?
Makes sense, fixed!
>
> Simon
>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers