[gdb/breakpoints] Fix gdb.base/scope-hw-watch-disable.exp on arm-linux

  On arm-linux, with test-case gdb.base/scope-hw-watch-disable.exp I run into:
...
(gdb) awatch a^M
Can't set read/access watchpoint when hardware watchpoints are disabled.^M
(gdb) PASS: $exp: unsuccessful attempt to create an access watchpoint
rwatch b^M
Can't set read/access watchpoint when hardware watchpoints are disabled.^M
(gdb) PASS: $exp: unsuccessful attempt to create a read watchpoint
continue^M
Continuing.^M
^M
Program received signal SIGSEGV, Segmentation fault.^M
0xf7ec82c8 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6^M
(gdb) FAIL: $exp: continue until exit
...

Using "maint info break", we can see that the two failed attempts to set a
watchpoint each left behind a stale "watchpoint scope" breakpoint:
...
-5      watchpoint scope del  y   0xf7ec569a  inf 1
-5.1                          y   0xf7ec569a  inf 1
	stop only in stack frame at 0xfffef4f8
-6      watchpoint scope del  y   0xf7ec569a  inf 1
-6.1                          y   0xf7ec569a  inf 1
	stop only in stack frame at 0xfffef4f8
...

The SIGSEGV is a consequence of the stale "watchpoint scope" breakpoint: the
same happens if we:
- have can-use-hw-watchpoints == 1,
- set one of the watchpoints, and
- continue to exit.
The problem is missing symbol info on libc which is supposed to tell which
code is thumb.  After doing "set arm fallback-mode thumb" the SIGSEGV
disappears.

Extend the test-case to check the "maint info break" command before and after
the two failed attempts, to make sure that we catch the stale
"watchpoint scope" breakpoints also on x86_64-linux.

Fix this in watch_command_1 by making sure that the "watchpoint scope"
breakpoints are cleaned up when update_watchpoint throws.

Tested on arm-linux and x86_64-linux.

PR breakpoints/31860
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31860
---
 gdb/breakpoint.c                              | 20 ++++++++++++++++++-
 .../gdb.base/scope-hw-watch-disable.exp       | 18 +++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)


base-commit: 195795b0fd408f23520f0c4b43057d8311bcf92d
  

Tom de Vries <tdevries@suse.de> writes:

> On arm-linux, with test-case gdb.base/scope-hw-watch-disable.exp I run into:
> ...
> (gdb) awatch a^M
> Can't set read/access watchpoint when hardware watchpoints are disabled.^M
> (gdb) PASS: $exp: unsuccessful attempt to create an access watchpoint
> rwatch b^M
> Can't set read/access watchpoint when hardware watchpoints are disabled.^M
> (gdb) PASS: $exp: unsuccessful attempt to create a read watchpoint
> continue^M
> Continuing.^M
> ^M
> Program received signal SIGSEGV, Segmentation fault.^M
> 0xf7ec82c8 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6^M
> (gdb) FAIL: $exp: continue until exit
> ...
>
> Using "maint info break", we can see that the two failed attempts to set a
> watchpoint each left behind a stale "watchpoint scope" breakpoint:
> ...
> -5      watchpoint scope del  y   0xf7ec569a  inf 1
> -5.1                          y   0xf7ec569a  inf 1
> 	stop only in stack frame at 0xfffef4f8
> -6      watchpoint scope del  y   0xf7ec569a  inf 1
> -6.1                          y   0xf7ec569a  inf 1
> 	stop only in stack frame at 0xfffef4f8
> ...
>
> The SIGSEGV is a consequence of the stale "watchpoint scope" breakpoint: the
> same happens if we:
> - have can-use-hw-watchpoints == 1,
> - set one of the watchpoints, and
> - continue to exit.
> The problem is missing symbol info on libc which is supposed to tell which
> code is thumb.  After doing "set arm fallback-mode thumb" the SIGSEGV
> disappears.
>
> Extend the test-case to check the "maint info break" command before and after
> the two failed attempts, to make sure that we catch the stale
> "watchpoint scope" breakpoints also on x86_64-linux.
>
> Fix this in watch_command_1 by making sure that the "watchpoint scope"
> breakpoints are cleaned up when update_watchpoint throws.
>
> Tested on arm-linux and x86_64-linux.
>
> PR breakpoints/31860
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31860
> ---
>  gdb/breakpoint.c                              | 20 ++++++++++++++++++-
>  .../gdb.base/scope-hw-watch-disable.exp       | 18 +++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 17bd627f867..3d40116371a 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -10626,7 +10626,25 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
>  
>    /* Finally update the new watchpoint.  This creates the locations
>       that should be inserted.  */
> -  update_watchpoint (w.get (), true /* reparse */);
> +  try
> +    {
> +      update_watchpoint (w.get (), true /* reparse */);
> +    }
> +  catch (...)

