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

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <guinevere@redhat.com> writes:

> 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.
>
> This command is meant to be used once the inferior is already at the
> desired location for the test. An example session would be:
>
> (gdb) start
> Temporary breakpoint 1, main () at omp.c:17
> 17          func();
> (gdb) maint frame-unwinder disable ARCH
> (gdb) bt
> \#0  main () at omp.c:17
> (gdb) maint frame-unwinder enable ARCH
> (gdb) cont
> Continuing.
>
> This commit is a more generic version of commit 3c3bb0580be0,
> and so, based on the final paragraph of the commit message:
>     gdb: Add switch to disable DWARF stack unwinders
> <...>
>     If in the future we find ourselves adding more switches to disable
>     different unwinders, then we should probably move to a more generic
>     solution, and remove this patch.
> this patch also reverts 3c3bb0580be0
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=8434
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  gdb/NEWS                                      |  17 ++
>  gdb/doc/gdb.texinfo                           |  49 ++--
>  gdb/dwarf2/frame-tailcall.c                   |   3 -
>  gdb/dwarf2/frame.c                            |  30 ---
>  gdb/dwarf2/frame.h                            |   6 -
>  gdb/frame-unwind.c                            | 237 +++++++++++++++++-
>  gdb/frame-unwind.h                            |   9 +
>  .../gdb.base/frame-info-consistent.exp        |   8 +-
>  gdb/testsuite/gdb.base/frame-unwind-disable.c |  22 ++
>  .../gdb.base/frame-unwind-disable.exp         | 137 ++++++++++
>  gdb/testsuite/gdb.base/maint.exp              |   4 -
>  11 files changed, 437 insertions(+), 85 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c
>  create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 245b355669a..ef3317c11cb 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -125,6 +125,13 @@ maintenance info blocks [ADDRESS]
>    are listed starting at the inner global block out to the most inner
>    block.
>  
> +maintenance frame-unwinder disable [-all | -name NAME | [-class] CLASS]
> +maintenance frame-unwinder enable [-all | -name NAME | [-class] CLASS]
> +  Enable or disable frame unwinders.  This is only meant to be used when
> +  testing unwinders themselves, and you want to ensure that a fallback
> +  algorithm won't obscure a regression.  GDB is not expected to behave
> +  well if you try to execute the inferior with unwinders disabled.
> +
>  info missing-objfile-handlers
>    List all the registered missing-objfile handlers.
>  
> @@ -167,6 +174,16 @@ maintenance print remote-registers
>  mainenance info frame-unwinders
>    Add a CLASS column to the output.  This class is a somewhat arbitrary
>    grouping of unwinders, based on which area of GDB adds the unwinder.
> +  Also add an ENABLED column, that will show if the unwinder is enabled
> +  or not.
> +
> +maintenance set dwarf unwinders (on|off)
> +  This command has been removed because the same functionality can be
> +  achieved with maint frame-unwinder (enable|disable) DEBUGINFO.
> +
> +maintenance show dwarf unwinders
> +  This command has been removed since the functionality can be achieved
> +  by checking the last column of maint info frame-unwinders.
>  
>  show configuration
>    Now includes the version of GNU Readline library that GDB is using.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 115c1f46b7f..10e6654a03b 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42210,31 +42210,6 @@ On hosts without threading, or where worker threads have been disabled
>  at runtime, this setting has no effect, as DWARF reading is always
>  done on the main thread, and is therefore always synchronous.
>  
> -@kindex maint set dwarf unwinders
> -@kindex maint show dwarf unwinders
> -@item maint set dwarf unwinders
> -@itemx maint show dwarf unwinders
> -Control use of the DWARF frame unwinders.
> -
> -@cindex DWARF frame unwinders
> -Many targets that support DWARF debugging use @value{GDBN}'s DWARF
> -frame unwinders to build the backtrace.  Many of these targets will
> -also have a second mechanism for building the backtrace for use in
> -cases where DWARF information is not available, this second mechanism
> -is often an analysis of a function's prologue.
> -
> -In order to extend testing coverage of the second level stack
> -unwinding mechanisms it is helpful to be able to disable the DWARF
> -stack unwinders, this can be done with this switch.
> -
> -In normal use of @value{GDBN} disabling the DWARF unwinders is not
> -advisable, there are cases that are better handled through DWARF than
> -prologue analysis, and the debug experience is likely to be better
> -with the DWARF frame unwinders enabled.
> -
> -If DWARF frame unwinders are not supported for a particular target
> -architecture, then enabling this flag does not cause them to be used.
> -
>  @kindex maint info frame-unwinders
>  @item maint info frame-unwinders
>  List the frame unwinders currently in effect, starting with the highest
> @@ -42252,6 +42227,30 @@ Unwinders installed by debug information readers.
>  Unwinders installed by the architecture specific code.
>  @end table
>  
> +@kindex maint frame-unwinder disable
> +@kindex maint frame-unwinder enable
> +@item maint frame-unwinder disable [@code{-all} | @code{-name} @var{name} | [@code{-class}] @var{class}]
> +@item maint frame-unwinder enable [@code{-all} | @code{-name} @var{name} | [@code{-class}] @var{class}]
> +Disable or enable frame unwinders.  This is meant only as a testing tool, and
> +@value{GDBN} is not guaranteed to work properly if it is unable to find
> +valid frame unwinders.
> +
> +The meaning of each of the invocations are as follows:
> +
> +@table @samp
> +@item @code{-all}
> +Disable or enable all unwinders.
> +@item @code{-name} @var{name}
> +@var{name} is the exact name of the unwinder to be disabled or enabled.
> +@item [@code{-class}] @var{class}
> +@var{class} is the class of frame unwinders to be disabled or enabled.
> +The class may include the prefix @code{FRAME_UNWINDER_}, but it is not
> +required.
> +@end table
> +
> +@var{name} and @var{class} are always case insensitive.  If no option
> +starting wth @code{-} is given, @value{GDBN} assumes @code{-class}.
> +
>  @kindex maint set worker-threads
>  @kindex maint show worker-threads
>  @item maint set worker-threads
> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> index 54813d00b03..2d7ab740f62 100644
> --- a/gdb/dwarf2/frame-tailcall.c
> +++ b/gdb/dwarf2/frame-tailcall.c
> @@ -321,9 +321,6 @@ tailcall_frame_sniffer (const struct frame_unwind *self,
>    int next_levels;
>    struct tailcall_cache *cache;
>  
> -  if (!dwarf2_frame_unwinders_enabled_p)
> -    return 0;
> -
>    /* Inner tail call element does not make sense for a sentinel frame.  */
>    next_frame = get_next_frame (this_frame);
>    if (next_frame == NULL)
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index 85e1d59bc09..e0e8eb5dbec 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -183,9 +183,6 @@ static ULONGEST read_encoded_value (struct comp_unit *unit, gdb_byte encoding,
>  				    unrelocated_addr func_base);
>  \f
>  
> -/* See dwarf2/frame.h.  */
> -bool dwarf2_frame_unwinders_enabled_p = true;
> -
>  /* Store the length the expression for the CFA in the `cfa_reg' field,
>     which is unused in that case.  */
>  #define cfa_exp_len cfa_reg
> @@ -1306,9 +1303,6 @@ static int
>  dwarf2_frame_sniffer (const struct frame_unwind *self,
>  		      const frame_info_ptr &this_frame, void **this_cache)
>  {
> -  if (!dwarf2_frame_unwinders_enabled_p)
> -    return 0;
> -
>    /* Grab an address that is guaranteed to reside somewhere within the
>       function.  get_frame_pc(), with a no-return next function, can
>       end up returning something past the end of this function's body.
> @@ -2253,34 +2247,10 @@ dwarf2_build_frame_info (struct objfile *objfile)
>    set_comp_unit (objfile, unit.release ());
>  }
>  
> -/* Handle 'maintenance show dwarf unwinders'.  */
> -
> -static void
> -show_dwarf_unwinders_enabled_p (struct ui_file *file, int from_tty,
> -				struct cmd_list_element *c,
> -				const char *value)
> -{
> -  gdb_printf (file,
> -	      _("The DWARF stack unwinders are currently %s.\n"),
> -	      value);
> -}
> -
>  void _initialize_dwarf2_frame ();
>  void
>  _initialize_dwarf2_frame ()
>  {
> -  add_setshow_boolean_cmd ("unwinders", class_obscure,
> -			   &dwarf2_frame_unwinders_enabled_p , _("\
> -Set whether the DWARF stack frame unwinders are used."), _("\
> -Show whether the DWARF stack frame unwinders are used."), _("\
> -When enabled the DWARF stack frame unwinders can be used for architectures\n\
> -that support the DWARF unwinders.  Enabling the DWARF unwinders for an\n\
> -architecture that doesn't support them will have no effect."),
> -			   NULL,
> -			   show_dwarf_unwinders_enabled_p,
> -			   &set_dwarf_cmdlist,
> -			   &show_dwarf_cmdlist);
> -
>  #if GDB_SELF_TEST
>    selftests::register_test_foreach_arch ("execute_cfa_program",
>  					 selftests::execute_cfa_program_test);
> diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h
> index 2167310fbdf..fe2d669a983 100644
> --- a/gdb/dwarf2/frame.h
> +++ b/gdb/dwarf2/frame.h
> @@ -198,12 +198,6 @@ struct dwarf2_frame_state
>    bool armcc_cfa_offsets_reversed = false;
>  };
>  
> -/* When this is true the DWARF frame unwinders can be used if they are
> -   registered with the gdbarch.  Not all architectures can or do use the
> -   DWARF unwinders.  Setting this to true on a target that does not
> -   otherwise support the DWARF unwinders has no effect.  */
> -extern bool dwarf2_frame_unwinders_enabled_p;
> -
>  /* Set the architecture-specific register state initialization
>     function for GDBARCH to INIT_REG.  */
>  
> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index 3ab6af1c4da..968bb846402 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -29,6 +29,7 @@
>  #include "gdbarch.h"
>  #include "dwarf2/frame-tailcall.h"
>  #include "cli/cli-cmds.h"
> +#include "cli/cli-option.h"
>  #include "inferior.h"
>  
>  /* Conversion list between the enum for frame_unwind_class and
> @@ -89,6 +90,20 @@ frame_unwinder_class_str (frame_unwind_class uclass)
>    return unwind_class_conversion[uclass];
>  }
>  
> +/* 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)
> +{
> +  for (int i = 0; i < UNWIND_CLASS_NUMBER; i++)
> +    {
> +      if (strcasecmp (unwind_class_conversion[i], class_str) == 0)
> +	return (frame_unwind_class) i;
> +    }
> +
> +  error (_("Unknown frame unwind class: %s"), class_str);
> +}
> +
>  void
>  frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
>  				const struct frame_unwind *unwinder)
> @@ -175,27 +190,46 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>    FRAME_SCOPED_DEBUG_ENTER_EXIT;
>    frame_debug_printf ("this_frame=%d", frame_relative_level (this_frame));
>  
> -  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 internal_error at the end of this function.  */
> +  bool seen_disabled_unwinder = false;
> +  /* Lambda to factor out the logic of checking if an unwinder is enabled,
> +     testing it and otherwise recording if we saw a disable unwinder.  */
> +  auto test_unwinder = [&] (const struct frame_unwind *unwinder)
> +    {
> +      if (unwinder == nullptr)
> +	return false;
> +
> +      if (!unwinder->enabled ())
> +	{
> +	  seen_disabled_unwinder = true;
> +	  return false;
> +	}
> +
> +      return frame_unwind_try_unwinder (this_frame,
> +					this_cache,
> +					unwinder);
> +    };
>  
> -  unwinder_from_target = target_get_unwinder ();
> -  if (unwinder_from_target != NULL
> -      && frame_unwind_try_unwinder (this_frame, this_cache,
> -				   unwinder_from_target))
> +  if (test_unwinder (target_get_unwinder ()))
>      return;
>  
> -  unwinder_from_target = target_get_tailcall_unwinder ();
> -  if (unwinder_from_target != NULL
> -      && frame_unwind_try_unwinder (this_frame, this_cache,
> -				   unwinder_from_target))
> +  if (test_unwinder (target_get_tailcall_unwinder ()))
>      return;
>  
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    std::vector<const frame_unwind *> *table = get_frame_unwind_table (gdbarch);
>    for (auto unwinder : *table)
> -    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
> -      return;
> +    {
> +      if (test_unwinder (unwinder))
> +	return;
> +    }
>  
> -  internal_error (_("frame_unwind_find_by_frame failed"));
> +  if (seen_disabled_unwinder)
> +    error (_("Required frame unwinder may have been disabled"
> +	     ", see 'maint info frame-unwinders'"));
> +  else
> +    internal_error (_("frame_unwind_find_by_frame failed"));
>  }
>  
>  /* A default frame sniffer which always accepts the frame.  Used by
> @@ -394,10 +428,11 @@ maintenance_info_frame_unwinders (const char *args, int from_tty)
>    std::vector<const frame_unwind *> *table = get_frame_unwind_table (gdbarch);
>  
>    ui_out *uiout = current_uiout;
> -  ui_out_emit_table table_emitter (uiout, 3, -1, "FrameUnwinders");
> +  ui_out_emit_table table_emitter (uiout, 4, -1, "FrameUnwinders");
>    uiout->table_header (27, ui_left, "name", "Name");
>    uiout->table_header (25, ui_left, "type", "Type");
>    uiout->table_header (9, ui_left, "class", "Class");
> +  uiout->table_header (8, ui_left, "enabled", "Enabled");
>    uiout->table_body ();
>  
>    for (auto unwinder : *table)
> @@ -407,10 +442,145 @@ maintenance_info_frame_unwinders (const char *args, int from_tty)
>        uiout->field_string ("type", frame_type_str (unwinder->type ()));
>        uiout->field_string ("class", frame_unwinder_class_str (
>  					unwinder->unwinder_class ()));
> +      uiout->field_string ("enabled", unwinder->enabled () ? "Y" : "N");
>        uiout->text ("\n");
>      }
>  }
>  
> +/* Options for disabling frame unwinders.  */
> +struct maint_frame_unwind_options
> +{
> +  std::string unwinder_name;
> +  const char *unwinder_class = nullptr;
> +  bool all = false;
> +};
> +
> +static const gdb::option::option_def maint_frame_unwind_opt_defs[] = {
> +
> +  gdb::option::flag_option_def<maint_frame_unwind_options> {
> +    "all",
> +    [] (maint_frame_unwind_options *opt) { return &opt->all; },
> +    nullptr,
> +    N_("Change the state of all unwinders")
> +  },
> +  gdb::option::string_option_def<maint_frame_unwind_options> {
> +    "name",
> +    [] (maint_frame_unwind_options *opt) { return &opt->unwinder_name; },
> +    nullptr,
> +    N_("The name of the unwinder to have its state changed")
> +  },
> +  gdb::option::enum_option_def<maint_frame_unwind_options> {
> +    "class",
> +    unwind_class_conversion,
> +    [] (maint_frame_unwind_options *opt) { return &opt->unwinder_class; },
> +    nullptr,
> +    N_("The class of unwinders to have their states changed")
> +  }
> +};
> +
> +static inline gdb::option::option_def_group
> +make_frame_unwind_enable_disable_options (maint_frame_unwind_options *opts)
> +{
> +  return {{maint_frame_unwind_opt_defs}, opts};
> +}
> +
> +/* 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)
> +{
> +  if (args == nullptr)
> +    {
> +      if (enable)
> +	error (_("Specify which frame unwinder(s) should be enabled"));
> +      else
> +	error (_("Specify which frame unwinder(s) should be disabled"));
> +    }
> +
> +  struct gdbarch* gdbarch = current_inferior ()->arch ();
> +  std::vector<const frame_unwind *> *unwinder_list
> +    = get_frame_unwind_table (gdbarch);
> +
> +  maint_frame_unwind_options opts;
> +  gdb::option::process_options
> +    (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR,
> +     make_frame_unwind_enable_disable_options (&opts));
> +
> +  if ((opts.all && !opts.unwinder_name.empty ())
> +      || (opts.all && opts.unwinder_class != nullptr)
> +      || (!opts.unwinder_name.empty () && opts.unwinder_class != nullptr))
> +    error (_("Options are mutually exclusive"));
> +
> +  /* First see if the user wants to change all unwinders.  */
> +  if (opts.all)
> +    {
> +      for (const frame_unwind *u : *unwinder_list)
> +	u->set_enabled (enable);
> +
> +      reinit_frame_cache ();
> +      return;
> +    }
> +
> +  /* If user entered a specific unwinder name, handle it here.  If the
> +     unwinder is already at the expected state, error out.  */
> +  if (!opts.unwinder_name.empty ())
> +    {
> +      bool did_something = false;
> +      for (const frame_unwind *unwinder : *unwinder_list)
> +	{
> +	  if (strcasecmp (unwinder->name (),
> +			  opts.unwinder_name.c_str ()) == 0)
> +	    {
> +	      if (unwinder->enabled () == enable)
> +		{
> +		  if (unwinder->enabled ())
> +		    error (_("unwinder %s is already enabled"),
> +			     unwinder->name ());
> +		  else
> +		    error (_("unwinder %s is already disabled"),
> +			     unwinder->name ());
> +		}
> +	      unwinder->set_enabled (enable);
> +
> +	      did_something = true;
> +	      break;
> +	    }
> +	}
> +      if (!did_something)
> +	error (_("couldn't find unwinder named %s"),
> +	       opts.unwinder_name.c_str ());
> +    }
> +  else
> +    {
> +      if (opts.unwinder_class == nullptr)
> +	opts.unwinder_class = args;
> +      enum frame_unwind_class dclass = str_to_frame_unwind_class
> +	(opts.unwinder_class);
> +      for (auto unwinder: *unwinder_list)

I think you should stick with the form you use earlier:

  (const frame_unwind *unwinder : *unwinder_list)

But if you really want to stick with auto, then I think:

 (const auto &unwinder : *unwinder_list)

would be OK.


> +	{
> +	  if (unwinder->unwinder_class () == dclass)
> +	    unwinder->set_enabled (enable);
> +	}
> +    }
> +
> +  reinit_frame_cache ();
> +}
> +
> +/* Implement "maint frame-unwinder disable" command.  */
> +static void
> +maintenance_disable_frame_unwinders (const char *args, int from_tty)
> +{
> +  enable_disable_frame_unwinders (args, from_tty, false);
> +}
> +
> +/* Implement "maint frame-unwinder enable" command.  */
> +static void
> +maintenance_enable_frame_unwinders (const char *args, int from_tty)
> +{
> +  enable_disable_frame_unwinders (args, from_tty, true);
> +}
> +
>  void _initialize_frame_unwind ();
>  void
>  _initialize_frame_unwind ()
> @@ -423,4 +593,45 @@ _initialize_frame_unwind ()
>  List the frame unwinders currently in effect.\n\
>  Unwinders are listed 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. Valid classes are:\n\
> +\t\tGDB       - Unwinders added by GDB core;\n\
> +\t\tEXTENSION - Unwinders added by extension languages;\n\
> +\t\tDEBUGINFO - Unwinders that handle debug information;\n\
> +\t\tARCH      - Unwinders that use architecture-specific information;\n\
> +\n\
> +UNWINDER and NAME are case insensitive."),
> +	   &maint_frame_unwinder);
> +
> +  add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders,
> +	   _("\
> +Enable one or more frame unwinder(s).\n\
> +Usage: maint frame-unwinder enable [-all | -name NAME | [-class] CLASS]\n\
> +\n\
> +These are the meanings of the options:\n\
> +\t-all    - All available unwinders will be enabled\n\
> +\t-name   - NAME is the exact name of the frame unwinder to be enabled\n\
> +\t-class  - CLASS is the class of unwinders to be enabled. Valid classes are:\n\
> +\t\tGDB       - Unwinders added by GDB core;\n\
> +\t\tEXTENSION - Unwinders added by extension languages;\n\
> +\t\tDEBUGINFO - Unwinders that handle debug information;\n\
> +\t\tARCH      - Unwinders that use architecture-specific information;\n\
> +\n\
> +UNWINDER and NAME are case insensitive."),
> +	   &maint_frame_unwinder);
>  }

