Re: [PATCH 4/4] GDB: introduce ability to disable frame unwinders

Guinevere Larsen
2024-03-06T14:07:46Z

On 06/03/2024 14:47, Eli Zaretskii wrote:
>> From: Guinevere Larsen <blarsen@redhat.com>
>> Cc: Guinevere Larsen <blarsen@redhat.com>
>> Date: Wed, 6 Mar 2024 13:51:35 +0100
>>
>> gdb/NEWS | 7 +
>> gdb/doc/gdb.texinfo | 24 +++
>> gdb/frame-unwind.c | 161 +++++++++++++++++-
>> gdb/frame-unwind.h | 13 ++
>> gdb/testsuite/gdb.base/frame-unwind-disable.c | 21 +++
>> .../gdb.base/frame-unwind-disable.exp | 114 +++++++++++++
>> 6 files changed, 336 insertions(+), 4 deletions(-)
>> create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c
>> create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp
> Thanks.
>
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -43,6 +43,13 @@ disable missing-debug-handler LOCUS HANDLER
>> maintenance info linux-lwps
>> List all LWPs under control of the linux-nat target.
>>
>> +maintenance frame-unwinder disable [-name]
>> +maintenance frame-unwinder enable [-name]
>> + 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
> ^^
> Two spaces between sentences, please.
>
>> +@kindex maint frame-unwinder disable
>> +@kindex maint frame-unwinder enable
>> +@item maint frame-unwinder disable [OPTION] UNWINDER
>> +@item maint frame-unwinder enable [OPTION] UNWINDER
> OPTION and UNWINDER should be @var{option} and @var{unwinder},
> respectively.
>
>> +The meaning of the argument @samp{unwinder} depends on the @samp{option}
> The correct markup here is @var, not @samp, since these are not
> literal symbols, but instead names of parameters that stand for
> something else. Likewise elsewhere in the gdb.texinfo part of the
> patch.

Ah, thanks for explaining! Should I also change the @samp in the table
section?

>
>> +@table @samp
>> +@item @code{-all}
>> +ignore @samp{unwinder} and disable/enable all unwinders
> This should be a complete sentence: begin with a capital letter and
> end with a period.
>
>> +@item @code{-class}
>> +@samp{unwinder} is the class on frame unwinders to be disabled or enabled.
> ^^
> This should probably be "of"?
>
>> +The class may include the prefix @code{FRAME_UNWINDER_}, but it is not
>> +required. This is the default option.
> What do you mean by the last sentence? What is "this" that is the
> default option? And what does "default" mean for an option, since an
> option is by definition something that doesn't happen by default.

option is an optional parameter, and if the user decides to not provide
it, the command will work as if the user had provided the -class option.

I can see where the confusion came from, but I'm not really sure how to
improve the wording.

>
>> +Disable one or more frame unwinder(s).\n\
>> +Usage: maint frame-unwinder disable [OPTION] UNWINDER\n\
>> +\n\
>> +The meaning of UNWINDER depends on the OPTION given. These are the possibilities:\n\
> ^^
>> +\t-all - UNWINDER is ignored. All available unwinders will be disabled\n\
> ^^
>
> Two spaces between sentences in doc strings as well.
>
>> +The meaning of UNWINDER depends on the OPTION given. These are the possibilities:\n\
>> +\t-all - UNWINDER is ignored. All available unwinders will be enabled\n\
> Likewise here.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>

--
Cheers,
Guinevere Larsen
She/Her/Hers