[PATCH] [regression] Do not read from catchpoint/watchpoint locations' addresses when checking for a permanent breakpoint

[PATCH] [regression] Do not read from catchpoint/watchpoint locations' addresses when checking for a permanent breakpoint

[Luis Machado]

While running bare-metal tests with GDB i noticed some failures in gdb.base/break.exp, related to the use of the catch commands.

It turns out GDB tries to access memory address 0x0 whenever one tries to insert a catchpoint, which should obviously not happen.

This was introduced with the changes for permanent breakpoints. In special, bp_loc_is_permanent tries to check if there is a breakpoint inserted at the same address as the current breakpoint's location's address. In the case of catchpoints, this is 0x0.

(top-gdb) catch fork
Sending packet: $m0,1#fa...Packet received: E01
Catchpoint 4 (fork)

(top-gdb) catch vfork
Sending packet: $m0,1#fa...Packet received: E01
Catchpoint 5 (vfork)

It is not obvious to detect because this fails silently for Linux. For our bare-metal testing, though, this fails with a clear error message from the target about not being able to read such address.

The attached patch addresses this by bailing out of bp_loc_is_permanent (...) if the location address is not meaningful. I also took the opportunity to update the comment for breakpoint_address_is_meaningful, which mentioned breakpoint addresses as opposed to their locations' addresses.

Is this OK?

Luis

2015-08-11  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (bp_loc_is_permanent): Return 0 when breakpoint
	location address is not meaningful.
	(breakpoint_address_is_meaningful): Update comment.
---
 gdb/breakpoint.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 91a53b9..94f4ee6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6930,14 +6930,14 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 \f
 
 /* Return true iff it is meaningful to use the address member of
-   BPT.  For some breakpoint types, the address member is irrelevant
-   and it makes no sense to attempt to compare it to other addresses
-   (or use it for any other purpose either).
+   BPT locations.  For some breakpoint types, the locations' address members
+   are irrelevant and it makes no sense to attempt to compare it to other
+   addresses (or use it for any other purpose either).
 
    More specifically, each of the following breakpoint types will
-   always have a zero valued address and we don't want to mark
+   always have a zero valued location address and we don't want to mark
    breakpoints of any of these types to be a duplicate of an actual
-   breakpoint at address zero:
+   breakpoint location at address zero:
 
       bp_watchpoint
       bp_catchpoint
@@ -8974,6 +8974,13 @@ bp_loc_is_permanent (struct bp_location *loc)
 
   gdb_assert (loc != NULL);
 
+  /* If we have a catchpoint or a watchpoint, just return 0.  We should not
+     attempt to read from the addresses the locations of these breakpoint types
+     point to.  program_breakpoint_here_p, below, will attempt to read
+     memory.  */
+  if (breakpoint_address_is_meaningful (loc->owner);
+    return 0;
+
   cleanup = save_current_space_and_thread ();
   switch_to_program_space_and_thread (loc->pspace);
 
-- 
1.9.1

Re: [PATCH] [regression] Do not read from catchpoint/watchpoint locations' addresses when checking for a permanent breakpoint

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]

