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

[Enze Li <lienze2010@hotmail.com>]

Hi Pedro,

Thank you for the review.

On Mon, 2022-04-18 at 17:04 +0100, Pedro Alves wrote:
> On 2022-04-16 04:42, Enze Li via Gdb-patches wrote:
> 

> 
<snip>

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

Fixed.

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

Fixed.

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

Thanks, this helps me a lot.

> > +gdb_test "maint info break -1" \
> > +    "-1.*shlib events.*keep y.*$bp_addr.*" \
> > +    "maint info breakpoint -1 error"
> 
> Why is this test called "... error" ?

Drop the word "error"? Done.

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

Fixed.

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

If this is a requirement, I'd like to implement it.  Is this something
that can be done later?

I've sent a fix patch to the gdb-patch@list and I posted it here:
https://sourceware.org/pipermail/gdb-patches/2022-April/188044.html


Thanks,
Enze