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

[Simon Marchi <simark@simark.ca>]

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.

> 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.

OTOH, I don't think accepting user inputs FRAME_UNWIND_ is really
necessary.

> +    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.


> +  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.

> @@ -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.

> +  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.

> @@ -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?

Simon