[PATCH] Notify observer of breakpoint auto-disabling

[PATCH] Notify observer of breakpoint auto-disabling

[Patrick Monnerat]

As observer is currently notified of breakpoint stop before handling its
auto-disabling after count is reached, the observer is never notified of
the disabling.

The problem does not seem to affect gdb alone. However it impacts insight:
breakpoint GUI window is not properly updated upon auto-disable.

This patch moves the observer notification after the auto-disabling
code.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336

* gdb/breakpoint.c (bpstat_stop_status): move observer notification
  after auto-disabling code.
---
 gdb/breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 89af44ee4c6..feca224ccf4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
 	  if (bs->stop)
 	    {
 	      ++(b->hit_count);
-	      gdb::observers::breakpoint_modified.notify (b);
 
 	      /* We will stop here.  */
 	      if (b->disposition == disp_disable)
@@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
+	      gdb::observers::breakpoint_modified.notify (b);
 	      if (b->silent)
 		bs->print = 0;
 	      bs->commands = b->commands;
-- 
2.31.1

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

[Simon Marchi]

On 2021-08-13 6:24 p.m., Patrick Monnerat via Gdb-patches wrote:
> As observer is currently notified of breakpoint stop before handling its
> auto-disabling after count is reached, the observer is never notified of
> the disabling.
> 
> The problem does not seem to affect gdb alone. However it impacts insight:
> breakpoint GUI window is not properly updated upon auto-disable.
> 
> This patch moves the observer notification after the auto-disabling
> code.
> 
> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
> 
> * gdb/breakpoint.c (bpstat_stop_status): move observer notification
>   after auto-disabling code.

Note that GDB doesn't use ChangeLogs anymore.

> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 89af44ee4c6..feca224ccf4 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
>  	  if (bs->stop)
>  	    {
>  	      ++(b->hit_count);
> -	      gdb::observers::breakpoint_modified.notify (b);
>  
>  	      /* We will stop here.  */
>  	      if (b->disposition == disp_disable)
> @@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
>  		    b->enable_state = bp_disabled;
>  		  removed_any = 1;
>  		}
> +	      gdb::observers::breakpoint_modified.notify (b);

I looked at the change a bit more in depth, in particular at the various
observers of breakpoint_modified.  One of them is MI, who prints the
=breakpoint-modified notification.  That suggested that your change
would affect that notification in a similar way that insight is
affected.  And indeed, if I run a program with a breakpoint with "enable
count 1 1", the notification I get without your patch is:

  =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="dis",enabled="n",...
                                                                              ^
And with the patch:

  =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="dis",enabled="y",
                                                                              ^

I think the change is desirable in MI for the same reason it is
desirable in insight.  But this means we should add a test for it.
Maybe gdb.mi/mi-breakpoint-changed.exp would be a good place for it.  I
don't think there's an official way to set a count on a breakpoint using
MI.  However, it seems to work through -break-enable, since
-break-enable is implemented on top of the CLI "enable" command.  So
"-break-enable count 1 1" is equivalent to "enable count 1 1".  It's not
documented but...

There is also the Python breakpoint_modified event.  This script:

  def func(bp):
      print(bp.enabled)

  gdb.events.breakpoint_modified.connect(func)

Prints "True" before your patch and "False" after.  We should have a
test for that too, ideally, I guess in gdb.python/py-breakpoint.exp.

Simon

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

[Patrick Monnerat]

On 8/14/21 6:05 AM, Simon Marchi wrote:
> I looked at the change a bit more in depth, in particular at the various
> observers of breakpoint_modified.  One of them is MI, who prints the
> =breakpoint-modified notification.

Good catch !

> if I run a program with a breakpoint with "enable
> count 1 1", the notification I get without your patch is:
>
>    =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="dis",enabled="n",...
>                                                                                ^
> And with the patch:
>
>    =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="dis",enabled="y",
>                                                                                ^

You probably mean the other way round...


Next patch post will contain tests according to your advices.

Many thanks for your hints and your work on this.

Patrick

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

[Patrick Monnerat]

On 8/16/21 5:10 PM, Simon Marchi wrote:
> Pushed, thanks again.
>
Thanks to you for your help, Simon.

Patrick

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

[Simon Marchi]

On 2021-08-16 10:09 a.m., Patrick Monnerat wrote:
> 
> On 8/16/21 4:04 PM, Simon Marchi wrote:
>>
>> Thanks, this is ok to push.  Do you have push access?  Would you like to
>> push this on your behalf?
> 
> No I don't. I only have push access to insight. Please can you do it for me? Thanks in advance.

Pushed, thanks again.

Simon

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

[Patrick Monnerat]

On 8/16/21 4:04 PM, Simon Marchi wrote:
>
> Thanks, this is ok to push.  Do you have push access?  Would you like to
> push this on your behalf?

No I don't. I only have push access to insight. Please can you do it for 
me? Thanks in advance.

Patrick

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

[Simon Marchi]

On 2021-08-16 8:44 a.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.
> 
> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336

Thanks, this is ok to push.  Do you have push access?  Would you like to
push this on your behalf?

Simon

[PATCH] Notify observer of breakpoint auto-disabling

[Patrick Monnerat]

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.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
---
 gdb/breakpoint.c                              |  2 +-
 .../gdb.mi/mi-breakpoint-changed.exp          | 41 +++++++++++++++++++
 gdb/testsuite/gdb.python/py-breakpoint.exp    | 23 +++++++++++
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 89af44ee4c6..feca224ccf4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
 	  if (bs->stop)
 	    {
 	      ++(b->hit_count);
-	      gdb::observers::breakpoint_modified.notify (b);
 
 	      /* We will stop here.  */
 	      if (b->disposition == disp_disable)
@@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
+	      gdb::observers::breakpoint_modified.notify (b);
 	      if (b->silent)
 		bs->print = 0;
 	      bs->commands = b->commands;
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index 3f7ef8a79b6..d068a25b9f4 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -257,3 +257,44 @@ proc test_pending_resolved { } {
 with_test_prefix "test_pending_resolved" {
     test_pending_resolved
 }
+
+# Test auto-disable is effective when notifying breakpoint-modified.
+
+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
+	}
+	-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..857480d4b61 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -799,6 +799,28 @@ proc_with_prefix test_catchpoints {} {
     gdb_test "continue" "Stopped at catchpoint event: ${num}"
 }
 
+# Test auto-disable is effective when notifying breakpoint_modified.
+
+proc_with_prefix test_bkpt_auto_disable { } {
+    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_no_output "enable count 1 2" "one shot enable"
+    gdb_test_no_output "python gdb.events.breakpoint_modified.connect(lambda bp: print(bp.enabled))" \
+	"trap breakpoint_modified event"
+    gdb_test "continue" "False.*" "auto-disabling after enable count reached"
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -815,3 +837,4 @@ test_bkpt_events
 test_bkpt_explicit_loc
 test_bkpt_qualified
 test_bkpt_probe
+test_bkpt_auto_disable
-- 
2.31.1

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

[Simon Marchi]

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

[PATCH] Notify observer of breakpoint auto-disabling

[Patrick Monnerat]

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.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
---
 gdb/breakpoint.c                              |  2 +-
 .../gdb.mi/mi-breakpoint-changed.exp          | 41 +++++++++++++++++++
 gdb/testsuite/gdb.python/py-breakpoint.exp    | 21 ++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 89af44ee4c6..feca224ccf4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
 	  if (bs->stop)
 	    {
 	      ++(b->hit_count);
-	      gdb::observers::breakpoint_modified.notify (b);
 
 	      /* We will stop here.  */
 	      if (b->disposition == disp_disable)
@@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
+	      gdb::observers::breakpoint_modified.notify (b);
 	      if (b->silent)
 		bs->print = 0;
 	      bs->commands = b->commands;
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index 3f7ef8a79b6..0b814443b06 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -257,3 +257,44 @@ proc test_pending_resolved { } {
 with_test_prefix "test_pending_resolved" {
     test_pending_resolved
 }
+
+# Test auto-disable is effective when notifying breakpoint-modified.
+
+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
+        }
+        -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 { } {
+    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."
+    gdb_test "python gdb.events.breakpoint_modified.connect(lambda bp: print(bp.enabled))" \
+	"" "Trap breakpoint_modified event"
+    gdb_test "continue" "False.*" "auto-disabling after enable count reached"
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -815,3 +835,4 @@ test_bkpt_events
 test_bkpt_explicit_loc
 test_bkpt_qualified
 test_bkpt_probe
+test_bkpt_auto_disable
-- 
2.31.1

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

[Patrick Monnerat]

On 8/13/21 8:59 PM, Simon Marchi wrote:
> On 2021-08-13 12:27 p.m., Patrick Monnerat wrote:
>> On 8/13/21 5:45 PM, Simon Marchi wrote:
>>> On 2021-08-13 11:31 a.m., Patrick Monnerat via Gdb-patches wrote:
>>>> As observer in currently notified of breakpoint stop before handling its
>>>> auto-disabling after count is reached, the observer is never notified of
>>>> the disabling.
>>>>
>>>> This patch moves the observer notification after the auto-disabling
>>>> code.
>>>>
>>>> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
>>>>
>>>> * gdb/breakpoint.c (bpstat_stop_status): move observer notification
>>>>     after auto-disabling code.
>>>> ---
>>>>    gdb/breakpoint.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>>> index 89af44ee4c6..feca224ccf4 100644
>>>> --- a/gdb/breakpoint.c
>>>> +++ b/gdb/breakpoint.c
>>>> @@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
>>>>          if (bs->stop)
>>>>            {
>>>>              ++(b->hit_count);
>>>> -          gdb::observers::breakpoint_modified.notify (b);
>>>>                /* We will stop here.  */
>>>>              if (b->disposition == disp_disable)
>>>> @@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
>>>>                b->enable_state = bp_disabled;
>>>>              removed_any = 1;
>>>>            }
>>>> +          gdb::observers::breakpoint_modified.notify (b);
>>>>              if (b->silent)
>>>>            bs->print = 0;
>>>>              bs->commands = b->commands;
>>>>
>>> Is there some user-visible behavior change, that we could write a test for?
>> With gdb alone, I don't think so.
>>
>> This impacts insight: the breakpoint GUI window does not refllect the disabling.
>>
>> No possible workaround here, since the observer is never notified again after breakpoint has been disabled (unless some user action on the later).
>>
>> As this is a GUI behavior, I don't think we can write a gdb test in this context.
> Ok, thanks.  I think this information should be included in the commit
> message, as it justifies the need for the change.

I will re-post patch with commit message updated.

Patrick

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

[Simon Marchi]

On 2021-08-13 12:27 p.m., Patrick Monnerat wrote:
> On 8/13/21 5:45 PM, Simon Marchi wrote:
>>
>> On 2021-08-13 11:31 a.m., Patrick Monnerat via Gdb-patches wrote:
>>> As observer in currently notified of breakpoint stop before handling its
>>> auto-disabling after count is reached, the observer is never notified of
>>> the disabling.
>>>
>>> This patch moves the observer notification after the auto-disabling
>>> code.
>>>
>>> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
>>>
>>> * gdb/breakpoint.c (bpstat_stop_status): move observer notification
>>>    after auto-disabling code.
>>> ---
>>>   gdb/breakpoint.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index 89af44ee4c6..feca224ccf4 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
>>>         if (bs->stop)
>>>           {
>>>             ++(b->hit_count);
>>> -          gdb::observers::breakpoint_modified.notify (b);
>>>               /* We will stop here.  */
>>>             if (b->disposition == disp_disable)
>>> @@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
>>>               b->enable_state = bp_disabled;
>>>             removed_any = 1;
>>>           }
>>> +          gdb::observers::breakpoint_modified.notify (b);
>>>             if (b->silent)
>>>           bs->print = 0;
>>>             bs->commands = b->commands;
>>>
>> Is there some user-visible behavior change, that we could write a test for?
> 
> With gdb alone, I don't think so.
> 
> This impacts insight: the breakpoint GUI window does not refllect the disabling.
> 
> No possible workaround here, since the observer is never notified again after breakpoint has been disabled (unless some user action on the later).
> 
> As this is a GUI behavior, I don't think we can write a gdb test in this context.

Ok, thanks.  I think this information should be included in the commit
message, as it justifies the need for the change.

Simon

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

[Patrick Monnerat]

On 8/13/21 5:45 PM, Simon Marchi wrote:
>
> On 2021-08-13 11:31 a.m., Patrick Monnerat via Gdb-patches wrote:
>> As observer in currently notified of breakpoint stop before handling its
>> auto-disabling after count is reached, the observer is never notified of
>> the disabling.
>>
>> This patch moves the observer notification after the auto-disabling
>> code.
>>
>> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
>>
>> * gdb/breakpoint.c (bpstat_stop_status): move observer notification
>>    after auto-disabling code.
>> ---
>>   gdb/breakpoint.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 89af44ee4c6..feca224ccf4 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
>>   	  if (bs->stop)
>>   	    {
>>   	      ++(b->hit_count);
>> -	      gdb::observers::breakpoint_modified.notify (b);
>>   
>>   	      /* We will stop here.  */
>>   	      if (b->disposition == disp_disable)
>> @@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
>>   		    b->enable_state = bp_disabled;
>>   		  removed_any = 1;
>>   		}
>> +	      gdb::observers::breakpoint_modified.notify (b);
>>   	      if (b->silent)
>>   		bs->print = 0;
>>   	      bs->commands = b->commands;
>>
> Is there some user-visible behavior change, that we could write a test for?

With gdb alone, I don't think so.

This impacts insight: the breakpoint GUI window does not refllect the 
disabling.

No possible workaround here, since the observer is never notified again 
after breakpoint has been disabled (unless some user action on the later).

As this is a GUI behavior, I don't think we can write a gdb test in this 
context.

Patrick

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

[Simon Marchi]

On 2021-08-13 11:31 a.m., Patrick Monnerat via Gdb-patches wrote:
> As observer in currently notified of breakpoint stop before handling its
> auto-disabling after count is reached, the observer is never notified of
> the disabling.
> 
> This patch moves the observer notification after the auto-disabling
> code.
> 
> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336
> 
> * gdb/breakpoint.c (bpstat_stop_status): move observer notification
>   after auto-disabling code.
> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 89af44ee4c6..feca224ccf4 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
>  	  if (bs->stop)
>  	    {
>  	      ++(b->hit_count);
> -	      gdb::observers::breakpoint_modified.notify (b);
>  
>  	      /* We will stop here.  */
>  	      if (b->disposition == disp_disable)
> @@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
>  		    b->enable_state = bp_disabled;
>  		  removed_any = 1;
>  		}
> +	      gdb::observers::breakpoint_modified.notify (b);
>  	      if (b->silent)
>  		bs->print = 0;
>  	      bs->commands = b->commands;
> 

Is there some user-visible behavior change, that we could write a test for?

Simon

[PATCH] Notify observer of breakpoint auto-disabling

[Patrick Monnerat]

As observer in currently notified of breakpoint stop before handling its
auto-disabling after count is reached, the observer is never notified of
the disabling.

This patch moves the observer notification after the auto-disabling
code.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=23336

* gdb/breakpoint.c (bpstat_stop_status): move observer notification
  after auto-disabling code.
---
 gdb/breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 89af44ee4c6..feca224ccf4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5491,7 +5491,6 @@ bpstat_stop_status (const address_space *aspace,
 	  if (bs->stop)
 	    {
 	      ++(b->hit_count);
-	      gdb::observers::breakpoint_modified.notify (b);
 
 	      /* We will stop here.  */
 	      if (b->disposition == disp_disable)
@@ -5501,6 +5500,7 @@ bpstat_stop_status (const address_space *aspace,
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
+	      gdb::observers::breakpoint_modified.notify (b);
 	      if (b->silent)
 		bs->print = 0;
 	      bs->commands = b->commands;
-- 
2.31.1