On 08/11/2015 11:30 AM, Luis Machado wrote:
> While running bare-metal tests with GDB i noticed some failures in gdb.base/break.exp, related to the use of the catch commands.
>
> It turns out GDB tries to access memory address 0x0 whenever one tries to insert a catchpoint, which should obviously not happen.
>
> This was introduced with the changes for permanent breakpoints. In special, bp_loc_is_permanent tries to check if there is a breakpoint inserted at the same address as the current breakpoint's location's address. In the case of catchpoints, this is 0x0.
>
> (top-gdb) catch fork
> Sending packet: $m0,1#fa...Packet received: E01
> Catchpoint 4 (fork)
>
> (top-gdb) catch vfork
> Sending packet: $m0,1#fa...Packet received: E01
> Catchpoint 5 (vfork)
>
> It is not obvious to detect because this fails silently for Linux. For our bare-metal testing, though, this fails with a clear error message from the target about not being able to read such address.
>
> The attached patch addresses this by bailing out of bp_loc_is_permanent (...) if the location address is not meaningful. I also took the opportunity to update the comment for breakpoint_address_is_meaningful, which mentioned breakpoint addresses as opposed to their locations' addresses.
>
> Is this OK?
>
> Luis
>
> 2015-08-11  Luis Machado  <lgustavo@codesourcery.com>
>
> 	* breakpoint.c (bp_loc_is_permanent): Return 0 when breakpoint
> 	location address is not meaningful.
> 	(breakpoint_address_is_meaningful): Update comment.
> ---
>   gdb/breakpoint.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 91a53b9..94f4ee6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6930,14 +6930,14 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
>   \f
>
>   /* Return true iff it is meaningful to use the address member of
> -   BPT.  For some breakpoint types, the address member is irrelevant
> -   and it makes no sense to attempt to compare it to other addresses
> -   (or use it for any other purpose either).
> +   BPT locations.  For some breakpoint types, the locations' address members
> +   are irrelevant and it makes no sense to attempt to compare it to other
> +   addresses (or use it for any other purpose either).
>
>      More specifically, each of the following breakpoint types will
> -   always have a zero valued address and we don't want to mark
> +   always have a zero valued location address and we don't want to mark
>      breakpoints of any of these types to be a duplicate of an actual
> -   breakpoint at address zero:
> +   breakpoint location at address zero:
>
>         bp_watchpoint
>         bp_catchpoint
> @@ -8974,6 +8974,13 @@ bp_loc_is_permanent (struct bp_location *loc)
>
>     gdb_assert (loc != NULL);
>
> +  /* If we have a catchpoint or a watchpoint, just return 0.  We should not
> +     attempt to read from the addresses the locations of these breakpoint types
> +     point to.  program_breakpoint_here_p, below, will attempt to read
> +     memory.  */
> +  if (breakpoint_address_is_meaningful (loc->owner);
> +    return 0;
> +
>     cleanup = save_current_space_and_thread ();
>     switch_to_program_space_and_thread (loc->pspace);
>
>

Don Breazeal has let me know about a typo in the patch above (thanks!), 
but turns out it doesn't even build properly. I had a different change 
and tested that one, but ended up changing the patch at the last minute. 
That's what you get!

Attached is an updated patch that does the right thing (and even builds!).

Luis

[-- Attachment #2: catch_regression.diff --]
[-- Type: text/x-patch, Size: 1904 bytes --]

2015-08-11  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (bp_loc_is_permanent): Return 0 when breakpoint
	location address is not meaningful.
	(breakpoint_address_is_meaningful): Update comment.
---
 gdb/breakpoint.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 91a53b9..dcc9e03 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6930,14 +6930,14 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 \f
 
 /* Return true iff it is meaningful to use the address member of
-   BPT.  For some breakpoint types, the address member is irrelevant
-   and it makes no sense to attempt to compare it to other addresses
-   (or use it for any other purpose either).
+   BPT locations.  For some breakpoint types, the locations' address members
+   are irrelevant and it makes no sense to attempt to compare them to other
+   addresses (or use them for any other purpose either).
 
    More specifically, each of the following breakpoint types will
-   always have a zero valued address and we don't want to mark
+   always have a zero valued location address and we don't want to mark
    breakpoints of any of these types to be a duplicate of an actual
-   breakpoint at address zero:
+   breakpoint location at address zero:
 
       bp_watchpoint
       bp_catchpoint
@@ -8974,6 +8974,13 @@ bp_loc_is_permanent (struct bp_location *loc)
 
   gdb_assert (loc != NULL);
 
+  /* If we have a catchpoint or a watchpoint, just return 0.  We should not
+     attempt to read from the addresses the locations of these breakpoint types
+     point to.  program_breakpoint_here_p, below, will attempt to read
+     memory.  */
+  if (!breakpoint_address_is_meaningful (loc->owner))
+    return 0;
+
   cleanup = save_current_space_and_thread ();
   switch_to_program_space_and_thread (loc->pspace);
 
-- 
1.9.1

Re: [PATCH] [regression] Do not read from catchpoint/watchpoint locations' addresses when checking for a permanent breakpoint

[Pedro Alves]

On 08/11/2015 03:30 PM, Luis Machado wrote:

> Is this OK?

OK for master and 7.10.

Thanks,
Pedro Alves

Re: [PATCH] [regression] Do not read from catchpoint/watchpoint locations' addresses when checking for a permanent breakpoint

[Luis Machado]

On 08/11/2015 03:03 PM, Pedro Alves wrote:
> On 08/11/2015 03:30 PM, Luis Machado wrote:
>
>> Is this OK?
>
> OK for master and 7.10.
>
> Thanks,
> Pedro Alves
>

Thanks Pedro.

Pushed to both:

master: 244558af868d5427903c35c5105bf5499639f81f
7.10: 835001623351330d49678d215d1338c0ce35c1f9