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

Simon Marchi via Gdb-patches
2021-08-16T01:33:34Z

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