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

[Lancelot SIX <lsix@lancelotsix.com>]

Hi,

I think Tom already OK-ed this patch, but I have minor comments, if you do not
mind.

> +
> +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" {

In this proc, the `var` param is only used in the messages, not in the
actual command.  Did you mean "maint info break $var" here?

> +	-re ".*(0x\[0-9a-f\]+).*$gdb_prompt $" {
> +	    return $expect_out(1,string)
> +	}
> +	timeout {
> +	    perror "couldn't find address of $var"
> +	    return ""
> +	}
> +    }
> +    return ""
> +}

> +
> +standard_testfile .c
> +
> +# This testcase just needs a "Hello world" source file, reuse
> +# gdb.base/main.c instead of adding a new one.
> +if  { [gdb_compile "${srcdir}/${subdir}/main.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 main.c:21" \
> +    ".*Breakpoint.* at .*" \
> +    "set breakpoint"

If I run this test, I have:

    (gdb) break -qualified main
    Breakpoint 1 at 0x401020: file .../gdb/testsuite/gdb.base/main.c, line 21.
    (gdb) run 
    Starting program: .../gdb/testsuite/outputs/gdb.base/clear_non_user_bp/clear_non_user_bp 
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library ".../lib/libthread_db.so.1".

    Breakpoint 1, main () at .../gdb/testsuite/gdb.base/main.c:21
    21        return 0;
    (gdb) break main.c:21
    Note: breakpoint 1 also set at pc 0x401020.
    Breakpoint 2 at 0x401020: file .../gdb/testsuite/gdb.base/main.c, line 21.
    (gdb) PASS: gdb.base/clear_non_user_bp.exp: set breakpoint

So you already have a user breakpoint inserted (which also happens to be
at the same address as the breakpoint you want to insert).  I am not sure
this adds much to your testcase (but I might be wrong).

Also, I do not know how maintainers feel about this (I am not one), but
I feel that using gdb.base/main.c which is not directly linked to this
testcase makes it easy to have main.c accidentally edited with this file
not being touched.  With a plain line number, we might end up with
unexpected test changes.  I really do not expect this file to change any
time soon, but who knows, an update in the license header might lead to
the line you are looking for not being 21 anymore.

If you really need this breakpoint, I guess I would go for something
like:

    gdb_break [gdb_get_line_number "return 0;"]

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

The last param is the name of the test, not an error message.  If the
test was to fail, the output would be:

    FAIL: main info breakpoint -1 error

"FAIL" and "error" would be somewhat redundant.  I would suggest
dropping the last argument altogether since, if missing, the framework
uses the 1st parameter (the command) as default value.

Also, I feel that both main

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

Here, I would suggest just dropping the " error" part of the testname.
"$bp_addr" is likely to change from one configuration to another,
making diffing .sum files difficult.

Best,
Lancelot.