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")

I think for flag_option_def, this should be:

  gdb::option::flag_option_def<maint_frame_unwind_options> {
    "all",
    [] (maint_frame_unwind_options *opt) { return &opt->all; },
    N_("Change the state of all unwinders")
  }

dropping the 'nullptr'.  If/when we use %OPTIONS% this will then print
that 'Change the state of all unwinders' text as the description of this
option.

Thanks,
Andrew