Re: [PATCH v3] gdb: fix using clear command to delete non-user breakpoints(PR cli/7161)

[Pedro Alves <pedro@palves.net>]

On 2022-04-16 04:42, Enze Li via Gdb-patches wrote:

> diff --git a/gdb/testsuite/gdb.base/clear_non_user_bp.exp b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
> new file mode 100644
> index 00000000000..4f0731e06f6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/clear_non_user_bp.exp
> @@ -0,0 +1,64 @@
> +# Copyright 2022 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/>.
> +
> +# Regression test for PR gdb/7161.  Test that GDB cannot delete non-user
> +# breakpoints with clear command.
> +
> +proc get_maint_info_bp { var } {
> +    global expect_out
> +    global gdb_prompt
> +
> +    gdb_test_multiple "maint info break -1" "find address of internal bp $var" {
> +	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> +            return $expect_out(1,string)
> +	}
> +	timeout {
> +	    perror "couldn't find address of $var"
> +	    return ""
> +	}

You shouldn't need a timeout match, gdb_test_multiple already has one internally.

> +    }
> +    return ""
> +}
> +
> +standard_testfile .c
> +
> +# this testcase just needs a "Hello world" source file, reuse

Uppercase "this" -> "This".

> +# gdb.server/sysroot.c instead of adding a new one.
> +if  { [gdb_compile "${srcdir}/gdb.server/sysroot.c" "${binfile}" executable {debug}] != "" } {
> +     untested "failed to compile"
> +     return -1
> +}
> +
> +# Start with a fresh gdb.
> +clean_restart ${binfile}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +gdb_test "break sysroot.c:23" \
> +	".*Breakpoint.* at .*" \
> +	"set breakpoint"
> +
> +set bp_addr [get_maint_info_bp "-1"]
> +

It's not portable to assume that breakpoint -1 is the shlib_events breakpoint.
Some targets don't even use such a breakpoint.  E.g., Windows.  It would be
better to instead make get_maint_info_bp look for the first internal breakpoint and
return its number.

> +gdb_test "maint info break -1" \
> +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> +    "maint info breakpoint -1 error"

Why is this test called "... error" ?

> +
> +gdb_test "clear *$bp_addr" \
> +    "No breakpoint at \\*$bp_addr." \
> +    "clear internal breakpoint error"
> +

It would be good to issue the "maint info break" command again after the clear,
to make sure that GDB really didn't delete the breakpoint, even if the clear command
errored out.

Not a strong requirement, but it would be great if we also had a test trying to clear an internal
python breakpoint.  Did you look into adding that?  If it's too complicated to add it here or to
a new file, maybe add a "clear" test to gdb.python/py-breakpoint.exp ?