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

Guinevere Larsen
2024-10-09T13:32:56Z

On 10/5/24 11:51 PM, Simon Marchi wrote:
>
> 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.
Heh, I guess thinking about the next test where I want to disable all
debuginfo unwinders and it just felt like the obvious choice.
>> 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.
Fixed
> OTOH, I don't think accepting user inputs FRAME_UNWIND_ is really
> necessary.
That's a fair question, but I don't think this is too costly (for
performance, or maintenance) to keep it, in case someone reads the code
instead of documentation (not that I do that or anything o.o'). If you
disagree, I am fine with removing it honestly, especially with
documenting the classes that you suggested later.
>
>> + 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.
done
>
>
>> + 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.
Good idea. I factored into a lambda that receives the unwinder as a
parameter, so I can use it every time frame_unwind_try_unwinder.
>
>> @@ -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.
Fixed both.
>
>> + 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.

I hadn't looked into using this framework when I first implemented it.
However, looking at it now, it looks like overkill for what I need. The
gdb::option path need the option array (with all the stuff that goes
along with it), to end up with a very similar setup in the function itself.

Now, if you think I'm overselling the complexity or that we are likely
to add more options to this command, I can add it for the next version.

>
>> @@ -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?
Makes sense, fixed!
>
> Simon
>

--
Cheers,
Guinevere Larsen
She/Her/Hers