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

Simon Marchi
2024-10-06T02:53:06Z

On 2024-10-01 14:42, Guinevere Larsen wrote:
> From: Guinevere Larsen <blarsen@redhat.com>
>
> Sometimes, in the GDB testsuite, we want to test the ability of specific
> unwinders to handle some piece of code. Usually this is done by trying
> to outsmart GDB, or by coercing the compiler to remove information that
> GDB would rely on. Both approaches have problems as GDB gets smarter
> with time, and that compilers might differ in version and behavior, or
> simply introduce new useful information. This was requested back in 2003
> in PR backtrace/8434.
>
> To improve our ability to thoroughly test GDB, this patch introduces a
> new maintenance command that allows a user to disable some unwinders,
> based on either the name of the unwinder or on its class. With this
> change, it will now be possible for GDB to not find any frame unwinders
> for a given frame, which would previously cause GDB to assert. GDB will
> now check if any frame unwinder has been disabled, and if some has, it
> will just error out instead of asserting.
>
> Unwinders can be disabled or re-enabled in 3 different ways:
> * Disabling/enabling all at once (using '-all').
> * By specifying an unwinder class to be disabled (option '-class').
> * By specifying the name of an unwinder (option '-name').
>
> If you give no options to the command, GDB assumes the input is an
> unwinder class. '-class' would make no difference if used, is just here
> for completeness.

Heh, it's funny, to me the natural way would have been to make -name the
default case.

> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index 7be01ec2203..f66f233c781 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -94,6 +94,23 @@ frame_unwinder_class_str (frame_unwind_class uclass)
> return location->second;
> }
>
> +/* Case insensitive search for a frame_unwind_class based on the given
> + string. */
> +static enum frame_unwind_class
> +str_to_frame_unwind_class (const char *class_str)
> +{
> + const char *prefix = "FRAME_UNWIND_";
> + /* Skip the prefix if present. */
> + if (strncasecmp (class_str, prefix, strlen(prefix)) == 0)

Missing space.

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

> + class_str += strlen (prefix);
> + for (const auto &it : unwind_class_conversion)
> + {
> + if (strcasecmp (it.second, class_str) == 0)
> + return it.first;
> + }

I would suggest spacing things out a bit, addking blank lines after the
declaration of `prefix`, after the if and after the for. I have a hard
time following things when they're all packed.

> + error (_("Unknown frame unwind class: %s"), class_str);
> +}
> +
> void
> frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
> const struct frame_unwind *unwinder)
> @@ -182,25 +199,47 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>
> const struct frame_unwind *unwinder_from_target;
>
> + /* If we see a disabled unwinder, we assume some test is being run on
> + GDB, and we don't want to assert at the end of this function. */
> + bool seen_disabled_unwinder = false;
> +
> unwinder_from_target = target_get_unwinder ();
> if (unwinder_from_target != NULL
> + && unwinder_from_target->enabled ()
> && frame_unwind_try_unwinder (this_frame, this_cache,
> unwinder_from_target))
> return;
> + else if (unwinder_from_target != nullptr
> + && !unwinder_from_target->enabled ())
> + seen_disabled_unwinder = true;

There would be less repetition like this:

if (unwinder_from_target != nullptr)
{
if (unwinder_from_target->enabled ())
{
if (frame_unwind_try_unwinder (this_frame, this_cache,
unwinder_from_target))
return;
}
else
seen_disabled_unwinder = true;
}

You could also factor out this logic to a lambda and use it at the
multiple spots in the function where this is done.

> @@ -405,15 +445,97 @@ maintenance_info_frame_unwinders (const char *args, int from_tty)
> const char *name = unwinder->name ();
> const char *type = frame_type_str (unwinder->type ());
> const char *uclass = frame_unwinder_class_str (unwinder->unwinder_class ());
> + const char *enabled = unwinder->enabled () ? "Y" : "N";
>
> ui_out_emit_list tuple_emitter (uiout, nullptr);
> uiout->field_string ("name", name);
> uiout->field_string ("type", type);
> uiout->field_string ("class", uclass);
> + uiout->field_string ("enabled", enabled);
> uiout->text ("\n");
> }
> }
>
> +/* Helper function to both enable and disable frame unwinders.
> + if ENABLE is true, this call will be enabling unwinders,
> + otherwise the unwinders will be disabled. */
> +static void
> +enable_disable_frame_unwinders (const char *args, int from_tty, bool enable)
> +{
> + reinit_frame_cache ();
> + if (args == nullptr)
> + {
> + if (enable)
> + error (_("specify which frame unwinder(s) should be enabled"));
> + else
> + error (_("specify which frame unwinder(s) should be disabled"));
> + }

Should the error messages be capitalized?

Not a big deal, but I would put the reinit_frame_cache a bit later, when
the unwinder list actually gets modified. If we error out before, we
technically don't need to reinit the frame cache.

> + struct gdbarch* gdbarch = current_inferior ()->arch ();
> + std::vector<const frame_unwind *> unwinder_list
> + = gdbarch_unwinder_list (gdbarch);
> +
> + /* First see if the user wants to change all unwinders. */
> + if (check_for_argument (&args, "-all"))
> + {
> + for (const frame_unwind *u : unwinder_list)
> + u->set_enabled (enable);
> + return;
> + }

Did you consider using the gdb::option framework for this (found in
cli/cli-option.h)? It's been a while since I touched it, I don't recall
if it would be a good fit for this, but it would be worth checking.

> @@ -425,4 +547,37 @@ _initialize_frame_unwind ()
> _("List the frame unwinders currently in effect, "
> "starting with the highest priority."),
> &maintenanceinfolist);
> +
> + /* Add "maint frame-unwinder disable/enable". */
> + static struct cmd_list_element *maint_frame_unwinder;
> +
> + add_basic_prefix_cmd ("frame-unwinder", class_maintenance,
> + _("Commands handling frame unwinders."),
> + &maint_frame_unwinder, 0, &maintenancelist);
> +
> + add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders,
> + _("\
> +Disable one or more frame unwinder(s).\n\
> +Usage: maint frame-unwinder disable [-all | -name NAME | [-class] CLASS]\n\
> +\n\
> +These are the meanings of the options:\n\
> +\t-all - All available unwinders will be disabled\n\
> +\t-name - NAME is the exact name of the frame unwinder to be disabled\n\
> +\t-class - CLASS is the class of unwinders to be disabled.\n\

Should the help maybe list the valid classes?

Simon