Re: [PATCH] Notify observer of breakpoint auto-disabling

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-08-14 8:33 p.m., Patrick Monnerat via Gdb-patches wrote:
> As breakpoint_modified observer is currently notified upon breakpoint stop
> before handling auto-disabling when enable count is reached, the observer is
> never notified of the disabling.
> 
> The problem affects:
> - The MI interpreter enabled= value when reporting =breakpoint-modified
> - A Python event handler for breakpoint_modified using the "enabled"
>   member of its parameter
> - insight: breakpoint GUI window is not properly updated upon auto-disable
> 
> This patch moves the observer notification after the auto-disabling
> code and implements corresponding tests for the MI and Python cases.

Thanks for writing test cases.  It looks good, just some nits to fix.

> +proc test_auto_disable { } {
> +    global mi_gdb_prompt
> +    global lib_sl1 lib_sl2
> +    global binfile
> +
> +    mi_clean_restart $binfile
> +
> +    mi_load_shlibs $lib_sl1 $lib_sl2
> +
> +    mi_runto_main
> +
> +    # Set the breakpoint.
> +    mi_gdb_test "-break-insert -f pendfunc1" \
> +	{(&.*)*.*~"Breakpoint 2 at.*\\n".*=breakpoint-created,bkpt=\{number="2",type="breakpoint".*\}.*\n\^done}
> +
> +    # Enable for one shot only.
> +    mi_gdb_test "-break-enable count 1 2" \
> +	{=breakpoint-modified,bkpt=\{number="2",type="breakpoint",disp="dis",enabled="y".*\}.*\n\^done}
> +
> +    mi_send_resuming_command "exec-continue" "continuing execution to breakpoint"
> +
> +    set test "breakpoint auto-disabling after enable count reached"
> +    gdb_expect {
> +        -re ".*=breakpoint-modified,bkpt=\{number=\"2\".*enabled=\"n\"" {
> +            pass $test

There are some issues with the indentation in the new code, groups of 8
spaces should be replaced with a tab.  For example, the above should be
one tab + 4 spaces (see the existing code for an example).

> +        }
> +        -re ".*${mi_gdb_prompt}$" {
> +            fail $test
> +        }
> +        timeout {
> +            fail "$test (timeout)"
> +        }
> +    }
> +}
> +
> +with_test_prefix "test_auto_disable" {
> +    test_auto_disable
> +}
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index e2ffe8cbe46..e9102e7e3ea 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -799,6 +799,26 @@ proc_with_prefix test_catchpoints {} {
>      gdb_test "continue" "Stopped at catchpoint event: ${num}"
>  }
>  
> +proc_with_prefix test_bkpt_auto_disable { } {

Please add a small comment above the proc indicating the intent of the
test.

> +    global srcfile testfile hex decimal
> +
> +    # Start with a fresh gdb.
> +    clean_restart ${testfile}
> +
> +    if ![runto_main] then {
> +	fail "cannot run to main."
> +	return 0
> +    }
> +    delete_breakpoints
> +
> +    set mult_line [gdb_get_line_number "Break at multiply."]
> +    gdb_breakpoint ${mult_line}
> +    gdb_test "enable count 1 2" "" "One shot enable."

You can use the "gdb_test_no_output" proc.  Also, test messages start
with a lower case letter and don't end with a period.

Simon