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

[Guinevere Larsen <guinevere@redhat.com>]

On 1/17/25 10:55 AM, Andrew Burgess wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> On 1/16/25 9:06 AM, Andrew Burgess wrote:
>>> 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):
>> I applied all the changes, and I like the idea of adding completion, but
>> I wonder: should we add a test for completion? If so, I think this is
>> better saved for a later patch, so that we can work on a good test and
>> further improve the completion, because it would be nice to also
>> complete for "-class", "-name" and "-all", but I'm not sure how that'd
>> work yet.
>>
>> If a test isn't all that important we can add it now and do more work
>> later on. I also have an inlined styling question
> I'm not sure every function with completion has tests.  We do have some
> general tests that cover completion though.
>
> If it bothers you, then feel free to leave it out for now, I certainly
> don't want to delay this work going in just for completion of a
> maintenance command, you already switched over to the option system for
> me, which means we're better placed to add completion now or later, so
> I'm grateful for that.
>
> The function I proposed should be able to complete the '-class',
> '-name', and '-all' option names.  Additionally, the '-class' value
> should complete.  And the class name as an operand, if there are no
> options given.
Oh, I must have misread the code then. I'm happy adding the completion then!
>
> Completion of -name values is trickier, as the possible set of names can
> change by architecture, and the set of extensions loaded (I think).
> What we really need is for -name to be a "dynamic" enum option, where
> the dynamic property is provided by a callback which gets the list of
> possible enum values.  But such dynamic enums don't currently exist.
> But I might look into adding that feature once this patch lands.
Yeah, I remember we talked about this. It would be a really nice addition!
>
> Anyway, feel free to add the completion, or not.  If not I'll follow up
> afterwards.
>
>>> 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*/)
>> Would it be alright to write "const char * /*word*/" ? the */* is
>> confusing my editor and it looks like malformed comments, and if I add
>> the space, the red highlights go away.
> Sure.  I just copied this from elsewhere in GDB :)
>
> Thanks,
> Andrew
>
>
>> -- 
>> Cheers,
>> Guinevere Larsen
>> She/Her/Hers
>>
>>> +{
>>> +  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);
>>>    }
>>>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers