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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

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.

> +	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.

> +	    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?

> +	    exp_continue
> +	}
> +	-re "${::gdb_prompt} $" {

With the changes above I can use -re -wrap "" here.

> +	    gdb_assert {${correct_state} == ${seen_arches}} "${gdb_test_name}"
> +	}
> +    }
> +}

-- 
Thiago