At the bottom of this email you'll find a small patch which applies on
top of this and adds some basic command completion.  It doesn't complete
the NAMEs, I'm not sure if there's a good way to do that (yet).  It also
doesn't switch the help text to use the %OPTIONS% mechanism, I tried
that, but I think there may be some bugs with the formatting (I'll look
into it), so I've left that off.

You don't have to, we can add completion later, but I do think it would
be good to merge the patch below with this one.  But it is up to you.
Take a look and see what you think.

> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
> index 13f18618d24..adf8b47982c 100644
> --- a/gdb/frame-unwind.h
> +++ b/gdb/frame-unwind.h
> @@ -192,6 +192,12 @@ class frame_unwind
>    const frame_data *unwind_data () const
>    { return m_unwind_data; }
>  
> +  bool enabled () const
> +  { return m_enabled; }
> +
> +  void set_enabled (bool state) const
> +  { m_enabled = state; }
> +
>    /* Default stop_reason implementation.  It reports NO_REASON, unless the
>       frame is the outermost.  */
>  
> @@ -245,6 +251,9 @@ class frame_unwind
>    frame_unwind_class m_unwinder_class;
>  
>    const frame_data *m_unwind_data;
> +
> +  /* Whether this unwinder can be used when sniffing.  */
> +  mutable bool m_enabled = true;
>  };
>  
>  /* This is a legacy version of the frame unwinder.  The original struct
> diff --git a/gdb/testsuite/gdb.base/frame-info-consistent.exp b/gdb/testsuite/gdb.base/frame-info-consistent.exp
> index fe0cfad95bc..4f483111a91 100644
> --- a/gdb/testsuite/gdb.base/frame-info-consistent.exp
> +++ b/gdb/testsuite/gdb.base/frame-info-consistent.exp
> @@ -95,11 +95,11 @@ proc compare_frames {frames} {
>      }
>  }
>  
> -proc test {dwarf_unwinder} {
> +proc test {enable} {
>  
>      clean_restart $::binfile
>  
> -    gdb_test_no_output "maint set dwarf unwinder $dwarf_unwinder"
> +    gdb_test_no_output "maint frame-unwinder $enable DEBUGINFO"
>  
>      if {![runto_main]} {
>  	return 0
> @@ -134,6 +134,6 @@ proc test {dwarf_unwinder} {
>      }
>  }
>  
> -foreach_with_prefix dwarf {"off" "on"} {
> -    test $dwarf
> +foreach_with_prefix action {"disable" "enable"} {
> +    test $action
>  }
> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.c b/gdb/testsuite/gdb.base/frame-unwind-disable.c
> new file mode 100644
> index 00000000000..bbcfb01316e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2024 Free Software Foundation, Inc.

Remember to update the copyright date when rebasing.

> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.exp b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
> new file mode 100644
> index 00000000000..f4cad9a47fd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
> @@ -0,0 +1,137 @@
> +# Copyright 2024 Free Software Foundation, Inc.

And again with the date.

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test multiple situations in which we may use the maintenance command to
> +# disable and enable frame unwinders, and check that they really are
> +# disabled when they say the are.
> +
> +standard_testfile .c

You can drop '.c' here, it's the default.

> +
> +# Proc to check if the unwinder of the given name is in the desired state.
> +# STATE can be either Y or N.
> +proc check_unwinder_state { unwinder_name state {testname ""} } {
> +    set should_pass false
> +    set command "maint info frame-unwinders"
> +    if {${testname} == ""} {
> +	set testname "checking state ${state} for ${unwinder_name}"
> +    }
> +    gdb_test_multiple "${command}" "${testname}" -lbl {
> +	-re "${unwinder_name}\\s+\\w+\\s+\\w+\\s+${state}\\s+(?=\r\n)" {
> +	    set should_pass true
> +	    exp_continue
> +	}
> +	-re "${command}" {
> +	    exp_continue
> +	}
> +	-re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+(?=\r\n)" {
> +	    exp_continue
> +	}
> +	-re -wrap "" {
> +	    gdb_assert ${should_pass} "${gdb_test_name}"

I don't think the quotes around "${gdb_test_name}" are needed here?  In
fact, there seem to be a few places where you place quotes around
expansion of a single variable (e.g. "${command}" on the
gdb_test_multiple line), I don't think any of these are needed.

With these changes (and with or without the completion patch):

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> +	}
> +    }
> +}
> +
> +# Check if all unwinders of class UNWINDER_CLASS are in the state STATE.
> +# STATE can be either Y or N.
> +# UNWINDER_CLASS can be one of: GDB, ARCH, EXTENSION, DEBUGINFO.  It can
> +# also be \\w+ if checking all unwinders.
> +proc check_unwinder_class { unwinder_class state {testname ""} } {
> +    set command "maint info frame-unwinders"
> +    set should_pass true
> +    if {$testname == ""} {
> +	set testname "checking if ${unwinder_class} state is ${state}"
> +    }
> +    gdb_test_multiple "${command}" "${testname}" -lbl {
> +	-re "^\[^\r\n\]+\\s+\\w+\\s+${unwinder_class}\\s+\(\[YN\]\)\\s+(?=\r\n)" {
> +	    # The unwinder name may have multiple words, so we need to use the
> +	    # more generic [^\r\n] pattern to match the unwinders.
> +	    set cur_state $expect_out(1,string)
> +	    if {$cur_state == $state} {
> +		set should_pass false
> +	    }
> +	    exp_continue
> +	}
> +	-re "${command}" {
> +	    exp_continue
> +	}
> +	-re -wrap "" {
> +	    gdb_assert ${should_pass} "${gdb_test_name}"
> +	}
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "couldn't run to main"
> +    return
> +}
> +
> +# Test disabling all unwinders.
> +check_unwinder_class "\\w+" "Y" \
> +    "all unwinders enabled before any changes"
> +gdb_test_no_output "maint frame-unwinder disable -all"
> +check_unwinder_class "\\w+" "N" \
> +    "all unwinders were properly disabled"
> +
> +# Test if GDB can still make a backtrace once all unwinders are disabled.
> +# It should be impossible.
> +gdb_test "backtrace" \
> +    ".*Required frame unwinder may have been disabled, see 'maint info frame-unwinders'.*" \
> +    "no suitable unwinders should be found"
> +
> +# Reenable all unwinders.
> +gdb_test_no_output "maint frame-unwinder enable -all"
> +check_unwinder_class "\\w+" "Y" \
> +    "all unwinders should be re-enabled"
> +
> +# Check that we are able to get backtraces once again.
> +gdb_test "backtrace" ".0\\s+main .. at.*" \
> +    "we can get usable backtraces again"
> +
> +# Check if we can disable an unwinder based on the name.
> +check_unwinder_state "dummy" "Y"
> +gdb_test_no_output "maint frame-unwinder disable -name dummy"
> +check_unwinder_state "dummy" "N"
> +# And verify what happens if you try to disable it again.
> +gdb_test "maint frame-unwinder disable -name dummy" \
> +    "unwinder dummy is already disabled" \
> +    "disable already disabled unwinder"
> +check_unwinder_state "dummy" "N" "dummy should continue disabled"
> +
> +foreach class {GDB ARCH DEBUGINFO EXTENSION} {
> +    # Disable all unwinders of type CLASS, and check that the command worked.
> +    gdb_test_no_output "maint frame-unwinder disable ${class}"
> +    check_unwinder_class "${class}" "N"
> +}
> +
> +# Now check if we are able to enable a single unwinder, and what happens if we
> +# enable it twice.
> +gdb_test_no_output "maint frame-unwinder enable -name dummy"
> +check_unwinder_state "dummy" "Y" "successfully enabled dummy unwinder"
> +gdb_test "maint frame-unwinder enable -name dummy" \
> +    "unwinder dummy is already enabled" \
> +    "enable already enabled unwinder"
> +check_unwinder_state "dummy" "Y" "dummy should continue enabled"
> +
> +foreach class {GDB ARCH DEBUGINFO EXTENSION} {
> +    # Enable all unwinders of type CLASS, and check that the command worked,
> +    # using "-class" option to ensure it works.  Should make no difference.
> +    gdb_test_no_output "maint frame-unwinder enable -class ${class}"
> +    check_unwinder_class "${class}" "Y"
> +}
> diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp
> index 2c58ffa36c5..ae7ad0787e6 100644
> --- a/gdb/testsuite/gdb.base/maint.exp
> +++ b/gdb/testsuite/gdb.base/maint.exp
> @@ -454,10 +454,6 @@ gdb_test_no_output "maint info line-table xxx.c" \
>  
>  set timeout $oldtimeout
>  
> -# Just check that the DWARF unwinders control flag is visible.
> -gdb_test "maint show dwarf unwinders" \
> -    "The DWARF stack unwinders are currently (on|off)\\."
> -
>  #============test help on maint commands
>  
>  test_prefix_command_help {"maint info" "maintenance info"} {
> -- 
> 2.47.0

---

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 1417651108f..f3019b7ec18 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -40,6 +40,7 @@ static const char * unwind_class_conversion[] =
   "EXTENSION",
   "DEBUGINFO",
   "ARCH",
+  nullptr
 };
 
 /* Default sniffers, that must always be the first in the unwinder list,
@@ -579,6 +580,29 @@ enable_disable_frame_unwinders (const char *args, int from_tty, bool enable)
   reinit_frame_cache ();
 }
 
+/* Completer for the "maint frame-unwinder enable|disable" commands.  */
+
+static void
+enable_disable_frame_unwinders_completer (struct cmd_list_element *ignore,
+					  completion_tracker &tracker,
+					  const char *text,
+					  const char */*word*/)
+{
+  maint_frame_unwind_options opts;
+  const auto group = make_frame_unwind_enable_disable_options (&opts);
+
+  const char *start = text;
+  if (gdb::option::complete_options
+      (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, group))
+    return;
+
+  /* Only complete a class name as a stand-alone operand when no options
+     are given.  */
+  if (start == text)
+    complete_on_enum (tracker, unwind_class_conversion, text, text);
+  return;
+}
+
 /* Implement "maint frame-unwinder disable" command.  */
 static void
 maintenance_disable_frame_unwinders (const char *args, int from_tty)
