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

[Guinevere Larsen <guinevere@redhat.com>]

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