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

[Enze Li <lienze2010@hotmail.com>]

Hi Lancelot,

Thank you for the review.

I'm very sorry for the long wait, I dunno what's going on with my
email.  I didn't receive this email from you and gdb-patches@list until
I happened to see your reply on the gdb-patches web page[1].  I copied
the content from the web page and replied to you here, please see below.

[1] https://sourceware.org/pipermail/gdb-patches/2022-April/187959.html

>Lancelot SIX lsix@lancelotsix.com
>Sun Apr 17 22:29:29 GMT 2022
>
>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?
>

Yes, I didn't notice the problem at the time.

>> +	-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;"]
>

I also noticed this "Note" message a few hours ago, and I was thinking
of deleting this piece of code, which is actually possible.

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

Thank you so much for providing such detailed instructions and advice,
it was really helpful.  I'll cc you when the next version is ready if
you don't mind.

Thanks again,
Enze