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

Guinevere Larsen
2024-10-08T19:23:56Z

On 10/2/24 11:45 PM, Thiago Jung Bauermann wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> +/* 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
> git complains about this line:
>
> /home/bauermann/src/binutils-gdb/.git/worktrees/binutils-gdb-wt-2/rebase-apply/patch:327: trailing whitespace.
> else
> warning: 1 line adds whitespace errors.
oops, fixed.
>
>> + error (_("specify which frame unwinder(s) should be disabled"));
>> + }
>> 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..972d6b70351
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
>> @@ -0,0 +1,140 @@
>> +# Copyright 2024 Free Software Foundation, Inc.
>> +
>> +# 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
>> +
>> +# 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+" {
> With 'make check-read1', this test only passes if I remove the '^' from
> the beginning of the pattern.
>
>> + set should_pass true
>> + exp_continue
>> + }
>> + -re "${command}" {
>> + exp_continue
>> + }
>> + -re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+" {
>> + exp_continue
>> + }
>> + -re "${::gdb_prompt} $" {
>> + # We can't use -wrap here because it expects the line with start
>> + # with \r\n, but the previous commands eat that
> If I use -re -wrap "" here things still work (with or without read1), so
> this comment probably applied only to a previous version of this test.
>
> It does apply to the prompt pattern in check_unwinder_class, but there's
> a way around that. See below.
Thanks for all the TCL and regexp help, it's all too arcane to me at
this point. I applied all of your suggestions.
>
>> + gdb_assert {${should_pass} == true} "${gdb_test_name}"
>> + }
>> + }
>> +}
>> +
>> +proc check_unwinder_class { unwinder_class state {testname ""} } {
>> + set command "maint info frame-unwinders"
>> + set seen_arches 0
>> + set correct_state 0
>> + 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" {
> You can change the last "\r\n" in the pattern to "(?=\r\n)", which tests
> for the presence of the characters without taking them out of the buffer.
>
>> + # The unwinder name may have multiple words, so we need to use the
>> + # more generic [^\r\n] pattern to match the unwinders.
>> + set should_pass true
>> + incr seen_arches
>> + set cur_state $expect_out(1,string)
>> + if {$cur_state == $state} {
>> + incr correct_state
>> + }
>> + exp_continue
>> + }
>> + -re "${command}" {
>> + exp_continue
>> + }
>> + -re "\[^\r\n\]+\r\n" {
> Same here, though this pattern is very similar to what the -lbl option
> adds:
>
> if {$line_by_line} {
> append code {
> -re "\r\n\[^\r\n\]*(?=\r\n)" {
> exp_continue
> }
> }
> }
>
> So is it necessary?
it isn't, you're correct! removed.
>
>> + exp_continue
>> + }
>> + -re "${::gdb_prompt} $" {
> With the changes above I can use -re -wrap "" here.
>
>> + gdb_assert {${correct_state} == ${seen_arches}} "${gdb_test_name}"
>> + }
>> + }
>> +}

--
Cheers,
Guinevere Larsen
She/Her/Hers