I have a memory of being told one time that we should avoid catching
'...' unless we're e.g. wrapping a library that might throw other
exception types.  For general GDB code we should just catch
gdb_exception or a sub-class.  Any other exception is going to result in
GDB terminating anyway (we only catch gdb_exception & sub-types higher
up the call stack).

> +    {
> +      if (scope_breakpoint != NULL)
> +	{
> +	  /* Decouple the scope breakpoint.  */
> +	  w->related_breakpoint = w.get ();
> +	  scope_breakpoint->related_breakpoint = scope_breakpoint;
> +
> +	  /* Clean up the scope breakpoint.  The watchpoint is cleaned up on
> +	     scope exit.  */
> +	  delete_breakpoint (scope_breakpoint);

The problem with this approach is that we're catching gdb_exception_quit
and then doing substantial work -- deleting the breakpoint.  Deleting
the breakpoint can call into Python code, which might itself be an
issue.

I'm not suggesting that you necessarily need to change your approach.  I
think in this area GDB wasn't really written with solving this problem
in mind.  So possibly fixing it would require much bigger changes.  I
think the only solution would be to have GDB do the work, but not commit
the changes until all the work is done.  Then on a quit exception we'd
just leave the watchpoint unchanged as if we'd never started the update
at all.

Thanks,
Andrew

> +	}
> +
> +      throw;
> +    }
>  
>    install_breakpoint (internal, std::move (w), 1);
>  }
> diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> index 61137703e5c..29eb682df47 100644
> --- a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> +++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> @@ -29,6 +29,15 @@ if {![runto_main]} {
>      return -1
>  }
>  
> +gdb_test_multiple "maint info break" "maint info break before" {
> +    -re -wrap "watchpoint.*" {
> +	fail $gdb_test_name
> +    }
> +    -re -wrap "" {
> +	pass $gdb_test_name
> +    }
> +}
> +
>  gdb_test "awatch a" \
>      "Can't set read/access watchpoint when hardware watchpoints are disabled." \
>      "unsuccessful attempt to create an access watchpoint"
> @@ -36,5 +45,14 @@ gdb_test "rwatch b" \
>      "Can't set read/access watchpoint when hardware watchpoints are disabled." \
>      "unsuccessful attempt to create a read watchpoint"
>  
> +gdb_test_multiple "maint info break" "maint info break after" {
> +    -re -wrap "watchpoint.*" {
> +	fail $gdb_test_name
> +    }
> +    -re -wrap "" {
> +	pass $gdb_test_name
> +    }
> +}
> +
>  # The program continues until termination.
>  gdb_continue_to_end
>
> base-commit: 195795b0fd408f23520f0c4b43057d8311bcf92d
> -- 
> 2.35.3
  

On 8/21/24 10:19, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On arm-linux, with test-case gdb.base/scope-hw-watch-disable.exp I run into:
>> ...
>> (gdb) awatch a^M
>> Can't set read/access watchpoint when hardware watchpoints are disabled.^M
>> (gdb) PASS: $exp: unsuccessful attempt to create an access watchpoint
>> rwatch b^M
>> Can't set read/access watchpoint when hardware watchpoints are disabled.^M
>> (gdb) PASS: $exp: unsuccessful attempt to create a read watchpoint
>> continue^M
>> Continuing.^M
>> ^M
>> Program received signal SIGSEGV, Segmentation fault.^M
>> 0xf7ec82c8 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6^M
>> (gdb) FAIL: $exp: continue until exit
>> ...
>>
>> Using "maint info break", we can see that the two failed attempts to set a
>> watchpoint each left behind a stale "watchpoint scope" breakpoint:
>> ...
>> -5      watchpoint scope del  y   0xf7ec569a  inf 1
>> -5.1                          y   0xf7ec569a  inf 1
>> 	stop only in stack frame at 0xfffef4f8
>> -6      watchpoint scope del  y   0xf7ec569a  inf 1
>> -6.1                          y   0xf7ec569a  inf 1
>> 	stop only in stack frame at 0xfffef4f8
>> ...
>>
>> The SIGSEGV is a consequence of the stale "watchpoint scope" breakpoint: the
>> same happens if we:
>> - have can-use-hw-watchpoints == 1,
>> - set one of the watchpoints, and
>> - continue to exit.
>> The problem is missing symbol info on libc which is supposed to tell which
>> code is thumb.  After doing "set arm fallback-mode thumb" the SIGSEGV
>> disappears.
>>
>> Extend the test-case to check the "maint info break" command before and after
>> the two failed attempts, to make sure that we catch the stale
>> "watchpoint scope" breakpoints also on x86_64-linux.
>>
>> Fix this in watch_command_1 by making sure that the "watchpoint scope"
>> breakpoints are cleaned up when update_watchpoint throws.
>>
>> Tested on arm-linux and x86_64-linux.
>>
>> PR breakpoints/31860
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31860
>> ---
>>   gdb/breakpoint.c                              | 20 ++++++++++++++++++-
>>   .../gdb.base/scope-hw-watch-disable.exp       | 18 +++++++++++++++++
>>   2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 17bd627f867..3d40116371a 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -10626,7 +10626,25 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
>>   
>>     /* Finally update the new watchpoint.  This creates the locations
>>        that should be inserted.  */
>> -  update_watchpoint (w.get (), true /* reparse */);
>> +  try
>> +    {
>> +      update_watchpoint (w.get (), true /* reparse */);
>> +    }
>> +  catch (...)
> 