@@ -613,8 +637,9 @@ Unwinders are listed starting with the highest priority."),
 			_("Commands handling frame unwinders."),
 			&maint_frame_unwinder, 0, &maintenancelist);
 
-  add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders,
-	   _("\
+  cmd_list_element *c
+    = 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\
@@ -628,10 +653,13 @@ These are the meanings of the options:\n\
 \t\tARCH      - Unwinders that use architecture-specific information;\n\
 \n\
 UNWINDER and NAME are case insensitive."),
-	   &maint_frame_unwinder);
+	       &maint_frame_unwinder);
+  set_cmd_completer_handle_brkchars (c,
+				     enable_disable_frame_unwinders_completer);
 
-  add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders,
-	   _("\
+  c =
+    add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders,
+	     _("\
 Enable one or more frame unwinder(s).\n\
 Usage: maint frame-unwinder enable [-all | -name NAME | [-class] CLASS]\n\
 \n\
@@ -645,5 +673,7 @@ These are the meanings of the options:\n\
 \t\tARCH      - Unwinders that use architecture-specific information;\n\
 \n\
 UNWINDER and NAME are case insensitive."),
-	   &maint_frame_unwinder);
+	     &maint_frame_unwinder);
+  set_cmd_completer_handle_brkchars (c,
+				     enable_disable_frame_unwinders_completer);
 }