[PATCH v8 4/5] gdb: introduce ability to disable frame unwinders
Guinevere Larsen
guinevere@redhat.com
Fri Jan 17 12:40:13 GMT 2025
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);
>>
>>
>> -/* 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
>
> 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.
--
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);
> }
>
More information about the Gdb-patches
mailing list