Hi Andrew,

thanks for the review.

> I have a memory of being told one time that we should avoid catching
> '...' unless we're e.g. wrapping a library that might throw other
> exception types.  For general GDB code we should just catch
> gdb_exception or a sub-class.  Any other exception is going to result in
> GDB terminating anyway (we only catch gdb_exception & sub-types higher
> up the call stack).
> 

Thanks for pointing that out.

I've started reviewing the source code for "catch (...)" and found at 
least one that I wrote (disasm-selftests.c) that probably shouldn't be 
there, so I'll try to submit a patch for that one.

I also found one in py-type.c, I'll try that one as well, though it's 
more complicated.

>> +    {
>> +      if (scope_breakpoint != NULL)
>> +	{
>> +	  /* Decouple the scope breakpoint.  */
>> +	  w->related_breakpoint = w.get ();
>> +	  scope_breakpoint->related_breakpoint = scope_breakpoint;
>> +
>> +	  /* Clean up the scope breakpoint.  The watchpoint is cleaned up on
>> +	     scope exit.  */
>> +	  delete_breakpoint (scope_breakpoint);
> 
> The problem with this approach is that we're catching gdb_exception_quit
> and then doing substantial work -- deleting the breakpoint.  Deleting
> the breakpoint can call into Python code, which might itself be an
> issue.
> 

Hmm, I didn't consider that, that's a problem indeed.

> I'm not suggesting that you necessarily need to change your approach.  I
> think in this area GDB wasn't really written with solving this problem
> in mind.  So possibly fixing it would require much bigger changes.  I
> think the only solution would be to have GDB do the work, but not commit
> the changes until all the work is done.  Then on a quit exception we'd
> just leave the watchpoint unchanged as if we'd never started the update
> at all.
> 

I think I found a way to deal with this without calling 
delete_breakpoint: moving the creation of scope breakpoint to after 
update_watchpoint.

Initially I was thrown off by the comment:
...
   Create the scope breakpoint before the watchpoint, so
   that we will encounter it first in bpstat_stop_status
...
but that is about order in the breakpoint_chain, and the proposed change 
doesn't change that order.

Since this is not obvious, I've added asserts to verify this ordering 
property.

Submitted as v2 ( 
https://sourceware.org/pipermail/gdb-patches/2024-August/211226.html ).

Thanks,
- Tom

> Thanks,
> Andrew
> 
>> +	}
>> +
>> +      throw;
>> +    }
>>   
>>     install_breakpoint (internal, std::move (w), 1);
>>   }
>> diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
>> index 61137703e5c..29eb682df47 100644
>> --- a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
>> +++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
>> @@ -29,6 +29,15 @@ if {![runto_main]} {
>>       return -1
>>   }
>>   
>> +gdb_test_multiple "maint info break" "maint info break before" {
>> +    -re -wrap "watchpoint.*" {
>> +	fail $gdb_test_name
>> +    }
>> +    -re -wrap "" {
>> +	pass $gdb_test_name
>> +    }
>> +}
>> +
>>   gdb_test "awatch a" \
>>       "Can't set read/access watchpoint when hardware watchpoints are disabled." \
>>       "unsuccessful attempt to create an access watchpoint"
>> @@ -36,5 +45,14 @@ gdb_test "rwatch b" \
>>       "Can't set read/access watchpoint when hardware watchpoints are disabled." \
>>       "unsuccessful attempt to create a read watchpoint"
>>   
>> +gdb_test_multiple "maint info break" "maint info break after" {
>> +    -re -wrap "watchpoint.*" {
>> +	fail $gdb_test_name
>> +    }
>> +    -re -wrap "" {
>> +	pass $gdb_test_name
>> +    }
>> +}
>> +
>>   # The program continues until termination.
>>   gdb_continue_to_end
>>
>> base-commit: 195795b0fd408f23520f0c4b43057d8311bcf92d
>> -- 
>> 2.35.3
>