[PATCH] breakpoint: Make sure location types match before swapping

[PATCH] breakpoint: Make sure location types match before swapping

[Keno Fischer]

This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1".
In particular, what occurs in that case is that a hardware breakpoint is hit,
after which GDB removes it and establishes a single step breakpoint at the
same location. Afterwards, rather than simply removing this breakpoint and
re-enabling the hardware breakpoint, GDB simply swaps the activation, without
informing the server, leading to an inconsistency in GDB's view of the world
and the server's view of the world. To remidy this situation, this
patch adds a check that ensures two breakpoint locations have the
same type before they are considered equal and thus eligible for silent
swapping.

gdb/ChangeLog:
	* breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 gdb/breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461b..582dae1946 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1,
     /* We compare bp_location.length in order to cover ranged breakpoints.  */
     return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
 				     loc2->pspace->aspace, loc2->address)
-	    && loc1->length == loc2->length);
+	    && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type);
 }
 
 static void
-- 
2.24.0

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Keno Fischer]

Bump. It would be great to get this fixed.

On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> wrote:
>
> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1".
> In particular, what occurs in that case is that a hardware breakpoint is hit,
> after which GDB removes it and establishes a single step breakpoint at the
> same location. Afterwards, rather than simply removing this breakpoint and
> re-enabling the hardware breakpoint, GDB simply swaps the activation, without
> informing the server, leading to an inconsistency in GDB's view of the world
> and the server's view of the world. To remidy this situation, this
> patch adds a check that ensures two breakpoint locations have the
> same type before they are considered equal and thus eligible for silent
> swapping.
>
> gdb/ChangeLog:
>         * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e49025461b..582dae1946 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1,
>      /* We compare bp_location.length in order to cover ranged breakpoints.  */
>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>                                      loc2->pspace->aspace, loc2->address)
> -           && loc1->length == loc2->length);
> +           && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type);
>  }
>
>  static void
> --
> 2.24.0
>

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Simon Marchi]

On 2020-04-14 3:01 a.m., Keno Fischer wrote:
> Bump. It would be great to get this fixed.
> 
> On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> wrote:
>>
>> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1".
>> In particular, what occurs in that case is that a hardware breakpoint is hit,
>> after which GDB removes it and establishes a single step breakpoint at the
>> same location. Afterwards, rather than simply removing this breakpoint and
>> re-enabling the hardware breakpoint, GDB simply swaps the activation, without
>> informing the server, leading to an inconsistency in GDB's view of the world
>> and the server's view of the world. To remidy this situation, this
>> patch adds a check that ensures two breakpoint locations have the
>> same type before they are considered equal and thus eligible for silent
>> swapping.
>>
>> gdb/ChangeLog:
>>         * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741
>>
>> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
>> ---
>>  gdb/breakpoint.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index e49025461b..582dae1946 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1,
>>      /* We compare bp_location.length in order to cover ranged breakpoints.  */
>>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>>                                      loc2->pspace->aspace, loc2->address)
>> -           && loc1->length == loc2->length);
>> +           && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type);
>>  }
>>
>>  static void
>> --
>> 2.24.0
>>

I think the change makes sense, but this is not an area I know well, and it's one that
is a bit sensitive.  I'll do a full test run and take a bit more time to look at it.

In the mean time, if anybody else wants to take a look, go for it.

Simon

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2020-04-14 11:04:47 -0400]:

> On 2020-04-14 3:01 a.m., Keno Fischer wrote:
> > Bump. It would be great to get this fixed.
> > 
> > On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> wrote:
> >>
> >> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1".
> >> In particular, what occurs in that case is that a hardware breakpoint is hit,
> >> after which GDB removes it and establishes a single step breakpoint at the
> >> same location. Afterwards, rather than simply removing this breakpoint and
> >> re-enabling the hardware breakpoint, GDB simply swaps the activation, without
> >> informing the server, leading to an inconsistency in GDB's view of the world
> >> and the server's view of the world. To remidy this situation, this
> >> patch adds a check that ensures two breakpoint locations have the
> >> same type before they are considered equal and thus eligible for silent
> >> swapping.
> >>
> >> gdb/ChangeLog:
> >>         * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741
> >>
> >> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> >> ---
> >>  gdb/breakpoint.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> >> index e49025461b..582dae1946 100644
> >> --- a/gdb/breakpoint.c
> >> +++ b/gdb/breakpoint.c
> >> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1,
> >>      /* We compare bp_location.length in order to cover ranged breakpoints.  */
> >>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
> >>                                      loc2->pspace->aspace, loc2->address)
> >> -           && loc1->length == loc2->length);
> >> +           && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type);
> >>  }
> >>
> >>  static void
> >> --
> >> 2.24.0
> >>
> 
> I think the change makes sense, but this is not an area I know well, and it's one that
> is a bit sensitive.  I'll do a full test run and take a bit more time to look at it.
> 
> In the mean time, if anybody else wants to take a look, go for it.

I was able to reproduce this issue with a hacked up RISC-V target
(forced software single step on for bare-metal) running on a
development board I have access too.

I agree that this fix feels like the right thing to do, it's inline
with location checking done in watchpoint_locations_match.  The only
question I had when comparing to watchpoint_locations_match, is
whether we should be similarly comparing the types of the owner
breakpoint rather than the actual location.  However, the b/p type is
overloaded to contain more than just whether the breakpoint is h/w or
s/w, so in this case we have the user h/w breakpoint, its type is
bp_hardware_breakpoint, while the software single step breakpoint has
type bp_single_step.  The conclusion I came to is that it really is
the type of the location we need to compare here.

The other slight issue I had with the patch was that based on the
original description I still had to go and figure out the exact
conditions that would trigger this bug.  I believe that to trigger
this you need a h/w breakpoint placed on an instruction that loops to
itself, I don't see how else this could happen.

I took a crack at writing a more detailed break down of the conditions
that trigger this bug.

I'm still running this through testing here, but I'd be interested to
hear if you think the fuller description is helpful.

Thanks,
Andrew

---

From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
From: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Tue, 14 Apr 2020 16:32:02 +0100
Subject: [PATCH] gdb: ensure location types match before swapping locations

In the following conditions:

  - A target with hardware breakpoints available, and
  - A target that uses software single stepping,
  - An instruction at ADDRESS loops back to itself,

Now consider the following steps:

  1. The user places a hardware breakpoint at ADDRESS (an instruction
  that loops to itself),

  2. The inferior runs and hits the breakpoint from #1,

  3. The user tells GDB to 'continue'.

In #3 when the user tells GDB to continue, GDB first disables the
hardware breakpoint at ADDRESS, and then inserts a software single
step breakpoint at ADDRESS.  The original user created breakpoint was
a hardware breakpoint, while the single step breakpoint will be a
software breakpoint.

GDB continues and immediately hits the software single step
breakpoint.

GDB then deletes the software single step breakpoint by calling
delete_single_step_breakpoints, which eventually calls
delete_breakpoint, which, once the breakpoint (and its locations) are
deleted, calls update_global_location_list.

During update_global_location_list GDB spots that we have an old
location (the software single step breakpoint location) that is
inserted, but being deleted, and a location at the same address which
we are keeping, but which is not currently inserted (the original
hardware breakpoint location), GDB then calls
breakpoint_locations_match on these two locations.  Currently the
locations do match, and so GDB calls swap_insertion which swaps the
"inserted" state of the two locations.  GDB finally returns through
the call stack and leaves delete_single_step_breakpoints.  After this
GDB continues with its normal "stopping" process, as part of this
stopping process GDB removes all the breakpoints from the target.  Due
to the swap the original hardware breakpoint location is removed.

The problem is that GDB inserted the software single step breakpoint
as a software breakpoint, and then, thanks to the swap, tries to
remove it as a hardware breakpoint.  This will leave the target in an
inconsistent state, and as in the original bug report (PR gdb/25741),
could result in the target throwing an error.

The solution for all this is to have two breakpoint locations of
different types (hardware breakpoint vs software breakpoint) not
match.  The result of this is that GDB will no longer swap the
inserted status of the two breakpoints.  The original call to
update_global_location_list (after deleting the software single step
breakpoint) will at this point trigger a call to remove the
breakpoint, something which will be done based on the type of that
location.  Later GDB will see that the original hardware breakpoint is
already not inserted, and so will leave it alone.

This patch was original proposed by Keno Fischer here:

  https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html

gdb/ChangeLog:

	PR gdb/25741
	* breakpoint.c (breakpoint_locations_match): Compare location
	types.
---
 gdb/ChangeLog    | 6 ++++++
 gdb/breakpoint.c | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..2ab40a8e40a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6838,7 +6838,8 @@ breakpoint_locations_match (struct bp_location *loc1,
     /* We compare bp_location.length in order to cover ranged breakpoints.  */
     return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
 				     loc2->pspace->aspace, loc2->address)
-	    && loc1->length == loc2->length);
+	    && loc1->length == loc2->length
+	    && loc1->loc_type == loc2->loc_type);
 }
 
 static void
-- 
2.25.2

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Keno Fischer]

Thanks for the detailed write up - I wasn't quite sure of the exact
circumstances that trigger it, since in my case it involved an external
signal handler that was rewriting register state. That did in effect lead
to the instruction looping back on itself.

On Tue, Apr 14, 2020, 12:01 Andrew Burgess <andrew.burgess@embecosm.com>
wrote:

> * Simon Marchi <simark@simark.ca> [2020-04-14 11:04:47 -0400]:
>
> > On 2020-04-14 3:01 a.m., Keno Fischer wrote:
> > > Bump. It would be great to get this fixed.
> > >
> > > On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com>
> wrote:
> > >>
> > >> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0,
> but remove it using z1".
> > >> In particular, what occurs in that case is that a hardware breakpoint
> is hit,
> > >> after which GDB removes it and establishes a single step breakpoint
> at the
> > >> same location. Afterwards, rather than simply removing this
> breakpoint and
> > >> re-enabling the hardware breakpoint, GDB simply swaps the activation,
> without
> > >> informing the server, leading to an inconsistency in GDB's view of
> the world
> > >> and the server's view of the world. To remidy this situation, this
> > >> patch adds a check that ensures two breakpoint locations have the
> > >> same type before they are considered equal and thus eligible for
> silent
> > >> swapping.
> > >>
> > >> gdb/ChangeLog:
> > >>         * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741
> > >>
> > >> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > >> ---
> > >>  gdb/breakpoint.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> > >> index e49025461b..582dae1946 100644
> > >> --- a/gdb/breakpoint.c
> > >> +++ b/gdb/breakpoint.c
> > >> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location
> *loc1,
> > >>      /* We compare bp_location.length in order to cover ranged
> breakpoints.  */
> > >>      return (breakpoint_address_match (loc1->pspace->aspace,
> loc1->address,
> > >>                                      loc2->pspace->aspace,
> loc2->address)
> > >> -           && loc1->length == loc2->length);
> > >> +           && loc1->length == loc2->length && loc1->loc_type ==
> loc2->loc_type);
> > >>  }
> > >>
> > >>  static void
> > >> --
> > >> 2.24.0
> > >>
> >
> > I think the change makes sense, but this is not an area I know well, and
> it's one that
> > is a bit sensitive.  I'll do a full test run and take a bit more time to
> look at it.
> >
> > In the mean time, if anybody else wants to take a look, go for it.
>
> I was able to reproduce this issue with a hacked up RISC-V target
> (forced software single step on for bare-metal) running on a
> development board I have access too.
>
> I agree that this fix feels like the right thing to do, it's inline
> with location checking done in watchpoint_locations_match.  The only
> question I had when comparing to watchpoint_locations_match, is
> whether we should be similarly comparing the types of the owner
> breakpoint rather than the actual location.  However, the b/p type is
> overloaded to contain more than just whether the breakpoint is h/w or
> s/w, so in this case we have the user h/w breakpoint, its type is
> bp_hardware_breakpoint, while the software single step breakpoint has
> type bp_single_step.  The conclusion I came to is that it really is
> the type of the location we need to compare here.
>
> The other slight issue I had with the patch was that based on the
> original description I still had to go and figure out the exact
> conditions that would trigger this bug.  I believe that to trigger
> this you need a h/w breakpoint placed on an instruction that loops to
> itself, I don't see how else this could happen.
>
> I took a crack at writing a more detailed break down of the conditions
> that trigger this bug.
>
> I'm still running this through testing here, but I'd be interested to
> hear if you think the fuller description is helpful.
>
> Thanks,
> Andrew
>
> ---
>
> From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Tue, 14 Apr 2020 16:32:02 +0100
> Subject: [PATCH] gdb: ensure location types match before swapping locations
>
> In the following conditions:
>
>   - A target with hardware breakpoints available, and
>   - A target that uses software single stepping,
>   - An instruction at ADDRESS loops back to itself,
>
> Now consider the following steps:
>
>   1. The user places a hardware breakpoint at ADDRESS (an instruction
>   that loops to itself),
>
>   2. The inferior runs and hits the breakpoint from #1,
>
>   3. The user tells GDB to 'continue'.
>
> In #3 when the user tells GDB to continue, GDB first disables the
> hardware breakpoint at ADDRESS, and then inserts a software single
> step breakpoint at ADDRESS.  The original user created breakpoint was
> a hardware breakpoint, while the single step breakpoint will be a
> software breakpoint.
>
> GDB continues and immediately hits the software single step
> breakpoint.
>
> GDB then deletes the software single step breakpoint by calling
> delete_single_step_breakpoints, which eventually calls
> delete_breakpoint, which, once the breakpoint (and its locations) are
> deleted, calls update_global_location_list.
>
> During update_global_location_list GDB spots that we have an old
> location (the software single step breakpoint location) that is
> inserted, but being deleted, and a location at the same address which
> we are keeping, but which is not currently inserted (the original
> hardware breakpoint location), GDB then calls
> breakpoint_locations_match on these two locations.  Currently the
> locations do match, and so GDB calls swap_insertion which swaps the
> "inserted" state of the two locations.  GDB finally returns through
> the call stack and leaves delete_single_step_breakpoints.  After this
> GDB continues with its normal "stopping" process, as part of this
> stopping process GDB removes all the breakpoints from the target.  Due
> to the swap the original hardware breakpoint location is removed.
>
> The problem is that GDB inserted the software single step breakpoint
> as a software breakpoint, and then, thanks to the swap, tries to
> remove it as a hardware breakpoint.  This will leave the target in an
> inconsistent state, and as in the original bug report (PR gdb/25741),
> could result in the target throwing an error.
>
> The solution for all this is to have two breakpoint locations of
> different types (hardware breakpoint vs software breakpoint) not
> match.  The result of this is that GDB will no longer swap the
> inserted status of the two breakpoints.  The original call to
> update_global_location_list (after deleting the software single step
> breakpoint) will at this point trigger a call to remove the
> breakpoint, something which will be done based on the type of that
> location.  Later GDB will see that the original hardware breakpoint is
> already not inserted, and so will leave it alone.
>
> This patch was original proposed by Keno Fischer here:
>
>   https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
>
> gdb/ChangeLog:
>
>         PR gdb/25741
>         * breakpoint.c (breakpoint_locations_match): Compare location
>         types.
> ---
>  gdb/ChangeLog    | 6 ++++++
>  gdb/breakpoint.c | 3 ++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e49025461ba..2ab40a8e40a 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6838,7 +6838,8 @@ breakpoint_locations_match (struct bp_location *loc1,
>      /* We compare bp_location.length in order to cover ranged
> breakpoints.  */
>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>                                      loc2->pspace->aspace, loc2->address)
> -           && loc1->length == loc2->length);
> +           && loc1->length == loc2->length
> +           && loc1->loc_type == loc2->loc_type);
>  }
>
>  static void
> --
> 2.25.2
>
>

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Pedro Alves]

On 4/14/20 5:00 PM, Andrew Burgess wrote:

> The other slight issue I had with the patch was that based on the
> original description I still had to go and figure out the exact
> conditions that would trigger this bug.  I believe that to trigger
> this you need a h/w breakpoint placed on an instruction that loops to
> itself, I don't see how else this could happen.
> 
> I took a crack at writing a more detailed break down of the conditions
> that trigger this bug.
> 

> I'm still running this through testing here, but I'd be interested to
> hear if you think the fuller description is helpful.

It is.

> From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Tue, 14 Apr 2020 16:32:02 +0100
> Subject: [PATCH] gdb: ensure location types match before swapping locations
> 
> In the following conditions:
> 
>   - A target with hardware breakpoints available, and
>   - A target that uses software single stepping,
>   - An instruction at ADDRESS loops back to itself,
> 
> Now consider the following steps:
> 
>   1. The user places a hardware breakpoint at ADDRESS (an instruction
>   that loops to itself),
> 
>   2. The inferior runs and hits the breakpoint from #1,
> 
>   3. The user tells GDB to 'continue'.
> 
> In #3 when the user tells GDB to continue, GDB first disables the
> hardware breakpoint at ADDRESS, and then inserts a software single
> step breakpoint at ADDRESS.  The original user created breakpoint was
> a hardware breakpoint, while the single step breakpoint will be a
> software breakpoint.
> 
> GDB continues and immediately hits the software single step
> breakpoint.
> 
> GDB then deletes the software single step breakpoint by calling
> delete_single_step_breakpoints, which eventually calls
> delete_breakpoint, which, once the breakpoint (and its locations) are
> deleted, calls update_global_location_list.
> 
> During update_global_location_list GDB spots that we have an old
> location (the software single step breakpoint location) that is
> inserted, but being deleted, and a location at the same address which
> we are keeping, but which is not currently inserted (the original
> hardware breakpoint location), GDB then calls
> breakpoint_locations_match on these two locations.  Currently the
> locations do match, and so GDB calls swap_insertion which swaps the
> "inserted" state of the two locations.  GDB finally returns through
> the call stack and leaves delete_single_step_breakpoints.  After this
> GDB continues with its normal "stopping" process, as part of this
> stopping process GDB removes all the breakpoints from the target.  Due
> to the swap the original hardware breakpoint location is removed.
> 
> The problem is that GDB inserted the software single step breakpoint
> as a software breakpoint, and then, thanks to the swap, tries to
> remove it as a hardware breakpoint.  This will leave the target in an
> inconsistent state, and as in the original bug report (PR gdb/25741),
> could result in the target throwing an error.
> 
> The solution for all this is to have two breakpoint locations of
> different types (hardware breakpoint vs software breakpoint) not
> match.  The result of this is that GDB will no longer swap the
> inserted status of the two breakpoints.  The original call to
> update_global_location_list (after deleting the software single step
> breakpoint) will at this point trigger a call to remove the
> breakpoint, something which will be done based on the type of that
> location.  Later GDB will see that the original hardware breakpoint is
> already not inserted, and so will leave it alone.
> 
> This patch was original proposed by Keno Fischer here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
> 
> gdb/ChangeLog:
> 
> 	PR gdb/25741
> 	* breakpoint.c (breakpoint_locations_match): Compare location
> 	types.

This seems right at first blush, though there are some things
that we need to look at.  Here are 3 cases that found while
looking for breakpoint_locations_match callers:

#1

E.g., with this, GDB will now install both a hw breakpoint location
and a software location at the same address.  E.g., a contrived case
to see it happen would be, with:

 (gdb) set breakpoint always-inserted on
 (gdb) set debug remote 1

before:

 (gdb) hbreak main
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK
 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

after:

 (gdb) hbreak main
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK
 (gdb) break main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK
 Sending packet: $Z0,400736,1#47...Packet received: OK

This is likely not to cause a problem, but it's worth it to consider.

#2

Another thing to consider is the automatic hardware breakpoints
support.  See insert_bp_location.  That means that breakpoint
locations can start as software breakpoints but still end up
inserted as hw breakpoints.  Similarly to #1 above, without the patch,
gdb is considering those locations as duplicates, and after the
patch it no longer will.  So we may end up with multiple insertions
for the same location.  Again, similar to #1.

#3

I suspect this the (automatic hardware breakpoints) can interfere with
e.g., update_breakpoint_locations 's logic of matching old locations
with new locations, in the have_ambiguous_names case.  I.e., after
insertion the locations are hw breakpoint locations, but after
a breakpoint re-set, the new locations will be software breakpoint
locations until they try to be inserted.  Before the patch, the
disabled state will still be carried over.  After the patch, they won't.
I think.

Maybe we need to explicitly consider the case in
update_breakpoint_locations.

Thanks,
Pedro Alves

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]:

> On 4/14/20 5:00 PM, Andrew Burgess wrote:
> 
> > The other slight issue I had with the patch was that based on the
> > original description I still had to go and figure out the exact
> > conditions that would trigger this bug.  I believe that to trigger
> > this you need a h/w breakpoint placed on an instruction that loops to
> > itself, I don't see how else this could happen.
> > 
> > I took a crack at writing a more detailed break down of the conditions
> > that trigger this bug.
> > 
> 
> > I'm still running this through testing here, but I'd be interested to
> > hear if you think the fuller description is helpful.
> 
> It is.
> 
> > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
> > From: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date: Tue, 14 Apr 2020 16:32:02 +0100
> > Subject: [PATCH] gdb: ensure location types match before swapping locations
> > 
> > In the following conditions:
> > 
> >   - A target with hardware breakpoints available, and
> >   - A target that uses software single stepping,
> >   - An instruction at ADDRESS loops back to itself,
> > 
> > Now consider the following steps:
> > 
> >   1. The user places a hardware breakpoint at ADDRESS (an instruction
> >   that loops to itself),
> > 
> >   2. The inferior runs and hits the breakpoint from #1,
> > 
> >   3. The user tells GDB to 'continue'.
> > 
> > In #3 when the user tells GDB to continue, GDB first disables the
> > hardware breakpoint at ADDRESS, and then inserts a software single
> > step breakpoint at ADDRESS.  The original user created breakpoint was
> > a hardware breakpoint, while the single step breakpoint will be a
> > software breakpoint.
> > 
> > GDB continues and immediately hits the software single step
> > breakpoint.
> > 
> > GDB then deletes the software single step breakpoint by calling
> > delete_single_step_breakpoints, which eventually calls
> > delete_breakpoint, which, once the breakpoint (and its locations) are
> > deleted, calls update_global_location_list.
> > 
> > During update_global_location_list GDB spots that we have an old
> > location (the software single step breakpoint location) that is
> > inserted, but being deleted, and a location at the same address which
> > we are keeping, but which is not currently inserted (the original
> > hardware breakpoint location), GDB then calls
> > breakpoint_locations_match on these two locations.  Currently the
> > locations do match, and so GDB calls swap_insertion which swaps the
> > "inserted" state of the two locations.  GDB finally returns through
> > the call stack and leaves delete_single_step_breakpoints.  After this
> > GDB continues with its normal "stopping" process, as part of this
> > stopping process GDB removes all the breakpoints from the target.  Due
> > to the swap the original hardware breakpoint location is removed.
> > 
> > The problem is that GDB inserted the software single step breakpoint
> > as a software breakpoint, and then, thanks to the swap, tries to
> > remove it as a hardware breakpoint.  This will leave the target in an
> > inconsistent state, and as in the original bug report (PR gdb/25741),
> > could result in the target throwing an error.
> > 
> > The solution for all this is to have two breakpoint locations of
> > different types (hardware breakpoint vs software breakpoint) not
> > match.  The result of this is that GDB will no longer swap the
> > inserted status of the two breakpoints.  The original call to
> > update_global_location_list (after deleting the software single step
> > breakpoint) will at this point trigger a call to remove the
> > breakpoint, something which will be done based on the type of that
> > location.  Later GDB will see that the original hardware breakpoint is
> > already not inserted, and so will leave it alone.
> > 
> > This patch was original proposed by Keno Fischer here:
> > 
> >   https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
> > 
> > gdb/ChangeLog:
> > 
> > 	PR gdb/25741
> > 	* breakpoint.c (breakpoint_locations_match): Compare location
> > 	types.
> 
> This seems right at first blush, though there are some things
> that we need to look at.  Here are 3 cases that found while
> looking for breakpoint_locations_match callers:
> 
> #1
> 
> E.g., with this, GDB will now install both a hw breakpoint location
> and a software location at the same address.  E.g., a contrived case
> to see it happen would be, with:
> 
>  (gdb) set breakpoint always-inserted on
>  (gdb) set debug remote 1
> 
> before:
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) b main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
> 
> after:
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) break main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  Sending packet: $Z0,400736,1#47...Packet received: OK
> 
> This is likely not to cause a problem, but it's worth it to consider.
> 
> #2
> 
> Another thing to consider is the automatic hardware breakpoints
> support.  See insert_bp_location.  That means that breakpoint
> locations can start as software breakpoints but still end up
> inserted as hw breakpoints.  Similarly to #1 above, without the patch,
> gdb is considering those locations as duplicates, and after the
> patch it no longer will.  So we may end up with multiple insertions
> for the same location.  Again, similar to #1.
> 
> #3
> 
> I suspect this the (automatic hardware breakpoints) can interfere with
> e.g., update_breakpoint_locations 's logic of matching old locations
> with new locations, in the have_ambiguous_names case.  I.e., after
> insertion the locations are hw breakpoint locations, but after
> a breakpoint re-set, the new locations will be software breakpoint
> locations until they try to be inserted.  Before the patch, the
> disabled state will still be carried over.  After the patch, they won't.
> I think.
> 
> Maybe we need to explicitly consider the case in
> update_breakpoint_locations.

I'll take another look at this code, but it sounds almost like we
should split breakpoint_locations_match in two, we have:

  1. match (v1) - two breakpoints at the same address match regardless
  of type, this would be used when deciding if we should insert
  bp_location 1, 2, or both.

  2. match (v2) - when one breakpoint is already inserted, can another
  breakpoint "take over" its inserted status.

It's almost as though we want:

  bool
  breakpoint_locations_swapable ( .... )
  {
     return (breakpoint_locations_match ( .... )
             && loc1->type == loc2->type);
  }

Maybe?

I'll take another look at this when I can.

Thanks,
Andrew

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]:

> On 4/14/20 5:00 PM, Andrew Burgess wrote:
> 
> > The other slight issue I had with the patch was that based on the
> > original description I still had to go and figure out the exact
> > conditions that would trigger this bug.  I believe that to trigger
> > this you need a h/w breakpoint placed on an instruction that loops to
> > itself, I don't see how else this could happen.
> > 
> > I took a crack at writing a more detailed break down of the conditions
> > that trigger this bug.
> > 
> 
> > I'm still running this through testing here, but I'd be interested to
> > hear if you think the fuller description is helpful.
> 
> It is.
> 
> > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
> > From: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date: Tue, 14 Apr 2020 16:32:02 +0100
> > Subject: [PATCH] gdb: ensure location types match before swapping locations
> > 
> > In the following conditions:
> > 
> >   - A target with hardware breakpoints available, and
> >   - A target that uses software single stepping,
> >   - An instruction at ADDRESS loops back to itself,
> > 
> > Now consider the following steps:
> > 
> >   1. The user places a hardware breakpoint at ADDRESS (an instruction
> >   that loops to itself),
> > 
> >   2. The inferior runs and hits the breakpoint from #1,
> > 
> >   3. The user tells GDB to 'continue'.
> > 
> > In #3 when the user tells GDB to continue, GDB first disables the
> > hardware breakpoint at ADDRESS, and then inserts a software single
> > step breakpoint at ADDRESS.  The original user created breakpoint was
> > a hardware breakpoint, while the single step breakpoint will be a
> > software breakpoint.
> > 
> > GDB continues and immediately hits the software single step
> > breakpoint.
> > 
> > GDB then deletes the software single step breakpoint by calling
> > delete_single_step_breakpoints, which eventually calls
> > delete_breakpoint, which, once the breakpoint (and its locations) are
> > deleted, calls update_global_location_list.
> > 
> > During update_global_location_list GDB spots that we have an old
> > location (the software single step breakpoint location) that is
> > inserted, but being deleted, and a location at the same address which
> > we are keeping, but which is not currently inserted (the original
> > hardware breakpoint location), GDB then calls
> > breakpoint_locations_match on these two locations.  Currently the
> > locations do match, and so GDB calls swap_insertion which swaps the
> > "inserted" state of the two locations.  GDB finally returns through
> > the call stack and leaves delete_single_step_breakpoints.  After this
> > GDB continues with its normal "stopping" process, as part of this
> > stopping process GDB removes all the breakpoints from the target.  Due
> > to the swap the original hardware breakpoint location is removed.
> > 
> > The problem is that GDB inserted the software single step breakpoint
> > as a software breakpoint, and then, thanks to the swap, tries to
> > remove it as a hardware breakpoint.  This will leave the target in an
> > inconsistent state, and as in the original bug report (PR gdb/25741),
> > could result in the target throwing an error.
> > 
> > The solution for all this is to have two breakpoint locations of
> > different types (hardware breakpoint vs software breakpoint) not
> > match.  The result of this is that GDB will no longer swap the
> > inserted status of the two breakpoints.  The original call to
> > update_global_location_list (after deleting the software single step
> > breakpoint) will at this point trigger a call to remove the
> > breakpoint, something which will be done based on the type of that
> > location.  Later GDB will see that the original hardware breakpoint is
> > already not inserted, and so will leave it alone.
> > 
> > This patch was original proposed by Keno Fischer here:
> > 
> >   https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
> > 
> > gdb/ChangeLog:
> > 
> > 	PR gdb/25741
> > 	* breakpoint.c (breakpoint_locations_match): Compare location
> > 	types.
> 
> This seems right at first blush, though there are some things
> that we need to look at.  Here are 3 cases that found while
> looking for breakpoint_locations_match callers:
> 
> #1
> 
> E.g., with this, GDB will now install both a hw breakpoint location
> and a software location at the same address.  E.g., a contrived case
> to see it happen would be, with:
> 
>  (gdb) set breakpoint always-inserted on
>  (gdb) set debug remote 1
> 
> before:
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) b main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
> 
> after:
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) break main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  Sending packet: $Z0,400736,1#47...Packet received: OK

While working on a revised patch I tried to reproduce this, and it's
most odd.  Notice that both before and after you have two Z1 packets,
you're inserting the hardware breakpoint twice with no intermediate
removal.

I don't see this behaviour, even on an unmodified GDB, and I know some
remove targets, for example OpenOCD, will sanity check against such
double insertions and throw an error.

Just though this was worth mentioning.

Anyway, here's a revised patch.  I've moved the location of the type
check so it is only done now for the swapping case.  This should
resolve all the concerns you raised, while still addressing the
original issue.  I updated the commit message a bit too.

What do you think?

Thanks,
Andrew

---

commit 10d62976088600ee39b9b828bae93f82a44a2974
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Apr 14 16:32:02 2020 +0100

    gdb: ensure location types match before swapping locations
    
    In the following conditions:
    
      - A target with hardware breakpoints available, and
      - A target that uses software single stepping,
      - An instruction at ADDRESS loops back to itself,
    
    Now consider the following steps:
    
      1. The user places a hardware breakpoint at ADDRESS (an instruction
      that loops to itself),
    
      2. The inferior runs and hits the breakpoint at ADDRESS,
    
      3. The user tells GDB to 'continue'.
    
    In #3 when the user tells GDB to continue, GDB first disables the
    hardware breakpoint at ADDRESS, and then inserts a software single
    step breakpoint at ADDRESS.  The original user created breakpoint was
    a hardware breakpoint, while the single step breakpoint will be a
    software breakpoint.
    
    GDB continues and immediately hits the software single step
    breakpoint.
    
    GDB then deletes the software single step breakpoint by calling
    delete_single_step_breakpoints, which eventually calls
    delete_breakpoint, which, once the breakpoint (and its locations) are
    deleted, calls update_global_location_list.
    
    During update_global_location_list GDB spots that we have an old
    location (the software single step breakpoint location) that is
    inserted, but being deleted, and a location (the original hardware
    breakpoint) at the same address which we are keeping, but which is not
    currently inserted, GDB then calls breakpoint_locations_match on these
    two locations.
    
    Currently the locations do match, and so GDB calls swap_insertion
    which swaps the "inserted" state of the two locations.  The user
    created hardware breakpoint is marked as inserted, while the GDB
    internal software single step breakpoint is now marked as not
    inserted.  After this GDB returns through the call stack and leaves
    delete_single_step_breakpoints.
    
    After this GDB continues with its normal "stopping" process, as part
    of this stopping process GDB removes all the breakpoints from the
    target.  Due to the swap it is now the user created hardware
    breakpoint that is marked as inserted, so it is this breakpoint GDB
    tries to remove.
    
    The problem is that GDB inserted the software single step breakpoint
    as a software breakpoint, but is now trying to remove the hardware
    breakpoint.  The problem is removing a software breakpoint is very
    different to removing a hardware breakpoint, this could result is some
    undetected undefined behaviour, or as in the original bug report (PR
    gdb/25741), could result in the target throwing an error.
    
    This original patch proposed by Keno Fischer is here:
    
      https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
    
    This patch modified breakpoint_locations_match so that two breakpoints
    of different types would no longer match.  This resolves the immediate
    problem described above, but introduces some other issues.
    
    The breakpoint_locations_match function is also used to control
    whether two breakpoints places at the same address should both be
    inserted or not.
    
    Consider this (slightly contrived) use case:
    
      (gdb) set debug remote 1
      (gdb) set breakpoint  always-inserted on
      (gdb) hbreak  MyFunc0
      Sending packet: $me0000202,2#84...Packet received: 0000
      Hardware assisted breakpoint 3 at 0xe0000202: file test.c, line 288.
      Sending packet: $m80000188,1#63...Packet received: 00
      Sending packet: $Z1,e0000202,2#ce...Packet received: OK
      (gdb) break MyFunc0
      Note: breakpoint 3 also set at pc 0xe0000202.
      Sending packet: $me0000202,2#84...Packet received: 0000
      Breakpoint 4 at 0xe0000202: file test.c, line 288.
      Sending packet: $m80000188,1#63...Packet received: 00
    
    Notice after setting the hardware breakpoint we see the Z1 packet
    sent, but after setting the software breakpoint there's no Z0 packet,
    GDB sees that the locations match and doesn't make the second
    insertion.  If we modify breakpoint_locations_match then we would
    insert both a hardware _and_ software breakpoint.  This probably
    doesn't matter, but is not ideal, so a solution that doesn't do this
    would be better.
    
    The proposal here is to focus specifically on the case where we are
    considering swapping the inserted status of two breakpoint locations,
    and moves the location type check out of breakpoint_locations_match,
    and into the caller.  This resolves the original issue, while avoiding
    the double insertion problem.
    
    gdb/ChangeLog:
    
            PR gdb/25741
            * breakpoint.c (breakpoint_locations_match): Compare location
            types.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..afd6459a634 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			      gdb_assert (is_hardware_watchpoint (loc2->owner));
 			      loc2->watchpoint_type = old_loc->watchpoint_type;
 			    }
+			  else if (is_breakpoint (old_loc->owner))
+			    {
+			      /* If we swap the inserted status of a
+				 hardware and software breakpoint then GDB
+				 will insert the breakpoint as one type,
+				 and later try to remove the breakpoint as
+				 the other type.  This is not good.  */
+			      gdb_assert (is_breakpoint (loc2->owner));
+			      if (old_loc->loc_type != loc2->loc_type)
+				continue;
+			    }
 
 			  /* loc2 is a duplicated location. We need to check
 			     if it should be inserted in case it will be

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Pedro Alves]

On 4/17/20 1:28 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]:
> 
>> On 4/14/20 5:00 PM, Andrew Burgess wrote:
>>
>>> The other slight issue I had with the patch was that based on the
>>> original description I still had to go and figure out the exact
>>> conditions that would trigger this bug.  I believe that to trigger
>>> this you need a h/w breakpoint placed on an instruction that loops to
>>> itself, I don't see how else this could happen.
>>>
>>> I took a crack at writing a more detailed break down of the conditions
>>> that trigger this bug.
>>>
>>
>>> I'm still running this through testing here, but I'd be interested to
>>> hear if you think the fuller description is helpful.
>>
>> It is.
>>
>>> From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
>>> From: Andrew Burgess <andrew.burgess@embecosm.com>
>>> Date: Tue, 14 Apr 2020 16:32:02 +0100
>>> Subject: [PATCH] gdb: ensure location types match before swapping locations
>>>
>>> In the following conditions:
>>>
>>>   - A target with hardware breakpoints available, and
>>>   - A target that uses software single stepping,
>>>   - An instruction at ADDRESS loops back to itself,
>>>
>>> Now consider the following steps:
>>>
>>>   1. The user places a hardware breakpoint at ADDRESS (an instruction
>>>   that loops to itself),
>>>
>>>   2. The inferior runs and hits the breakpoint from #1,
>>>
>>>   3. The user tells GDB to 'continue'.
>>>
>>> In #3 when the user tells GDB to continue, GDB first disables the
>>> hardware breakpoint at ADDRESS, and then inserts a software single
>>> step breakpoint at ADDRESS.  The original user created breakpoint was
>>> a hardware breakpoint, while the single step breakpoint will be a
>>> software breakpoint.
>>>
>>> GDB continues and immediately hits the software single step
>>> breakpoint.
>>>
>>> GDB then deletes the software single step breakpoint by calling
>>> delete_single_step_breakpoints, which eventually calls
>>> delete_breakpoint, which, once the breakpoint (and its locations) are
>>> deleted, calls update_global_location_list.
>>>
>>> During update_global_location_list GDB spots that we have an old
>>> location (the software single step breakpoint location) that is
>>> inserted, but being deleted, and a location at the same address which
>>> we are keeping, but which is not currently inserted (the original
>>> hardware breakpoint location), GDB then calls
>>> breakpoint_locations_match on these two locations.  Currently the
>>> locations do match, and so GDB calls swap_insertion which swaps the
>>> "inserted" state of the two locations.  GDB finally returns through
>>> the call stack and leaves delete_single_step_breakpoints.  After this
>>> GDB continues with its normal "stopping" process, as part of this
>>> stopping process GDB removes all the breakpoints from the target.  Due
>>> to the swap the original hardware breakpoint location is removed.
>>>
>>> The problem is that GDB inserted the software single step breakpoint
>>> as a software breakpoint, and then, thanks to the swap, tries to
>>> remove it as a hardware breakpoint.  This will leave the target in an
>>> inconsistent state, and as in the original bug report (PR gdb/25741),
>>> could result in the target throwing an error.
>>>
>>> The solution for all this is to have two breakpoint locations of
>>> different types (hardware breakpoint vs software breakpoint) not
>>> match.  The result of this is that GDB will no longer swap the
>>> inserted status of the two breakpoints.  The original call to
>>> update_global_location_list (after deleting the software single step
>>> breakpoint) will at this point trigger a call to remove the
>>> breakpoint, something which will be done based on the type of that
>>> location.  Later GDB will see that the original hardware breakpoint is
>>> already not inserted, and so will leave it alone.
>>>
>>> This patch was original proposed by Keno Fischer here:
>>>
>>>   https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
>>>
>>> gdb/ChangeLog:
>>>
>>> 	PR gdb/25741
>>> 	* breakpoint.c (breakpoint_locations_match): Compare location
>>> 	types.
>>
>> This seems right at first blush, though there are some things
>> that we need to look at.  Here are 3 cases that found while
>> looking for breakpoint_locations_match callers:
>>
>> #1
>>
>> E.g., with this, GDB will now install both a hw breakpoint location
>> and a software location at the same address.  E.g., a contrived case
>> to see it happen would be, with:
>>
>>  (gdb) set breakpoint always-inserted on
>>  (gdb) set debug remote 1
>>
>> before:
>>
>>  (gdb) hbreak main
>>  Sending packet: $m400736,1#fe...Packet received: 48
>>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>>  Sending packet: $Z1,400736,1#48...Packet received: OK
>>  (gdb) b main
>>  Note: breakpoint 1 also set at pc 0x400736.
>>  Sending packet: $m400736,1#fe...Packet received: 48
>>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>>  Sending packet: $Z1,400736,1#48...Packet received: OK
>>
>> after:
>>
>>  (gdb) hbreak main
>>  Sending packet: $m400736,1#fe...Packet received: 48
>>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>>  Sending packet: $Z1,400736,1#48...Packet received: OK
>>  (gdb) break main
>>  Note: breakpoint 1 also set at pc 0x400736.
>>  Sending packet: $m400736,1#fe...Packet received: 48
>>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>>  Sending packet: $Z1,400736,1#48...Packet received: OK
>>  Sending packet: $Z0,400736,1#47...Packet received: OK
> 
> While working on a revised patch I tried to reproduce this, and it's
> most odd.  Notice that both before and after you have two Z1 packets,
> you're inserting the hardware breakpoint twice with no intermediate
> removal.

That happens because when a software breakpoint location is created,
it is created with condition_changed == condition_modified.
Then, in update_global_location_list, we end up in 
force_breakpoint_reinsertion:

	  /* Check if this is a new/duplicated location or a duplicated
	     location that had its condition modified.  If so, we want to send
	     its condition to the target if evaluation of conditions is taking
	     place there.  */
	  if ((*loc2p)->condition_changed == condition_modified
	      && (last_addr != old_loc->address
		  || last_pspace_num != old_loc->pspace->num))
	    {
	      force_breakpoint_reinsertion (*loc2p);
	      last_pspace_num = old_loc->pspace->num;
	    }

force_breakpoint_reinsertion forces reinsertion of all breakpoint
locations at the same address.

> 
> I don't see this behaviour, even on an unmodified GDB, 

I was debugging against pristine master x86-64 gdbserver.

And:

 (gdb) show breakpoint condition-evaluation 
 Breakpoint condition evaluation mode is auto (currently target).

Maybe you tested against a different remote server which doesn't
support target-side condition evaluation?

> and I know some remove targets, for example OpenOCD, will sanity check against such
> double insertions and throw an error.

If they do that, they should be fixed.  z/Z packets are supposed to be
idempotent.  Double insertions are to be expected and should not cause
an error.

 @emph{Implementation notes: A remote target shall return an empty string
 for an unrecognized breakpoint or watchpoint packet @var{type}.  A
 remote target shall support either both or neither of a given
 @samp{Z@var{type}@dots{}} and @samp{z@var{type}@dots{}} packet pair.  To
 avoid potential problems with duplicate packets, the operations should
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 be implemented in an idempotent way.}
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We took advantage of that when we added support for target-side
breakpoint conditions.

> 
> Just though this was worth mentioning.
> 
> Anyway, here's a revised patch.  I've moved the location of the type
> check so it is only done now for the swapping case.  This should
> resolve all the concerns you raised, while still addressing the
> original issue.  I updated the commit message a bit too.
> 
> What do you think?

Let me try to find a bit of time to give this a think.

Thanks,
Pedro Alves

Re: [PATCH] breakpoint: Make sure location types match before swapping

[Pedro Alves]

On 4/17/20 1:28 PM, Andrew Burgess wrote:
> @@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  			      gdb_assert (is_hardware_watchpoint (loc2->owner));
>  			      loc2->watchpoint_type = old_loc->watchpoint_type;
>  			    }
> +			  else if (is_breakpoint (old_loc->owner))
> +			    {
> +			      /* If we swap the inserted status of a
> +				 hardware and software breakpoint then GDB
> +				 will insert the breakpoint as one type,
> +				 and later try to remove the breakpoint as
> +				 the other type.  This is not good.  */
> +			      gdb_assert (is_breakpoint (loc2->owner));
> +			      if (old_loc->loc_type != loc2->loc_type)
> +				continue;
> +			    }

Unfortunately this doesn't actually work correctly.

For example:

 $ gdb ~/gdb/tests/threads -ex "set breakpoint always-inserted on" -ex "tar rem :9999" -ex "set debug remote 1"
 ...
 (gdb) hbreak main
 Sending packet: $m400700,40#28...Packet received:  89e58b...
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

Note no Z0 was sent after "b main".  That's because the sw and
hw breakpoints are still considered duplicates.

But now, let's delete the hw breakpoint:

(gdb) del 1
Sending packet: $z1,400736,1#68...Packet received: OK

Note z1 (remove hw break) was sent, but Z0 (insert sw break) was not.
At this point, the target side has no breakpoint installed!
That is very wrong, because we have "set breakpoint always-inserted on".
If the target was running at this point (e.g., non-stop mode),
even with "set breakpoint always-inserted off", GDB should keep
breakpoints installed, otherwise threads would miss the breakpoint.
"set breakpoint always-inserted on" is just a handy way to emulate
that.

If we do any other operation that forces installing breakpoints,
then finally the Z0 is sent:

 (gdb) b main
 Note: breakpoint 2 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 3 at 0x400736: file threads.c, line 57.
 Sending packet: $Z0,400736,1#47...Packet received: OK
 (gdb) 

I spent a good while yesterday trying to make this all work
with GDB still considering hw breakpoints and sw breakpoints
as duplicates.

We also need to handle my concern about insert_bp_location
changing the type of breakpoint locations from sw breakpoint
to hw breakpoint, _after_ the update_global_location_list
sorted out which breakpoints are duplicates of which.

The result is that GDB behaves like this:

 (gdb) hbreak main
 Sending packet: $m400700,40#28...Packet received: 89e58b...
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) del 1
 Sending packet: $Z0,400736,1#47...Packet received: OK
 Sending packet: $z1,400736,1#68...Packet received: OK
 Sending packet: $Z0,400736,1#47...Packet received: OK

Note how after deleting breakpoint 1 (the hw break),
GDB first installs the sw breakpoint, and then deletes
the hw breakpoint.  This order is important to ensures that
at no moment could a thread miss breakpoint 2.  The final
Z0, is an artifact of the target-side condition handling.
We could get rid of that but I didn't try it.

It was only after all the effort that I realized that it's
pointless to try to make hw and sw breakpoint locations
match _if we always need to install the new sw break before
deleting the hw break_.  I mean, we go through all that
effort, but then there's always going to be a small window
where the remote target needs to be able to handle
a sw breakpoint and a hw breakpoint at the same address
anyway (e.g., handle target-side breakpoint conditions
correctly, handle target-side commands correctly, etc.)

So the patch below is the one I wrote yesterday.  It
wasn't finished, but was well advanced.  I'm posting
it for the archives and for discussion.  At this point,
I don't believe we should follow this patch's approach.
I'll send another reply with today's new patch.

From e2fe5095cc2e136dadbec373ebaed2938e282145 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 18 Apr 2020 23:28:46 +0100
Subject: [PATCH] Handle hw and sw breakpoints at the same address

---
 gdb/breakpoint.c | 314 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 236 insertions(+), 78 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..25ce3b9d217 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2405,6 +2405,8 @@ breakpoint_kind (struct bp_location *bl, CORE_ADDR *addr)
     return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
 }
 
+static void handle_automatic_hardware_breakpoints (bp_location *bl);
+
 /* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
    location.  Any error messages are printed to TMP_ERROR_STREAM; and
    DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -2454,69 +2456,6 @@ insert_bp_location (struct bp_location *bl,
   if (bl->loc_type == bp_loc_software_breakpoint
       || bl->loc_type == bp_loc_hardware_breakpoint)
     {
-      if (bl->owner->type != bp_hardware_breakpoint)
-	{
-	  /* If the explicitly specified breakpoint type
-	     is not hardware breakpoint, check the memory map to see
-	     if the breakpoint address is in read only memory or not.
-
-	     Two important cases are:
-	     - location type is not hardware breakpoint, memory
-	     is readonly.  We change the type of the location to
-	     hardware breakpoint.
-	     - location type is hardware breakpoint, memory is
-	     read-write.  This means we've previously made the
-	     location hardware one, but then the memory map changed,
-	     so we undo.
-	     
-	     When breakpoints are removed, remove_breakpoints will use
-	     location types we've just set here, the only possible
-	     problem is that memory map has changed during running
-	     program, but it's not going to work anyway with current
-	     gdb.  */
-	  struct mem_region *mr 
-	    = lookup_mem_region (bl->target_info.reqstd_address);
-	  
-	  if (mr)
-	    {
-	      if (automatic_hardware_breakpoints)
-		{
-		  enum bp_loc_type new_type;
-		  
-		  if (mr->attrib.mode != MEM_RW)
-		    new_type = bp_loc_hardware_breakpoint;
-		  else 
-		    new_type = bp_loc_software_breakpoint;
-		  
-		  if (new_type != bl->loc_type)
-		    {
-		      static int said = 0;
-
-		      bl->loc_type = new_type;
-		      if (!said)
-			{
-			  fprintf_filtered (gdb_stdout,
-					    _("Note: automatically using "
-					      "hardware breakpoints for "
-					      "read-only addresses.\n"));
-			  said = 1;
-			}
-		    }
-		}
-	      else if (bl->loc_type == bp_loc_software_breakpoint
-		       && mr->attrib.mode != MEM_RW)
-		{
-		  fprintf_unfiltered (tmp_error_stream,
-				      _("Cannot insert breakpoint %d.\n"
-					"Cannot set software breakpoint "
-					"at read-only address %s\n"),
-				      bl->owner->number,
-				      paddress (bl->gdbarch, bl->address));
-		  return 1;
-		}
-	    }
-	}
-        
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
 	  || bl->section == NULL
@@ -2830,7 +2769,11 @@ insert_breakpoints (void)
 
   /* Updating watchpoints creates new locations, so update the global
      location list.  Explicitly tell ugll to insert locations and
-     ignore breakpoints_always_inserted_mode.  */
+     ignore breakpoints_always_inserted_mode.  Also,
+     update_global_location_list tries to "upgrade" software
+     breakpoints to hardware breakpoints to handle "set breakpoint
+     auto-hw", so we need to call it even if we don't have new
+     locations.  */
   update_global_location_list (UGLL_INSERT);
 }
 
@@ -2905,6 +2848,22 @@ update_inserted_breakpoint_locations (void)
     }
 }
 
+static void
+breakpoint_error_stream (string_file *tmp_error_stream,
+			 int *hw_breakpoint_error,
+			 int *hw_bp_error_explained_already)
+{
+  /* If a hardware breakpoint or watchpoint was inserted, add a
+     message about possibly exhausted resources.  */
+  if (*hw_breakpoint_error && !*hw_bp_error_explained_already)
+    {
+      tmp_error_stream->printf ("Could not insert hardware breakpoints:\n\
+You may have requested too many hardware breakpoints/watchpoints.\n");
+    }
+  target_terminal::ours_for_output ();
+  error_stream (*tmp_error_stream);
+}
+
 /* Used when starting or continuing the program.  */
 
 static void
@@ -2991,17 +2950,9 @@ insert_breakpoint_locations (void)
     }
 
   if (error_flag)
-    {
-      /* If a hardware breakpoint or watchpoint was inserted, add a
-         message about possibly exhausted resources.  */
-      if (hw_breakpoint_error && !hw_bp_error_explained_already)
-	{
-	  tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\
-You may have requested too many hardware breakpoints/watchpoints.\n");
-	}
-      target_terminal::ours_for_output ();
-      error_stream (tmp_error_stream);
-    }
+    breakpoint_error_stream (&tmp_error_stream,
+			     &hw_breakpoint_error,
+			     &hw_bp_error_explained_already);
 }
 
 /* Used when the program stops.
@@ -8515,8 +8466,93 @@ mention (struct breakpoint *b)
 }
 \f
 
+static int
+bp_location_number (bp_location *loc)
+{
+  int n = 1;
+  for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
+    {
+      if (l == loc)
+	return n;
+      ++n;
+    }
+
+  gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
+}
+
 static bool bp_loc_is_permanent (struct bp_location *loc);
 
+static void
+handle_automatic_hardware_breakpoints (bp_location *bl)
+{
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
+    {
+      if (bl->owner->type != bp_hardware_breakpoint)
+	{
+	  /* If the explicitly specified breakpoint type is not
+	     hardware breakpoint, check the memory map to see if the
+	     breakpoint address is in read only memory or not.
+
+	     Two important cases are:
+	     - location type is not hardware breakpoint, memory is
+	     readonly.  We change the type of the location to hardware
+	     breakpoint.
+	     - location type is hardware breakpoint, memory is
+	     read-write.  This means we've previously made the
+	     location hardware one, but then the memory map changed,
+	     so we undo.
+
+	     When breakpoints are removed, remove_breakpoints will use
+	     location types we've just set here, the only possible
+	     problem is that memory map has changed during running
+	     program, but it's not going to work anyway with current
+	     gdb.  */
+	  struct mem_region *mr
+	    = lookup_mem_region (bl->target_info.reqstd_address);
+
+	  if (mr)
+	    {
+	      if (automatic_hardware_breakpoints)
+		{
+		  enum bp_loc_type new_type;
+
+		  if (mr->attrib.mode != MEM_RW)
+		    new_type = bp_loc_hardware_breakpoint;
+		  else
+		    new_type = bp_loc_software_breakpoint;
+
+		  if (new_type != bl->loc_type)
+		    {
+		      static int said = 0;
+
+		      bl->loc_type = new_type;
+		      if (!said)
+			{
+			  fprintf_filtered (gdb_stdout,
+					    _("Note: automatically using "
+					      "hardware breakpoints for "
+					      "read-only addresses.\n"));
+			  said = 1;
+			}
+		    }
+		}
+	      else if (bl->loc_type == bp_loc_software_breakpoint
+		       && mr->attrib.mode != MEM_RW)
+		{
+		  bl->enabled = false;
+
+		  warning (_("Breakpoint %d.%d disabled.\n"
+			    "Cannot set software breakpoint "
+			    "at read-only address %s\n"),
+			   bl->owner->number, bp_location_number (bl),
+			   paddress (bl->gdbarch, bl->address));
+		}
+	    }
+	}
+    }
+}
+
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
@@ -11451,6 +11487,29 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
   if (a->permanent != b->permanent)
     return a->permanent > b->permanent;
 
+  /* Sort hardware breakpoint locations before software breakpoint
+     locations, in order to prefer inserting hardware breakpoint
+     locations.  */
+  auto type_order = [](bp_loc_type loc_type)
+    {
+      switch (loc_type)
+	{
+	case bp_loc_hardware_breakpoint:
+	return 0;
+	case bp_loc_software_breakpoint:
+	return 1;
+	case bp_loc_hardware_watchpoint:
+	return 2;
+	case bp_loc_other:
+	return 3;
+	}
+
+      gdb_assert_not_reached ("bad switch");
+    };
+
+  if (type_order (a->loc_type) < type_order (b->loc_type))
+    return true;
+
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */
@@ -11625,6 +11684,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
       loc->cond_bytecode.reset ();
     }
 }
+
 /* Called whether new breakpoints are created, or existing breakpoints
    deleted, to update the global location list and recompute which
    locations are duplicate of which.
@@ -11654,6 +11714,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
   struct bp_location *awp_loc_first; /* access watchpoint */
   struct bp_location *rwp_loc_first; /* read watchpoint */
 
+  /* Used for the case we need to insert a sw breakpoint to replace a
+     hw breakpoint, and vice versa.  */
+  bool error_flag = false;
+  int disabled_breaks = 0;
+  int hw_breakpoint_error = 0;
+  int hw_bp_error_explained_already = 0;
+  string_file tmp_error_stream;
+  /* Explicitly mark the warning -- this will only
+     be printed if there was an error.  */
+  tmp_error_stream.puts ("Warning:\n");
+
   /* Saved former bp_locations array which we compare against the newly
      built bp_locations from the current state of ALL_BREAKPOINTS.  */
   struct bp_location **old_locp;
@@ -11673,6 +11744,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
   ALL_BREAKPOINTS (b)
     for (loc = b->loc; loc; loc = loc->next)
       *locp++ = loc;
+
+  /* See if we need to "upgrade" a software breakpoint to a hardware
+     breakpoint.  Do this before deciding whether locations are
+     duplicates.  Also do this before sorting because sorting order
+     depends on location type.  */
+  for (locp = bp_locations;
+       locp < bp_locations + bp_locations_count;
+       locp++)
+    {
+      loc = *locp;
+      if (!loc->inserted)
+	handle_automatic_hardware_breakpoints (loc);
+    }
+
   std::sort (bp_locations, bp_locations + bp_locations_count,
 	     bp_location_is_less_than);
 
@@ -11769,6 +11854,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	      /* OLD_LOC comes from existing struct breakpoint.  */
 	      if (bl_address_is_meaningful (old_loc))
 		{
+		  bp_location *replacement_loc = nullptr;
+
 		  for (loc2p = locp;
 		       (loc2p < bp_locations + bp_locations_count
 			&& (*loc2p)->address == old_loc->address);
@@ -11776,6 +11863,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 		    {
 		      struct bp_location *loc2 = *loc2p;
 
+		      if (loc2 == old_loc)
+			continue;
+
 		      if (breakpoint_locations_match (loc2, old_loc))
 			{
 			  /* Read watchpoint locations are switched to
@@ -11786,12 +11876,25 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			      gdb_assert (is_hardware_watchpoint (loc2->owner));
 			      loc2->watchpoint_type = old_loc->watchpoint_type;
 			    }
+			  else if ((old_loc->loc_type == bp_loc_hardware_breakpoint
+				    || old_loc->loc_type == bp_loc_software_breakpoint)
+				   && loc2->loc_type != old_loc->loc_type)
+			    {
+			      /* A hardware breakpoint is being
+				 removed.  Don't consider software
+				 breakpoint locations as duplicates,
+				 to avoid consuming hardware resources
+				 unnecessarily without user
+				 request.  */
+			      if (unduplicated_should_be_inserted (loc2))
+				replacement_loc = loc2;
+			      continue;
+			    }
 
 			  /* loc2 is a duplicated location. We need to check
 			     if it should be inserted in case it will be
 			     unduplicated.  */
-			  if (loc2 != old_loc
-			      && unduplicated_should_be_inserted (loc2))
+			  if (unduplicated_should_be_inserted (loc2))
 			    {
 			      swap_insertion (old_loc, loc2);
 			      keep_in_target = 1;
@@ -11799,6 +11902,29 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			    }
 			}
 		    }
+
+		  /* If we're deleting a harware breakpoint, and we
+		     have a replacement software breakpoint to install
+		     (or vice versa), we must install the new one
+		     before deleting the old one, so that the inferior
+		     doesn't miss a breakpoint in case it is running
+		     right now.  */
+		  if (!keep_in_target && replacement_loc != nullptr)
+		    {
+		      scoped_restore_current_pspace_and_thread restore_pspace_thread;
+
+		      switch_to_program_space_and_thread (replacement_loc->pspace);
+
+		      replacement_loc->duplicate = false;
+
+		      int val
+			= insert_bp_location (replacement_loc, &tmp_error_stream,
+					      &disabled_breaks,
+					      &hw_breakpoint_error,
+					      &hw_bp_error_explained_already);
+		      if (val)
+			error_flag = val;
+		    }
 		}
 	    }
 
@@ -11959,13 +12085,45 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	 is not duplicated, and is the inserted one.
 	 All following are marked as duplicated, and are not inserted.  */
       if (loc->inserted)
-	swap_insertion (loc, *loc_first_p);
+	{
+	  if (loc->loc_type == bp_loc_software_breakpoint
+	      && (*loc_first_p)->loc_type == bp_loc_hardware_breakpoint)
+	    {
+	      /* Insert the replacement breakpoint first, so that
+		 there's never a time the inferior could miss a
+		 breakpoint, in case the inferior is running right
+		 now.  */
+	      scoped_restore_current_pspace_and_thread restore_pspace_thread;
+
+	      switch_to_program_space_and_thread ((*loc_first_p)->pspace);
+
+	      int val
+		= insert_bp_location ((*loc_first_p), &tmp_error_stream,
+				      &disabled_breaks,
+				      &hw_breakpoint_error,
+				      &hw_bp_error_explained_already);
+	      if (val)
+		error_flag = val;
+
+	      remove_breakpoint (loc);
+	    }
+	  else
+	    swap_insertion (loc, *loc_first_p);
+	}
       loc->duplicate = 1;
 
       /* Clear the condition modification flag.  */
       loc->condition_changed = condition_unchanged;
     }
 
+  /* Should this be propagated down to insert_breakpoint_locations if
+     it's going to be called, so that we have a single place that
+     throws?  */
+  if (error_flag)
+    breakpoint_error_stream (&tmp_error_stream,
+			     &hw_breakpoint_error,
+			     &hw_bp_error_explained_already);
+
   if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())
     {
       if (insert_mode != UGLL_DONT_INSERT)

base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
-- 
2.14.5

[PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)

[Pedro Alves]

On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote:

> It was only after all the effort that I realized that it's
> pointless to try to make hw and sw breakpoint locations
> match _if we always need to install the new sw break before
> deleting the hw break_.  I mean, we go through all that
> effort, but then there's always going to be a small window
> where the remote target needs to be able to handle
> a sw breakpoint and a hw breakpoint at the same address
> anyway (e.g., handle target-side breakpoint conditions
> correctly, handle target-side commands correctly, etc.)
> 
> So the patch below is the one I wrote yesterday.  It
> wasn't finished, but was well advanced.  I'm posting
> it for the archives and for discussion.  At this point,
> I don't believe we should follow this patch's approach.
> I'll send another reply with today's new patch.

So here's my proposed patch.

This makes breakpoint_locations_match consider the location's
type too, like Keno's original patch.  But then does a bunch
more stuff to actually make that work correctly.

- We need to handle the duplicates detection better.  Take a look
  at the loop at the tail end of update_global_location_list.
  Currently, because breakpoint locations aren't sorted by type,
  we can end up with, at the same address, a sw break, then a hw break,
  then a sw break, etc.  The result is that that second sw break won't
  be considered a duplicate of the first sw break.  Seems like
  we already handle that incorrectly for range breakpoints.

- The "set breakpoint auto-hw" handling is moved out of insert_bp_location
  to update_global_location_list, before the duplicates determination.

  This change comes with one visible behavior change with
  "set breakpoint auto-hw off", however.  Here:

  Before:

   (gdb) break *0x400487
   Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
   (gdb) c
   Continuing.
   Warning:
   Cannot insert breakpoint 2.
   Cannot set software breakpoint at read-only address 0x400487

   Command aborted.
   (gdb)

  After:

   (gdb) break *0x400487
   Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
   warning: Breakpoint 2.1 disabled.
   Cannot set software breakpoint at read-only address 0x400487

  I.e., we now warn earlier, instead of waiting until resume time to error out.

- In update_breakpoint_locations, the logic of matching old locations
  with new locations, in the have_ambiguous_names case, is updated
  to still consider sw vs hw locations the same.

- I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any
  needed updating.  Note that that macro walks all locations at
  a given address, and doesn't call breakpoint_locations_match.

The result against GDBserver is this:

 (gdb) hbreak main
 Sending packet: $m400700,40#28...Packet received: 89e58b0....
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z0,400736,1#47...Packet received: OK
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) del 1
 Sending packet: $z1,400736,1#68...Packet received: OK
 Sending packet: $Z0,400736,1#47...Packet received: OK
 (gdb) 

Or, with "set breakpoint condition-evaluation host", 

 (gdb) hbreak main
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK
 (gdb) b main
 Note: breakpoint 3 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 4 at 0x400736: file threads.c, line 57.
 Sending packet: $Z0,400736,1#47...Packet received: OK
 (gdb) del 3
 Sending packet: $z1,400736,1#68...Packet received: OK

From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 19 Apr 2020 18:25:55 +0100
Subject: [PATCH] Stop considering hw and sw breakpoints duplicates

---
 gdb/breakpoint.c                                   | 269 ++++++++++++++-------
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |   2 +-
 2 files changed, 183 insertions(+), 88 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..27799e89807 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
 static int watchpoint_locations_match (struct bp_location *loc1,
 				       struct bp_location *loc2);
 
+static int breakpoint_locations_match (struct bp_location *loc1,
+				       struct bp_location *loc2,
+				       bool sw_hw_bps_match = false);
+
 static int breakpoint_location_address_match (struct bp_location *bl,
 					      const struct address_space *aspace,
 					      CORE_ADDR addr);
@@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl)
     return;
 
   /* Do a first pass to check for locations with no assigned
-     conditions or conditions that fail to parse to a valid agent expression
-     bytecode.  If any of these happen, then it's no use to send conditions
-     to the target since this location will always trigger and generate a
-     response back to GDB.  */
+     conditions or conditions that fail to parse to a valid agent
+     expression bytecode.  If any of these happen, then it's no use to
+     send conditions to the target since this location will always
+     trigger and generate a response back to GDB.  Note we consider
+     all locations at the same address irrespective of type, i.e.,
+     even if the locations aren't considered duplicates (e.g.,
+     software breakpoint and hardware breakpoint at the same
+     address).  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
@@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl)
 	}
     }
 
-  /* No NULL conditions or failed bytecode generation.  Build a condition list
-     for this location's address.  */
+  /* No NULL conditions or failed bytecode generation.  Build a
+     condition list for this location's address.  If we have software
+     and hardware locations at the same address, they aren't
+     considered duplicates, but we still marge all the conditions
+     anyway, as it's simpler, and doesn't really make a practical
+     difference.  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
@@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl)
   if (dprintf_style != dprintf_style_agent)
     return;
 
-  /* For now, if we have any duplicate location that isn't a dprintf,
-     don't install the target-side commands, as that would make the
-     breakpoint not be reported to the core, and we'd lose
+  /* For now, if we have any location at the same address that isn't a
+     dprintf, don't install the target-side commands, as that would
+     make the breakpoint not be reported to the core, and we'd lose
      control.  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
@@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl)
 	}
     }
 
-  /* No NULL commands or failed bytecode generation.  Build a command list
-     for this location's address.  */
+  /* No NULL commands or failed bytecode generation.  Build a command
+     list for all duplicate locations at this location's address.
+     Note that here we must care for whether the breakpoint location
+     types are considered duplicates, otherwise, say, if we have a
+     software and hardware location at the same address, the target
+     could end up running the commands twice.  For the moment, we only
+     support targets-side commands with dprintf, but it doesn't hurt
+     to be pedantically correct in case that changes.  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
-      if (loc->owner->extra_string
+      if (breakpoint_locations_match (bl, loc)
+	  && loc->owner->extra_string
 	  && is_breakpoint (loc->owner)
 	  && loc->pspace->num == bl->pspace->num
 	  && loc->owner->enable_state == bp_enabled
@@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl,
   if (bl->loc_type == bp_loc_software_breakpoint
       || bl->loc_type == bp_loc_hardware_breakpoint)
     {
-      if (bl->owner->type != bp_hardware_breakpoint)
-	{
-	  /* If the explicitly specified breakpoint type
-	     is not hardware breakpoint, check the memory map to see
-	     if the breakpoint address is in read only memory or not.
-
-	     Two important cases are:
-	     - location type is not hardware breakpoint, memory
-	     is readonly.  We change the type of the location to
-	     hardware breakpoint.
-	     - location type is hardware breakpoint, memory is
-	     read-write.  This means we've previously made the
-	     location hardware one, but then the memory map changed,
-	     so we undo.
-	     
-	     When breakpoints are removed, remove_breakpoints will use
-	     location types we've just set here, the only possible
-	     problem is that memory map has changed during running
-	     program, but it's not going to work anyway with current
-	     gdb.  */
-	  struct mem_region *mr 
-	    = lookup_mem_region (bl->target_info.reqstd_address);
-	  
-	  if (mr)
-	    {
-	      if (automatic_hardware_breakpoints)
-		{
-		  enum bp_loc_type new_type;
-		  
-		  if (mr->attrib.mode != MEM_RW)
-		    new_type = bp_loc_hardware_breakpoint;
-		  else 
-		    new_type = bp_loc_software_breakpoint;
-		  
-		  if (new_type != bl->loc_type)
-		    {
-		      static int said = 0;
-
-		      bl->loc_type = new_type;
-		      if (!said)
-			{
-			  fprintf_filtered (gdb_stdout,
-					    _("Note: automatically using "
-					      "hardware breakpoints for "
-					      "read-only addresses.\n"));
-			  said = 1;
-			}
-		    }
-		}
-	      else if (bl->loc_type == bp_loc_software_breakpoint
-		       && mr->attrib.mode != MEM_RW)
-		{
-		  fprintf_unfiltered (tmp_error_stream,
-				      _("Cannot insert breakpoint %d.\n"
-					"Cannot set software breakpoint "
-					"at read-only address %s\n"),
-				      bl->owner->number,
-				      paddress (bl->gdbarch, bl->address));
-		  return 1;
-		}
-	    }
-	}
-        
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
 	  || bl->section == NULL
@@ -2830,7 +2786,11 @@ insert_breakpoints (void)
 
   /* Updating watchpoints creates new locations, so update the global
      location list.  Explicitly tell ugll to insert locations and
-     ignore breakpoints_always_inserted_mode.  */
+     ignore breakpoints_always_inserted_mode.  Also,
+     update_global_location_list tries to "upgrade" software
+     breakpoints to hardware breakpoints to handle "set breakpoint
+     auto-hw", so we need to call it even if we don't have new
+     locations.  */
   update_global_location_list (UGLL_INSERT);
 }
 
@@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1,
 
 /* Assuming LOC1 and LOC2's types' have meaningful target addresses
    (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
-   the same location.  */
+   the same location.  If SW_HW_BPS_MATCH is true, then software
+   breakpoint locations and hardware breakpoint locations match,
+   otherwise they don't.  */
 
 static int
-breakpoint_locations_match (struct bp_location *loc1, 
-			    struct bp_location *loc2)
+breakpoint_locations_match (struct bp_location *loc1,
+			    struct bp_location *loc2,
+			    bool sw_hw_bps_match)
 {
   int hw_point1, hw_point2;
 
@@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1,
   else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
     return tracepoint_locations_match (loc1, loc2);
   else
-    /* We compare bp_location.length in order to cover ranged breakpoints.  */
+    /* We compare bp_location.length in order to cover ranged
+       breakpoints.  Keep this in sync with
+       bp_location_is_less_than.  */
     return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
 				     loc2->pspace->aspace, loc2->address)
+	    && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
 	    && loc1->length == loc2->length);
 }
 
@@ -8515,8 +8481,99 @@ mention (struct breakpoint *b)
 }
 \f
 
+/* Return the location number of LOC.  */
+
+static int
+bp_location_number (bp_location *loc)
+{
+  int n = 1;
+  for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
+    {
+      if (l == loc)
+	return n;
+      ++n;
+    }
+
+  gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
+}
+
 static bool bp_loc_is_permanent (struct bp_location *loc);
 
+/* Handle "set breakpoint auto-hw".
+
+   If the explicitly specified breakpoint type is not hardware
+   breakpoint, check the memory map to see whether the breakpoint
+   address is in read-only memory.
+
+   And then, if "set breakpoint auto-hw" is enabled:
+
+   - location type is not hardware breakpoint, memory is read-only.
+   We change the type of the location to hardware breakpoint.
+
+   - location type is hardware breakpoint, memory is read-write.  This
+   means we've previously made the location hardware one, but then the
+   memory map changed, so we undo.
+
+   However, if "set breakpoint auto-hw" is disabled, and memory is
+   read-only, we warn and disable the breakpoint location.  */
+
+static void
+handle_automatic_hardware_breakpoints (bp_location *bl)
+{
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
+    {
+      if (bl->owner->type != bp_hardware_breakpoint)
+	{
+	  /* When breakpoints are removed, remove_breakpoints will use
+	     location types we've just set here, the only possible
+	     problem is that memory map has changed during running
+	     program, but it's not going to work anyway with current
+	     gdb.  */
+	  mem_region *mr = lookup_mem_region (bl->address);
+
+	  if (mr != nullptr)
+	    {
+	      if (automatic_hardware_breakpoints)
+		{
+		  enum bp_loc_type new_type;
+
+		  if (mr->attrib.mode != MEM_RW)
+		    new_type = bp_loc_hardware_breakpoint;
+		  else
+		    new_type = bp_loc_software_breakpoint;
+
+		  if (new_type != bl->loc_type)
+		    {
+		      static bool said = false;
+
+		      bl->loc_type = new_type;
+		      if (!said)
+			{
+			  fprintf_filtered (gdb_stdout,
+					    _("Note: automatically using "
+					      "hardware breakpoints for "
+					      "read-only addresses.\n"));
+			  said = true;
+			}
+		    }
+		}
+	      else if (bl->loc_type == bp_loc_software_breakpoint
+		       && mr->attrib.mode != MEM_RW)
+		{
+		  bl->enabled = false;
+
+		  warning (_("Breakpoint %d.%d disabled.\n"
+			    "Cannot set software breakpoint "
+			    "at read-only address %s\n"),
+			   bl->owner->number, bp_location_number (bl),
+			   paddress (bl->gdbarch, bl->address));
+		}
+	    }
+	}
+    }
+}
+
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
@@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
   if (a->permanent != b->permanent)
     return a->permanent > b->permanent;
 
+  /* Sort by type in order to make duplicate determination easier.
+     See update_global_location_list.  This is kept in sync with
+     breakpoint_locations_match.  */
+  if (a->loc_type < b->loc_type)
+    return true;
+
+  /* Likewise, for range-breakpoints, sort by length.  */
+  if (a->loc_type == bp_loc_hardware_breakpoint
+      && b->loc_type == bp_loc_hardware_breakpoint
+      && a->length < b->length)
+    return true;
+
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */
@@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
       loc->cond_bytecode.reset ();
     }
 }
+
 /* Called whether new breakpoints are created, or existing breakpoints
    deleted, to update the global location list and recompute which
    locations are duplicate of which.
@@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
   ALL_BREAKPOINTS (b)
     for (loc = b->loc; loc; loc = loc->next)
       *locp++ = loc;
+
+  /* See if we need to "upgrade" a software breakpoint to a hardware
+     breakpoint.  Do this before deciding whether locations are
+     duplicates.  Also do this before sorting because sorting order
+     depends on location type.  */
+  for (locp = bp_locations;
+       locp < bp_locations + bp_locations_count;
+       locp++)
+    {
+      loc = *locp;
+      if (!loc->inserted)
+	handle_automatic_hardware_breakpoints (loc);
+    }
+
   std::sort (bp_locations, bp_locations + bp_locations_count,
 	     bp_location_is_less_than);
 
@@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 		    {
 		      struct bp_location *loc2 = *loc2p;
 
+		      if (loc2 == old_loc)
+			continue;
+
 		      if (breakpoint_locations_match (loc2, old_loc))
 			{
 			  /* Read watchpoint locations are switched to
@@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			  /* loc2 is a duplicated location. We need to check
 			     if it should be inserted in case it will be
 			     unduplicated.  */
-			  if (loc2 != old_loc
-			      && unduplicated_should_be_inserted (loc2))
+			  if (unduplicated_should_be_inserted (loc2))
 			    {
 			      swap_insertion (old_loc, loc2);
 			      keep_in_target = 1;
@@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b,
 	    if (have_ambiguous_names)
 	      {
 		for (; l; l = l->next)
-		  if (breakpoint_locations_match (e, l))
-		    {
-		      l->enabled = 0;
-		      break;
-		    }
+		  {
+		    /* Ignore software vs hardware location type at
+		       this point, because with "set breakpoint
+		       auto-hw", after a re-set, locations that were
+		       hardware can end up as software, or vice versa.
+		       As mentioned above, this is an heuristic and in
+		       practice should give the correct answer often
+		       enough.  */
+		    if (breakpoint_locations_match (e, l, true))
+		      {
+			l->enabled = 0;
+			break;
+		      }
+		  }
 	      }
 	    else
 	      {
diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
index 65f7baae9f5..b950e433ed8 100644
--- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
@@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \
 # Ensure that inserting a software breakpoint in a known-read-only
 # region fails.
 gdb_test "break *$main_lo" \
-    "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \
+    "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \
     "inserting software breakpoint in read-only memory fails"
 
 delete_breakpoints

base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
-- 
2.14.5

Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2020-04-19 19:49:12 +0100]:

> On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote:
> 
> > It was only after all the effort that I realized that it's
> > pointless to try to make hw and sw breakpoint locations
> > match _if we always need to install the new sw break before
> > deleting the hw break_.  I mean, we go through all that
> > effort, but then there's always going to be a small window
> > where the remote target needs to be able to handle
> > a sw breakpoint and a hw breakpoint at the same address
> > anyway (e.g., handle target-side breakpoint conditions
> > correctly, handle target-side commands correctly, etc.)
> > 
> > So the patch below is the one I wrote yesterday.  It
> > wasn't finished, but was well advanced.  I'm posting
> > it for the archives and for discussion.  At this point,
> > I don't believe we should follow this patch's approach.
> > I'll send another reply with today's new patch.
> 
> So here's my proposed patch.
> 
> This makes breakpoint_locations_match consider the location's
> type too, like Keno's original patch.  But then does a bunch
> more stuff to actually make that work correctly.

LGTM.

Thanks,
Andrew


> 
> - We need to handle the duplicates detection better.  Take a look
>   at the loop at the tail end of update_global_location_list.
>   Currently, because breakpoint locations aren't sorted by type,
>   we can end up with, at the same address, a sw break, then a hw break,
>   then a sw break, etc.  The result is that that second sw break won't
>   be considered a duplicate of the first sw break.  Seems like
>   we already handle that incorrectly for range breakpoints.
> 
> - The "set breakpoint auto-hw" handling is moved out of insert_bp_location
>   to update_global_location_list, before the duplicates determination.
> 
>   This change comes with one visible behavior change with
>   "set breakpoint auto-hw off", however.  Here:
> 
>   Before:
> 
>    (gdb) break *0x400487
>    Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
>    (gdb) c
>    Continuing.
>    Warning:
>    Cannot insert breakpoint 2.
>    Cannot set software breakpoint at read-only address 0x400487
> 
>    Command aborted.
>    (gdb)
> 
>   After:
> 
>    (gdb) break *0x400487
>    Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
>    warning: Breakpoint 2.1 disabled.
>    Cannot set software breakpoint at read-only address 0x400487
> 
>   I.e., we now warn earlier, instead of waiting until resume time to error out.
> 
> - In update_breakpoint_locations, the logic of matching old locations
>   with new locations, in the have_ambiguous_names case, is updated
>   to still consider sw vs hw locations the same.
> 
> - I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any
>   needed updating.  Note that that macro walks all locations at
>   a given address, and doesn't call breakpoint_locations_match.
> 
> The result against GDBserver is this:
> 
>  (gdb) hbreak main
>  Sending packet: $m400700,40#28...Packet received: 89e58b0....
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
> 
>  (gdb) b main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z0,400736,1#47...Packet received: OK
>  Sending packet: $Z1,400736,1#48...Packet received: OK
> 
>  (gdb) del 1
>  Sending packet: $z1,400736,1#68...Packet received: OK
>  Sending packet: $Z0,400736,1#47...Packet received: OK
>  (gdb) 
> 
> Or, with "set breakpoint condition-evaluation host", 
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) b main
>  Note: breakpoint 3 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 4 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z0,400736,1#47...Packet received: OK
>  (gdb) del 3
>  Sending packet: $z1,400736,1#68...Packet received: OK
> 
> From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 19 Apr 2020 18:25:55 +0100
> Subject: [PATCH] Stop considering hw and sw breakpoints duplicates
> 
> ---
>  gdb/breakpoint.c                                   | 269 ++++++++++++++-------
>  gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |   2 +-
>  2 files changed, 183 insertions(+), 88 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e49025461ba..27799e89807 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
>  static int watchpoint_locations_match (struct bp_location *loc1,
>  				       struct bp_location *loc2);
>  
> +static int breakpoint_locations_match (struct bp_location *loc1,
> +				       struct bp_location *loc2,
> +				       bool sw_hw_bps_match = false);
> +
>  static int breakpoint_location_address_match (struct bp_location *bl,
>  					      const struct address_space *aspace,
>  					      CORE_ADDR addr);
> @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl)
>      return;
>  
>    /* Do a first pass to check for locations with no assigned
> -     conditions or conditions that fail to parse to a valid agent expression
> -     bytecode.  If any of these happen, then it's no use to send conditions
> -     to the target since this location will always trigger and generate a
> -     response back to GDB.  */
> +     conditions or conditions that fail to parse to a valid agent
> +     expression bytecode.  If any of these happen, then it's no use to
> +     send conditions to the target since this location will always
> +     trigger and generate a response back to GDB.  Note we consider
> +     all locations at the same address irrespective of type, i.e.,
> +     even if the locations aren't considered duplicates (e.g.,
> +     software breakpoint and hardware breakpoint at the same
> +     address).  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl)
>  	}
>      }
>  
> -  /* No NULL conditions or failed bytecode generation.  Build a condition list
> -     for this location's address.  */
> +  /* No NULL conditions or failed bytecode generation.  Build a
> +     condition list for this location's address.  If we have software
> +     and hardware locations at the same address, they aren't
> +     considered duplicates, but we still marge all the conditions
> +     anyway, as it's simpler, and doesn't really make a practical
> +     difference.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl)
>    if (dprintf_style != dprintf_style_agent)
>      return;
>  
> -  /* For now, if we have any duplicate location that isn't a dprintf,
> -     don't install the target-side commands, as that would make the
> -     breakpoint not be reported to the core, and we'd lose
> +  /* For now, if we have any location at the same address that isn't a
> +     dprintf, don't install the target-side commands, as that would
> +     make the breakpoint not be reported to the core, and we'd lose
>       control.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
> @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl)
>  	}
>      }
>  
> -  /* No NULL commands or failed bytecode generation.  Build a command list
> -     for this location's address.  */
> +  /* No NULL commands or failed bytecode generation.  Build a command
> +     list for all duplicate locations at this location's address.
> +     Note that here we must care for whether the breakpoint location
> +     types are considered duplicates, otherwise, say, if we have a
> +     software and hardware location at the same address, the target
> +     could end up running the commands twice.  For the moment, we only
> +     support targets-side commands with dprintf, but it doesn't hurt
> +     to be pedantically correct in case that changes.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> -      if (loc->owner->extra_string
> +      if (breakpoint_locations_match (bl, loc)
> +	  && loc->owner->extra_string
>  	  && is_breakpoint (loc->owner)
>  	  && loc->pspace->num == bl->pspace->num
>  	  && loc->owner->enable_state == bp_enabled
> @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl,
>    if (bl->loc_type == bp_loc_software_breakpoint
>        || bl->loc_type == bp_loc_hardware_breakpoint)
>      {
> -      if (bl->owner->type != bp_hardware_breakpoint)
> -	{
> -	  /* If the explicitly specified breakpoint type
> -	     is not hardware breakpoint, check the memory map to see
> -	     if the breakpoint address is in read only memory or not.
> -
> -	     Two important cases are:
> -	     - location type is not hardware breakpoint, memory
> -	     is readonly.  We change the type of the location to
> -	     hardware breakpoint.
> -	     - location type is hardware breakpoint, memory is
> -	     read-write.  This means we've previously made the
> -	     location hardware one, but then the memory map changed,
> -	     so we undo.
> -	     
> -	     When breakpoints are removed, remove_breakpoints will use
> -	     location types we've just set here, the only possible
> -	     problem is that memory map has changed during running
> -	     program, but it's not going to work anyway with current
> -	     gdb.  */
> -	  struct mem_region *mr 
> -	    = lookup_mem_region (bl->target_info.reqstd_address);
> -	  
> -	  if (mr)
> -	    {
> -	      if (automatic_hardware_breakpoints)
> -		{
> -		  enum bp_loc_type new_type;
> -		  
> -		  if (mr->attrib.mode != MEM_RW)
> -		    new_type = bp_loc_hardware_breakpoint;
> -		  else 
> -		    new_type = bp_loc_software_breakpoint;
> -		  
> -		  if (new_type != bl->loc_type)
> -		    {
> -		      static int said = 0;
> -
> -		      bl->loc_type = new_type;
> -		      if (!said)
> -			{
> -			  fprintf_filtered (gdb_stdout,
> -					    _("Note: automatically using "
> -					      "hardware breakpoints for "
> -					      "read-only addresses.\n"));
> -			  said = 1;
> -			}
> -		    }
> -		}
> -	      else if (bl->loc_type == bp_loc_software_breakpoint
> -		       && mr->attrib.mode != MEM_RW)
> -		{
> -		  fprintf_unfiltered (tmp_error_stream,
> -				      _("Cannot insert breakpoint %d.\n"
> -					"Cannot set software breakpoint "
> -					"at read-only address %s\n"),
> -				      bl->owner->number,
> -				      paddress (bl->gdbarch, bl->address));
> -		  return 1;
> -		}
> -	    }
> -	}
> -        
>        /* First check to see if we have to handle an overlay.  */
>        if (overlay_debugging == ovly_off
>  	  || bl->section == NULL
> @@ -2830,7 +2786,11 @@ insert_breakpoints (void)
>  
>    /* Updating watchpoints creates new locations, so update the global
>       location list.  Explicitly tell ugll to insert locations and
> -     ignore breakpoints_always_inserted_mode.  */
> +     ignore breakpoints_always_inserted_mode.  Also,
> +     update_global_location_list tries to "upgrade" software
> +     breakpoints to hardware breakpoints to handle "set breakpoint
> +     auto-hw", so we need to call it even if we don't have new
> +     locations.  */
>    update_global_location_list (UGLL_INSERT);
>  }
>  
> @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1,
>  
>  /* Assuming LOC1 and LOC2's types' have meaningful target addresses
>     (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
> -   the same location.  */
> +   the same location.  If SW_HW_BPS_MATCH is true, then software
> +   breakpoint locations and hardware breakpoint locations match,
> +   otherwise they don't.  */
>  
>  static int
> -breakpoint_locations_match (struct bp_location *loc1, 
> -			    struct bp_location *loc2)
> +breakpoint_locations_match (struct bp_location *loc1,
> +			    struct bp_location *loc2,
> +			    bool sw_hw_bps_match)
>  {
>    int hw_point1, hw_point2;
>  
> @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1,
>    else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
>      return tracepoint_locations_match (loc1, loc2);
>    else
> -    /* We compare bp_location.length in order to cover ranged breakpoints.  */
> +    /* We compare bp_location.length in order to cover ranged
> +       breakpoints.  Keep this in sync with
> +       bp_location_is_less_than.  */
>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>  				     loc2->pspace->aspace, loc2->address)
> +	    && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
>  	    && loc1->length == loc2->length);
>  }
>  
> @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b)
>  }
>  \f
>  
> +/* Return the location number of LOC.  */
> +
> +static int
> +bp_location_number (bp_location *loc)
> +{
> +  int n = 1;
> +  for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
> +    {
> +      if (l == loc)
> +	return n;
> +      ++n;
> +    }
> +
> +  gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
> +}
> +
>  static bool bp_loc_is_permanent (struct bp_location *loc);
>  
> +/* Handle "set breakpoint auto-hw".
> +
> +   If the explicitly specified breakpoint type is not hardware
> +   breakpoint, check the memory map to see whether the breakpoint
> +   address is in read-only memory.
> +
> +   And then, if "set breakpoint auto-hw" is enabled:
> +
> +   - location type is not hardware breakpoint, memory is read-only.
> +   We change the type of the location to hardware breakpoint.
> +
> +   - location type is hardware breakpoint, memory is read-write.  This
> +   means we've previously made the location hardware one, but then the
> +   memory map changed, so we undo.
> +
> +   However, if "set breakpoint auto-hw" is disabled, and memory is
> +   read-only, we warn and disable the breakpoint location.  */
> +
> +static void
> +handle_automatic_hardware_breakpoints (bp_location *bl)
> +{
> +  if (bl->loc_type == bp_loc_software_breakpoint
> +      || bl->loc_type == bp_loc_hardware_breakpoint)
> +    {
> +      if (bl->owner->type != bp_hardware_breakpoint)
> +	{
> +	  /* When breakpoints are removed, remove_breakpoints will use
> +	     location types we've just set here, the only possible
> +	     problem is that memory map has changed during running
> +	     program, but it's not going to work anyway with current
> +	     gdb.  */
> +	  mem_region *mr = lookup_mem_region (bl->address);
> +
> +	  if (mr != nullptr)
> +	    {
> +	      if (automatic_hardware_breakpoints)
> +		{
> +		  enum bp_loc_type new_type;
> +
> +		  if (mr->attrib.mode != MEM_RW)
> +		    new_type = bp_loc_hardware_breakpoint;
> +		  else
> +		    new_type = bp_loc_software_breakpoint;
> +
> +		  if (new_type != bl->loc_type)
> +		    {
> +		      static bool said = false;
> +
> +		      bl->loc_type = new_type;
> +		      if (!said)
> +			{
> +			  fprintf_filtered (gdb_stdout,
> +					    _("Note: automatically using "
> +					      "hardware breakpoints for "
> +					      "read-only addresses.\n"));
> +			  said = true;
> +			}
> +		    }
> +		}
> +	      else if (bl->loc_type == bp_loc_software_breakpoint
> +		       && mr->attrib.mode != MEM_RW)
> +		{
> +		  bl->enabled = false;
> +
> +		  warning (_("Breakpoint %d.%d disabled.\n"
> +			    "Cannot set software breakpoint "
> +			    "at read-only address %s\n"),
> +			   bl->owner->number, bp_location_number (bl),
> +			   paddress (bl->gdbarch, bl->address));
> +		}
> +	    }
> +	}
> +    }
> +}
> +
>  static struct bp_location *
>  add_location_to_breakpoint (struct breakpoint *b,
>  			    const struct symtab_and_line *sal)
> @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
>    if (a->permanent != b->permanent)
>      return a->permanent > b->permanent;
>  
> +  /* Sort by type in order to make duplicate determination easier.
> +     See update_global_location_list.  This is kept in sync with
> +     breakpoint_locations_match.  */
> +  if (a->loc_type < b->loc_type)
> +    return true;
> +
> +  /* Likewise, for range-breakpoints, sort by length.  */
> +  if (a->loc_type == bp_loc_hardware_breakpoint
> +      && b->loc_type == bp_loc_hardware_breakpoint
> +      && a->length < b->length)
> +    return true;
> +
>    /* Make the internal GDB representation stable across GDB runs
>       where A and B memory inside GDB can differ.  Breakpoint locations of
>       the same type at the same address can be sorted in arbitrary order.  */
> @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
>        loc->cond_bytecode.reset ();
>      }
>  }
> +
>  /* Called whether new breakpoints are created, or existing breakpoints
>     deleted, to update the global location list and recompute which
>     locations are duplicate of which.
> @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>    ALL_BREAKPOINTS (b)
>      for (loc = b->loc; loc; loc = loc->next)
>        *locp++ = loc;
> +
> +  /* See if we need to "upgrade" a software breakpoint to a hardware
> +     breakpoint.  Do this before deciding whether locations are
> +     duplicates.  Also do this before sorting because sorting order
> +     depends on location type.  */
> +  for (locp = bp_locations;
> +       locp < bp_locations + bp_locations_count;
> +       locp++)
> +    {
> +      loc = *locp;
> +      if (!loc->inserted)
> +	handle_automatic_hardware_breakpoints (loc);
> +    }
> +
>    std::sort (bp_locations, bp_locations + bp_locations_count,
>  	     bp_location_is_less_than);
>  
> @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  		    {
>  		      struct bp_location *loc2 = *loc2p;
>  
> +		      if (loc2 == old_loc)
> +			continue;
> +
>  		      if (breakpoint_locations_match (loc2, old_loc))
>  			{
>  			  /* Read watchpoint locations are switched to
> @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  			  /* loc2 is a duplicated location. We need to check
>  			     if it should be inserted in case it will be
>  			     unduplicated.  */
> -			  if (loc2 != old_loc
> -			      && unduplicated_should_be_inserted (loc2))
> +			  if (unduplicated_should_be_inserted (loc2))
>  			    {
>  			      swap_insertion (old_loc, loc2);
>  			      keep_in_target = 1;
> @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b,
>  	    if (have_ambiguous_names)
>  	      {
>  		for (; l; l = l->next)
> -		  if (breakpoint_locations_match (e, l))
> -		    {
> -		      l->enabled = 0;
> -		      break;
> -		    }
> +		  {
> +		    /* Ignore software vs hardware location type at
> +		       this point, because with "set breakpoint
> +		       auto-hw", after a re-set, locations that were
> +		       hardware can end up as software, or vice versa.
> +		       As mentioned above, this is an heuristic and in
> +		       practice should give the correct answer often
> +		       enough.  */
> +		    if (breakpoint_locations_match (e, l, true))
> +		      {
> +			l->enabled = 0;
> +			break;
> +		      }
> +		  }
>  	      }
>  	    else
>  	      {
> diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> index 65f7baae9f5..b950e433ed8 100644
> --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \
>  # Ensure that inserting a software breakpoint in a known-read-only
>  # region fails.
>  gdb_test "break *$main_lo" \
> -    "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \
> +    "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \
>      "inserting software breakpoint in read-only memory fails"
>  
>  delete_breakpoints
> 
> base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
> -- 
> 2.14.5
> 

Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)

[Christian Biesinger]

On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 19 Apr 2020 18:25:55 +0100
> Subject: [PATCH] Stop considering hw and sw breakpoints duplicates
>
> ---
>  gdb/breakpoint.c                                   | 269 ++++++++++++++-------
>  gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |   2 +-
>  2 files changed, 183 insertions(+), 88 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e49025461ba..27799e89807 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
>  static int watchpoint_locations_match (struct bp_location *loc1,
>                                        struct bp_location *loc2);
>
> +static int breakpoint_locations_match (struct bp_location *loc1,
> +                                      struct bp_location *loc2,
> +                                      bool sw_hw_bps_match = false);
> +

Should this return a bool?

>  static int breakpoint_location_address_match (struct bp_location *bl,
>                                               const struct address_space *aspace,
>                                               CORE_ADDR addr);

And this, I guess.

> @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl)
>      return;
>
>    /* Do a first pass to check for locations with no assigned
> -     conditions or conditions that fail to parse to a valid agent expression
> -     bytecode.  If any of these happen, then it's no use to send conditions
> -     to the target since this location will always trigger and generate a
> -     response back to GDB.  */
> +     conditions or conditions that fail to parse to a valid agent
> +     expression bytecode.  If any of these happen, then it's no use to
> +     send conditions to the target since this location will always
> +     trigger and generate a response back to GDB.  Note we consider
> +     all locations at the same address irrespective of type, i.e.,
> +     even if the locations aren't considered duplicates (e.g.,
> +     software breakpoint and hardware breakpoint at the same
> +     address).  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl)
>         }
>      }
>
> -  /* No NULL conditions or failed bytecode generation.  Build a condition list
> -     for this location's address.  */
> +  /* No NULL conditions or failed bytecode generation.  Build a
> +     condition list for this location's address.  If we have software
> +     and hardware locations at the same address, they aren't
> +     considered duplicates, but we still marge all the conditions
> +     anyway, as it's simpler, and doesn't really make a practical
> +     difference.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl)
>    if (dprintf_style != dprintf_style_agent)
>      return;
>
> -  /* For now, if we have any duplicate location that isn't a dprintf,
> -     don't install the target-side commands, as that would make the
> -     breakpoint not be reported to the core, and we'd lose
> +  /* For now, if we have any location at the same address that isn't a
> +     dprintf, don't install the target-side commands, as that would
> +     make the breakpoint not be reported to the core, and we'd lose
>       control.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
> @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl)
>         }
>      }
>
> -  /* No NULL commands or failed bytecode generation.  Build a command list
> -     for this location's address.  */
> +  /* No NULL commands or failed bytecode generation.  Build a command
> +     list for all duplicate locations at this location's address.
> +     Note that here we must care for whether the breakpoint location
> +     types are considered duplicates, otherwise, say, if we have a
> +     software and hardware location at the same address, the target
> +     could end up running the commands twice.  For the moment, we only
> +     support targets-side commands with dprintf, but it doesn't hurt
> +     to be pedantically correct in case that changes.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> -      if (loc->owner->extra_string
> +      if (breakpoint_locations_match (bl, loc)
> +         && loc->owner->extra_string
>           && is_breakpoint (loc->owner)
>           && loc->pspace->num == bl->pspace->num
>           && loc->owner->enable_state == bp_enabled
> @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl,
>    if (bl->loc_type == bp_loc_software_breakpoint
>        || bl->loc_type == bp_loc_hardware_breakpoint)
>      {
> -      if (bl->owner->type != bp_hardware_breakpoint)
> -       {
> -         /* If the explicitly specified breakpoint type
> -            is not hardware breakpoint, check the memory map to see
> -            if the breakpoint address is in read only memory or not.
> -
> -            Two important cases are:
> -            - location type is not hardware breakpoint, memory
> -            is readonly.  We change the type of the location to
> -            hardware breakpoint.
> -            - location type is hardware breakpoint, memory is
> -            read-write.  This means we've previously made the
> -            location hardware one, but then the memory map changed,
> -            so we undo.
> -
> -            When breakpoints are removed, remove_breakpoints will use
> -            location types we've just set here, the only possible
> -            problem is that memory map has changed during running
> -            program, but it's not going to work anyway with current
> -            gdb.  */
> -         struct mem_region *mr
> -           = lookup_mem_region (bl->target_info.reqstd_address);
> -
> -         if (mr)
> -           {
> -             if (automatic_hardware_breakpoints)
> -               {
> -                 enum bp_loc_type new_type;
> -
> -                 if (mr->attrib.mode != MEM_RW)
> -                   new_type = bp_loc_hardware_breakpoint;
> -                 else
> -                   new_type = bp_loc_software_breakpoint;
> -
> -                 if (new_type != bl->loc_type)
> -                   {
> -                     static int said = 0;
> -
> -                     bl->loc_type = new_type;
> -                     if (!said)
> -                       {
> -                         fprintf_filtered (gdb_stdout,
> -                                           _("Note: automatically using "
> -                                             "hardware breakpoints for "
> -                                             "read-only addresses.\n"));
> -                         said = 1;
> -                       }
> -                   }
> -               }
> -             else if (bl->loc_type == bp_loc_software_breakpoint
> -                      && mr->attrib.mode != MEM_RW)
> -               {
> -                 fprintf_unfiltered (tmp_error_stream,
> -                                     _("Cannot insert breakpoint %d.\n"
> -                                       "Cannot set software breakpoint "
> -                                       "at read-only address %s\n"),
> -                                     bl->owner->number,
> -                                     paddress (bl->gdbarch, bl->address));
> -                 return 1;
> -               }
> -           }
> -       }
> -
>        /* First check to see if we have to handle an overlay.  */
>        if (overlay_debugging == ovly_off
>           || bl->section == NULL
> @@ -2830,7 +2786,11 @@ insert_breakpoints (void)
>
>    /* Updating watchpoints creates new locations, so update the global
>       location list.  Explicitly tell ugll to insert locations and
> -     ignore breakpoints_always_inserted_mode.  */
> +     ignore breakpoints_always_inserted_mode.  Also,
> +     update_global_location_list tries to "upgrade" software
> +     breakpoints to hardware breakpoints to handle "set breakpoint
> +     auto-hw", so we need to call it even if we don't have new
> +     locations.  */
>    update_global_location_list (UGLL_INSERT);
>  }
>
> @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1,
>
>  /* Assuming LOC1 and LOC2's types' have meaningful target addresses
>     (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
> -   the same location.  */
> +   the same location.  If SW_HW_BPS_MATCH is true, then software
> +   breakpoint locations and hardware breakpoint locations match,
> +   otherwise they don't.  */
>
>  static int
> -breakpoint_locations_match (struct bp_location *loc1,
> -                           struct bp_location *loc2)
> +breakpoint_locations_match (struct bp_location *loc1,
> +                           struct bp_location *loc2,
> +                           bool sw_hw_bps_match)
>  {
>    int hw_point1, hw_point2;
>
> @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1,
>    else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
>      return tracepoint_locations_match (loc1, loc2);
>    else
> -    /* We compare bp_location.length in order to cover ranged breakpoints.  */
> +    /* We compare bp_location.length in order to cover ranged
> +       breakpoints.  Keep this in sync with
> +       bp_location_is_less_than.  */
>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>                                      loc2->pspace->aspace, loc2->address)
> +           && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
>             && loc1->length == loc2->length);
>  }
>
> @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b)
>  }
>
>
> +/* Return the location number of LOC.  */
> +
> +static int
> +bp_location_number (bp_location *loc)
> +{
> +  int n = 1;
> +  for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
> +    {
> +      if (l == loc)
> +       return n;
> +      ++n;
> +    }
> +
> +  gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
> +}
> +
>  static bool bp_loc_is_permanent (struct bp_location *loc);
>
> +/* Handle "set breakpoint auto-hw".
> +
> +   If the explicitly specified breakpoint type is not hardware
> +   breakpoint, check the memory map to see whether the breakpoint
> +   address is in read-only memory.
> +
> +   And then, if "set breakpoint auto-hw" is enabled:
> +
> +   - location type is not hardware breakpoint, memory is read-only.
> +   We change the type of the location to hardware breakpoint.
> +
> +   - location type is hardware breakpoint, memory is read-write.  This
> +   means we've previously made the location hardware one, but then the
> +   memory map changed, so we undo.
> +
> +   However, if "set breakpoint auto-hw" is disabled, and memory is
> +   read-only, we warn and disable the breakpoint location.  */
> +
> +static void
> +handle_automatic_hardware_breakpoints (bp_location *bl)
> +{
> +  if (bl->loc_type == bp_loc_software_breakpoint
> +      || bl->loc_type == bp_loc_hardware_breakpoint)
> +    {
> +      if (bl->owner->type != bp_hardware_breakpoint)
> +       {
> +         /* When breakpoints are removed, remove_breakpoints will use
> +            location types we've just set here, the only possible
> +            problem is that memory map has changed during running
> +            program, but it's not going to work anyway with current
> +            gdb.  */
> +         mem_region *mr = lookup_mem_region (bl->address);
> +
> +         if (mr != nullptr)
> +           {
> +             if (automatic_hardware_breakpoints)
> +               {
> +                 enum bp_loc_type new_type;
> +
> +                 if (mr->attrib.mode != MEM_RW)
> +                   new_type = bp_loc_hardware_breakpoint;
> +                 else
> +                   new_type = bp_loc_software_breakpoint;
> +
> +                 if (new_type != bl->loc_type)
> +                   {
> +                     static bool said = false;
> +
> +                     bl->loc_type = new_type;
> +                     if (!said)
> +                       {
> +                         fprintf_filtered (gdb_stdout,
> +                                           _("Note: automatically using "
> +                                             "hardware breakpoints for "
> +                                             "read-only addresses.\n"));
> +                         said = true;
> +                       }
> +                   }
> +               }
> +             else if (bl->loc_type == bp_loc_software_breakpoint
> +                      && mr->attrib.mode != MEM_RW)
> +               {
> +                 bl->enabled = false;
> +
> +                 warning (_("Breakpoint %d.%d disabled.\n"
> +                           "Cannot set software breakpoint "
> +                           "at read-only address %s\n"),
> +                          bl->owner->number, bp_location_number (bl),
> +                          paddress (bl->gdbarch, bl->address));
> +               }
> +           }
> +       }
> +    }
> +}
> +
>  static struct bp_location *
>  add_location_to_breakpoint (struct breakpoint *b,
>                             const struct symtab_and_line *sal)
> @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
>    if (a->permanent != b->permanent)
>      return a->permanent > b->permanent;
>
> +  /* Sort by type in order to make duplicate determination easier.
> +     See update_global_location_list.  This is kept in sync with
> +     breakpoint_locations_match.  */
> +  if (a->loc_type < b->loc_type)
> +    return true;
> +
> +  /* Likewise, for range-breakpoints, sort by length.  */
> +  if (a->loc_type == bp_loc_hardware_breakpoint
> +      && b->loc_type == bp_loc_hardware_breakpoint
> +      && a->length < b->length)
> +    return true;
> +
>    /* Make the internal GDB representation stable across GDB runs
>       where A and B memory inside GDB can differ.  Breakpoint locations of
>       the same type at the same address can be sorted in arbitrary order.  */
> @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
>        loc->cond_bytecode.reset ();
>      }
>  }
> +
>  /* Called whether new breakpoints are created, or existing breakpoints
>     deleted, to update the global location list and recompute which
>     locations are duplicate of which.
> @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>    ALL_BREAKPOINTS (b)
>      for (loc = b->loc; loc; loc = loc->next)
>        *locp++ = loc;
> +
> +  /* See if we need to "upgrade" a software breakpoint to a hardware
> +     breakpoint.  Do this before deciding whether locations are
> +     duplicates.  Also do this before sorting because sorting order
> +     depends on location type.  */
> +  for (locp = bp_locations;
> +       locp < bp_locations + bp_locations_count;
> +       locp++)
> +    {
> +      loc = *locp;
> +      if (!loc->inserted)
> +       handle_automatic_hardware_breakpoints (loc);
> +    }
> +
>    std::sort (bp_locations, bp_locations + bp_locations_count,
>              bp_location_is_less_than);
>
> @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>                     {
>                       struct bp_location *loc2 = *loc2p;
>
> +                     if (loc2 == old_loc)
> +                       continue;
> +
>                       if (breakpoint_locations_match (loc2, old_loc))
>                         {
>                           /* Read watchpoint locations are switched to
> @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>                           /* loc2 is a duplicated location. We need to check
>                              if it should be inserted in case it will be
>                              unduplicated.  */
> -                         if (loc2 != old_loc
> -                             && unduplicated_should_be_inserted (loc2))
> +                         if (unduplicated_should_be_inserted (loc2))
>                             {
>                               swap_insertion (old_loc, loc2);
>                               keep_in_target = 1;
> @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b,
>             if (have_ambiguous_names)
>               {
>                 for (; l; l = l->next)
> -                 if (breakpoint_locations_match (e, l))
> -                   {
> -                     l->enabled = 0;
> -                     break;
> -                   }
> +                 {
> +                   /* Ignore software vs hardware location type at
> +                      this point, because with "set breakpoint
> +                      auto-hw", after a re-set, locations that were
> +                      hardware can end up as software, or vice versa.
> +                      As mentioned above, this is an heuristic and in
> +                      practice should give the correct answer often
> +                      enough.  */
> +                   if (breakpoint_locations_match (e, l, true))
> +                     {
> +                       l->enabled = 0;
> +                       break;
> +                     }
> +                 }
>               }
>             else
>               {
> diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> index 65f7baae9f5..b950e433ed8 100644
> --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \
>  # Ensure that inserting a software breakpoint in a known-read-only
>  # region fails.
>  gdb_test "break *$main_lo" \
> -    "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \
> +    "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \
>      "inserting software breakpoint in read-only memory fails"
>
>  delete_breakpoints
>
> base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
> --
> 2.14.5
>

Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)

[Pedro Alves]

On 4/21/20 5:24 PM, Christian Biesinger via Gdb-patches wrote:
> On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>> From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <palves@redhat.com>
>> Date: Sun, 19 Apr 2020 18:25:55 +0100
>> Subject: [PATCH] Stop considering hw and sw breakpoints duplicates
>>
>> ---
>>  gdb/breakpoint.c                                   | 269 ++++++++++++++-------
>>  gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |   2 +-
>>  2 files changed, 183 insertions(+), 88 deletions(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index e49025461ba..27799e89807 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
>>  static int watchpoint_locations_match (struct bp_location *loc1,
>>                                        struct bp_location *loc2);
>>
>> +static int breakpoint_locations_match (struct bp_location *loc1,
>> +                                      struct bp_location *loc2,
>> +                                      bool sw_hw_bps_match = false);
>> +
> 
> Should this return a bool?
> 
>>  static int breakpoint_location_address_match (struct bp_location *bl,
>>                                               const struct address_space *aspace,
>>                                               CORE_ADDR addr);
> 
> And this, I guess.

Yeah, they should.  Old, pre-C++ code.
It feels like an orthogonal change though, particularly since I think we
could change all of those two, plus watchpoint_locations_match at the
same time.

Thanks,
Pedro Alves

[PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741)

[Pedro Alves]

On 4/19/20 7:49 PM, Pedro Alves via Gdb-patches wrote:

> This makes breakpoint_locations_match consider the location's
> type too, like Keno's original patch.  But then does a bunch
> more stuff to actually make that work correctly.
> 
> - We need to handle the duplicates detection better.  Take a look
>   at the loop at the tail end of update_global_location_list.
>   Currently, because breakpoint locations aren't sorted by type,
>   we can end up with, at the same address, a sw break, then a hw break,
>   then a sw break, etc.  The result is that that second sw break won't
>   be considered a duplicate of the first sw break.  Seems like
>   we already handle that incorrectly for range breakpoints.
> 
> - The "set breakpoint auto-hw" handling is moved out of insert_bp_location
>   to update_global_location_list, before the duplicates determination.
> 
>   This change comes with one visible behavior change with
>   "set breakpoint auto-hw off", however.  Here:
> 
>   Before:
> 
>    (gdb) break *0x400487
>    Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
>    (gdb) c
>    Continuing.
>    Warning:
>    Cannot insert breakpoint 2.
>    Cannot set software breakpoint at read-only address 0x400487
> 
>    Command aborted.
>    (gdb)
> 
>   After:
> 
>    (gdb) break *0x400487
>    Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
>    warning: Breakpoint 2.1 disabled.
>    Cannot set software breakpoint at read-only address 0x400487
> 
>   I.e., we now warn earlier, instead of waiting until resume time to error out.

While looking at the test_single_step part of 
gdb.base/breapoint-in-ro-region.exp, I realized that the change mentioned
above isn't correct as is.  The problem is that we would be warning and
disabling the location, but if that location belongs to e.g., a software
single-step location, GDB would end up not installing the
location (because it was disabled), and then the inferior would be 
resumed, and GDB would lose control of the inferior, as it run free
with no software single-step breakpoint installed.

So instead I've moved the "set breakpoint auto-hw ON" handling to
update_global_location_list, and left the "set breakpoint auto-hw OFF"
where it was, in insert_bp_location.  This means there's no longer
a user-visible change.

I have not tested this on a software single-step target (I don't
think I have such a target readily handy), but I believe that this
new version will pass there, and the previous version would fail.

> +  /* See if we need to "upgrade" a software breakpoint to a hardware
> +     breakpoint.  Do this before deciding whether locations are
> +     duplicates.  Also do this before sorting because sorting order
> +     depends on location type.  */
> +  for (locp = bp_locations;
> +       locp < bp_locations + bp_locations_count;
> +       locp++)
> +    {
> +      loc = *locp;
> +      if (!loc->inserted)
> +	handle_automatic_hardware_breakpoints (loc);

This here missed checking should_be_inserted -- there's no
point in calling handle_automatic_hardware_breakpoints for
disabled locations.  It no longer makes that much of a 
difference with the "set breakpoint auto-hw off" warning
removed from handle_automatic_hardware_breakpoints, but it
doesn't hurt to be correct.

I've also realized that before:

 commit 7c16b83e0521a007e4d86fc30e334b41b01668b4
 Author:     Pedro Alves <palves@redhat.com>
 AuthorDate: Wed Oct 15 20:18:31 2014 +0100

    Put single-step breakpoints on the bp_location chain

we were already inserting hw breakpoints and single-step
breakpoints at the same address.  But not hw breakpoints
and regular software breakpoints, though.  So it was
still broken, but I thought that was interesting in the
sense that it just seems like the hw-and-sw-breakpoints
at the same address case just wasn't thought about much
in the early days.

I've added a new testcase too now.  It fails against 
gdbserver on current master, passes with the fix.

I've also now updated the git commit log and write ChangeLog entries.

WDYT of this version?

From 0a08d49d0aa0cdfb1e6f4da044df2084d3403dc3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 2 May 2020 21:04:00 +0100
Subject: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates
 (PR gdb/25741)

In the following conditions:

  - A target with hardware breakpoints available, and
  - A target that uses software single stepping,
  - An instruction at ADDRESS loops back to itself,

Now consider the following steps:

  1. The user places a hardware breakpoint at ADDRESS (an instruction
  that loops to itself),

  2. The inferior runs and hits the breakpoint at ADDRESS,

  3. The user tells GDB to 'continue'.

In #3 when the user tells GDB to continue, GDB first disables the
hardware breakpoint at ADDRESS, and then inserts a software
single-step breakpoint at ADDRESS.  The original user-created
breakpoint was a hardware breakpoint, while the single-step breakpoint
will be a software breakpoint.

GDB continues and immediately hits the software single-step
breakpoint.

GDB then deletes the software single-step breakpoint by calling
delete_single_step_breakpoints, which eventually calls
delete_breakpoint, which, once the breakpoint (and its locations) are
deleted, calls update_global_location_list.

During update_global_location_list GDB spots that we have an old
location (the software single step breakpoint location) that is
inserted, but being deleted, and a location (the original hardware
breakpoint) at the same address which we are keeping, but which is not
currently inserted, GDB then calls breakpoint_locations_match on these
two locations.

Currently the locations do match, and so GDB calls swap_insertion
which swaps the "inserted" state of the two locations.  The user
created hardware breakpoint is marked as inserted, while the GDB
internal software single step breakpoint is now marked as not
inserted.  After this GDB returns through the call stack and leaves
delete_single_step_breakpoints.

After this GDB continues with its normal "stopping" process, as part
of this stopping process GDB removes all the breakpoints from the
target.  Due to the swap it is now the user-created hardware
breakpoint that is marked as inserted, so it is this breakpoint GDB
tries to remove.

The problem is that GDB inserted the software single-step breakpoint
as a software breakpoint, but is now trying to remove the hardware
breakpoint.  The problem is removing a software breakpoint is very
different to removing a hardware breakpoint, this could result is some
undetected undefined behaviour, or as in the original bug report (PR
gdb/25741), could result in the target throwing an error.

With "set breakpoint always-inserted on", we can easily reproduce this
against GDBserver.  E.g.:

  (gdb) hbreak main
  Sending packet: $m400700,40#28...Packet received: 89e58b....
  Sending packet: $m400736,1#fe...Packet received: 48
  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
  Sending packet: $Z1,400736,1#48...Packet received: OK
  Packet Z1 (hardware-breakpoint) is supported

  (gdb) b main
  Note: breakpoint 1 also set at pc 0x400736.
  Sending packet: $m400736,1#fe...Packet received: 48
  Breakpoint 2 at 0x400736: file threads.c, line 57.

  (gdb) del
  Delete all breakpoints? (y or n) y
  Sending packet: $z0,400736,1#67...Packet received: E01
  warning: Error removing breakpoint 2

This patch adds a testcase that does exactly that.

Trying to enhance GDB to handle this scenario while continuing to
avoid inserting redundant software and hardware breakpoints at the
same address turns out futile, because, given non-stop and breakpoints
always-inserted, if the user:

 #1 - inserts a hw breakpoint, then
 #2 - inserts a sw breakpoint at the same address, and then
 #3 - removes the original hw breakpoint,

GDB would have to make sure to insert the sw breakpoint before
removing the hw breakpoint, to avoid running threads missing the
breakpoint.  I.e., there's always going to be a window where a target
needs to be able to handle both sw and a hw breakpoints installed at
the same address.  You can see more detailed description of that issue
here:
https://sourceware.org/pipermail/gdb-patches/2020-April/167738.html

So the fix here is to just stop considering software breakpoints and
hw breakpoints duplicates, and let GDB insert sw and hw breakpoints at
the same address.

The central change is to make breakpoint_locations_match consider the
location's type too.  There are several other changes necessary to
actually make that that work correctly, however:

- We need to handle the duplicates detection better.  Take a look at
  the loop at the tail end of update_global_location_list.  Currently,
  because breakpoint locations aren't sorted by type, we can end up
  with, at the same address, a sw break, then a hw break, then a sw
  break, etc.  The result is that that second sw break won't be
  considered a duplicate of the first sw break.  Seems like we already
  handle that incorrectly for range breakpoints.

- The "set breakpoint auto-hw on" handling is moved out of
  insert_bp_location to update_global_location_list, before the
  duplicates determination.

  Moving "set breakpoint auto-hw off" handling as well and downgrading
  it to a warning+'disabling the location' was considered too, but in
  the end discarded, because we want to error out with internal and
  momentary breakpoints, like software single-step breakpoints.
  Disabling such locations at update_global_location_list time would
  make GDB lose control of the inferior.

- In update_breakpoint_locations, the logic of matching old locations
  with new locations, in the have_ambiguous_names case, is updated to
  still consider sw vs hw locations the same.

- Review all ALL_BP_LOCATIONS_AT_ADDR uses, and update those that
  might need to be updated, and update comments for those that don't.
  Note that that macro walks all locations at a given address, and
  doesn't call breakpoint_locations_match.

The result against GDBserver (with "set breakpoint
condition-evaluation host" to avoid seeing confusing reinsertions) is:

 (gdb) hbreak main
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file main.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 4 at 0x400736: file main.c, line 57.
 Sending packet: $Z0,400736,1#47...Packet received: OK

 (gdb) del 3
 Sending packet: $z1,400736,1#68...Packet received: OK

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
	    Andrew Burgess  <andrew.burgess@embecosm.com>
	    Keno Fischer  <keno@juliacomputing.com>

	PR gdb/25741
	* breakpoint.c (build_target_condition_list): Update comments.
	(build_target_command_list): Update comments and skip matching
	locations.
	(insert_bp_location): Move "set breakpoint auto-hw on" handling to
	a separate function.  Simplify "set breakpoint auto-hw off"
	handling.
	(insert_breakpoints): Update comment.
	(tracepoint_locations_match): New parameter.  For breakpoints,
	compare location types too, if the caller wants to.
	(handle_automatic_hardware_breakpoints): New functions.
	(bp_location_is_less_than): Also sort by location type and
	hardware breakpoint length.
	(update_global_location_list): Handle "set breakpoint auto-hw on"
	here.
	(update_breakpoint_locations): Ask breakpoint_locations_match to
	ignore location types.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/25741
	* gdb.base/hw-sw-break-same-address.exp: New file.
---
 gdb/breakpoint.c                                   | 253 ++++++++++++++-------
 .../gdb.base/hw-sw-break-same-address.exp          |  73 ++++++
 2 files changed, 240 insertions(+), 86 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/hw-sw-break-same-address.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 858f4c75691..3feed929e78 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
 static int watchpoint_locations_match (struct bp_location *loc1,
 				       struct bp_location *loc2);
 
+static int breakpoint_locations_match (struct bp_location *loc1,
+				       struct bp_location *loc2,
+				       bool sw_hw_bps_match = false);
+
 static int breakpoint_location_address_match (struct bp_location *bl,
 					      const struct address_space *aspace,
 					      CORE_ADDR addr);
@@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl)
     return;
 
   /* Do a first pass to check for locations with no assigned
-     conditions or conditions that fail to parse to a valid agent expression
-     bytecode.  If any of these happen, then it's no use to send conditions
-     to the target since this location will always trigger and generate a
-     response back to GDB.  */
+     conditions or conditions that fail to parse to a valid agent
+     expression bytecode.  If any of these happen, then it's no use to
+     send conditions to the target since this location will always
+     trigger and generate a response back to GDB.  Note we consider
+     all locations at the same address irrespective of type, i.e.,
+     even if the locations aren't considered duplicates (e.g.,
+     software breakpoint and hardware breakpoint at the same
+     address).  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
@@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl)
 	}
     }
 
-  /* No NULL conditions or failed bytecode generation.  Build a condition list
-     for this location's address.  */
+  /* No NULL conditions or failed bytecode generation.  Build a
+     condition list for this location's address.  If we have software
+     and hardware locations at the same address, they aren't
+     considered duplicates, but we still marge all the conditions
+     anyway, as it's simpler, and doesn't really make a practical
+     difference.  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
@@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl)
   if (dprintf_style != dprintf_style_agent)
     return;
 
-  /* For now, if we have any duplicate location that isn't a dprintf,
-     don't install the target-side commands, as that would make the
-     breakpoint not be reported to the core, and we'd lose
+  /* For now, if we have any location at the same address that isn't a
+     dprintf, don't install the target-side commands, as that would
+     make the breakpoint not be reported to the core, and we'd lose
      control.  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
@@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl)
 	}
     }
 
-  /* No NULL commands or failed bytecode generation.  Build a command list
-     for this location's address.  */
+  /* No NULL commands or failed bytecode generation.  Build a command
+     list for all duplicate locations at this location's address.
+     Note that here we must care for whether the breakpoint location
+     types are considered duplicates, otherwise, say, if we have a
+     software and hardware location at the same address, the target
+     could end up running the commands twice.  For the moment, we only
+     support targets-side commands with dprintf, but it doesn't hurt
+     to be pedantically correct in case that changes.  */
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
-      if (loc->owner->extra_string
+      if (breakpoint_locations_match (bl, loc)
+	  && loc->owner->extra_string
 	  && is_breakpoint (loc->owner)
 	  && loc->pspace->num == bl->pspace->num
 	  && loc->owner->enable_state == bp_enabled
@@ -2451,72 +2470,31 @@ insert_bp_location (struct bp_location *bl,
       bl->needs_update = 0;
     }
 
+  /* If "set breakpoint auto-hw" is "on" and a software breakpoint was
+     set at a read-only address, then a breakpoint location will have
+     been changed to hardware breakpoint before we get here.  If it is
+     "off" however, error out before actually trying to insert the
+     breakpoint, with a nicer error message.  */
   if (bl->loc_type == bp_loc_software_breakpoint
-      || bl->loc_type == bp_loc_hardware_breakpoint)
+      && !automatic_hardware_breakpoints)
     {
-      if (bl->owner->type != bp_hardware_breakpoint)
-	{
-	  /* If the explicitly specified breakpoint type
-	     is not hardware breakpoint, check the memory map to see
-	     if the breakpoint address is in read only memory or not.
-
-	     Two important cases are:
-	     - location type is not hardware breakpoint, memory
-	     is readonly.  We change the type of the location to
-	     hardware breakpoint.
-	     - location type is hardware breakpoint, memory is
-	     read-write.  This means we've previously made the
-	     location hardware one, but then the memory map changed,
-	     so we undo.
-	     
-	     When breakpoints are removed, remove_breakpoints will use
-	     location types we've just set here, the only possible
-	     problem is that memory map has changed during running
-	     program, but it's not going to work anyway with current
-	     gdb.  */
-	  struct mem_region *mr 
-	    = lookup_mem_region (bl->target_info.reqstd_address);
-	  
-	  if (mr)
-	    {
-	      if (automatic_hardware_breakpoints)
-		{
-		  enum bp_loc_type new_type;
-		  
-		  if (mr->attrib.mode != MEM_RW)
-		    new_type = bp_loc_hardware_breakpoint;
-		  else 
-		    new_type = bp_loc_software_breakpoint;
-		  
-		  if (new_type != bl->loc_type)
-		    {
-		      static int said = 0;
+      mem_region *mr = lookup_mem_region (bl->address);
 
-		      bl->loc_type = new_type;
-		      if (!said)
-			{
-			  fprintf_filtered (gdb_stdout,
-					    _("Note: automatically using "
-					      "hardware breakpoints for "
-					      "read-only addresses.\n"));
-			  said = 1;
-			}
-		    }
-		}
-	      else if (bl->loc_type == bp_loc_software_breakpoint
-		       && mr->attrib.mode != MEM_RW)
-		{
-		  fprintf_unfiltered (tmp_error_stream,
-				      _("Cannot insert breakpoint %d.\n"
-					"Cannot set software breakpoint "
-					"at read-only address %s\n"),
-				      bl->owner->number,
-				      paddress (bl->gdbarch, bl->address));
-		  return 1;
-		}
-	    }
+      if (mr != nullptr && mr->attrib.mode != MEM_RW)
+	{
+	  fprintf_unfiltered (tmp_error_stream,
+			      _("Cannot insert breakpoint %d.\n"
+				"Cannot set software breakpoint "
+				"at read-only address %s\n"),
+			      bl->owner->number,
+			      paddress (bl->gdbarch, bl->address));
+	  return 1;
 	}
-        
+    }
+
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
+    {
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
 	  || bl->section == NULL
@@ -2830,7 +2808,11 @@ insert_breakpoints (void)
 
   /* Updating watchpoints creates new locations, so update the global
      location list.  Explicitly tell ugll to insert locations and
-     ignore breakpoints_always_inserted_mode.  */
+     ignore breakpoints_always_inserted_mode.  Also,
+     update_global_location_list tries to "upgrade" software
+     breakpoints to hardware breakpoints to handle "set breakpoint
+     auto-hw", so we need to call it even if we don't have new
+     locations.  */
   update_global_location_list (UGLL_INSERT);
 }
 
@@ -6813,11 +6795,14 @@ tracepoint_locations_match (struct bp_location *loc1,
 
 /* Assuming LOC1 and LOC2's types' have meaningful target addresses
    (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
-   the same location.  */
+   the same location.  If SW_HW_BPS_MATCH is true, then software
+   breakpoint locations and hardware breakpoint locations match,
+   otherwise they don't.  */
 
 static int
-breakpoint_locations_match (struct bp_location *loc1, 
-			    struct bp_location *loc2)
+breakpoint_locations_match (struct bp_location *loc1,
+			    struct bp_location *loc2,
+			    bool sw_hw_bps_match)
 {
   int hw_point1, hw_point2;
 
@@ -6835,9 +6820,12 @@ breakpoint_locations_match (struct bp_location *loc1,
   else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
     return tracepoint_locations_match (loc1, loc2);
   else
-    /* We compare bp_location.length in order to cover ranged breakpoints.  */
+    /* We compare bp_location.length in order to cover ranged
+       breakpoints.  Keep this in sync with
+       bp_location_is_less_than.  */
     return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
 				     loc2->pspace->aspace, loc2->address)
+	    && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
 	    && loc1->length == loc2->length);
 }
 
@@ -8517,6 +8505,61 @@ mention (struct breakpoint *b)
 
 static bool bp_loc_is_permanent (struct bp_location *loc);
 
+/* Handle "set breakpoint auto-hw on".
+
+   If the explicitly specified breakpoint type is not hardware
+   breakpoint, check the memory map to see whether the breakpoint
+   address is in read-only memory.
+
+   - location type is not hardware breakpoint, memory is read-only.
+   We change the type of the location to hardware breakpoint.
+
+   - location type is hardware breakpoint, memory is read-write.  This
+   means we've previously made the location hardware one, but then the
+   memory map changed, so we undo.
+*/
+
+static void
+handle_automatic_hardware_breakpoints (bp_location *bl)
+{
+  if (automatic_hardware_breakpoints
+      && bl->owner->type != bp_hardware_breakpoint
+      && (bl->loc_type == bp_loc_software_breakpoint
+	  || bl->loc_type == bp_loc_hardware_breakpoint))
+    {
+      /* When breakpoints are removed, remove_breakpoints will use
+	 location types we've just set here, the only possible problem
+	 is that memory map has changed during running program, but
+	 it's not going to work anyway with current gdb.  */
+      mem_region *mr = lookup_mem_region (bl->address);
+
+      if (mr != nullptr)
+	{
+	  enum bp_loc_type new_type;
+
+	  if (mr->attrib.mode != MEM_RW)
+	    new_type = bp_loc_hardware_breakpoint;
+	  else
+	    new_type = bp_loc_software_breakpoint;
+
+	  if (new_type != bl->loc_type)
+	    {
+	      static bool said = false;
+
+	      bl->loc_type = new_type;
+	      if (!said)
+		{
+		  fprintf_filtered (gdb_stdout,
+				    _("Note: automatically using "
+				      "hardware breakpoints for "
+				      "read-only addresses.\n"));
+		  said = true;
+		}
+	    }
+	}
+    }
+}
+
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
@@ -11451,6 +11494,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
   if (a->permanent != b->permanent)
     return a->permanent > b->permanent;
 
+  /* Sort by type in order to make duplicate determination easier.
+     See update_global_location_list.  This is kept in sync with
+     breakpoint_locations_match.  */
+  if (a->loc_type < b->loc_type)
+    return true;
+
+  /* Likewise, for range-breakpoints, sort by length.  */
+  if (a->loc_type == bp_loc_hardware_breakpoint
+      && b->loc_type == bp_loc_hardware_breakpoint
+      && a->length < b->length)
+    return true;
+
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */
@@ -11625,6 +11680,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
       loc->cond_bytecode.reset ();
     }
 }
+
 /* Called whether new breakpoints are created, or existing breakpoints
    deleted, to update the global location list and recompute which
    locations are duplicate of which.
@@ -11673,6 +11729,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
   ALL_BREAKPOINTS (b)
     for (loc = b->loc; loc; loc = loc->next)
       *locp++ = loc;
+
+  /* See if we need to "upgrade" a software breakpoint to a hardware
+     breakpoint.  Do this before deciding whether locations are
+     duplicates.  Also do this before sorting because sorting order
+     depends on location type.  */
+  for (locp = bp_locations;
+       locp < bp_locations + bp_locations_count;
+       locp++)
+    {
+      loc = *locp;
+      if (!loc->inserted && should_be_inserted (loc))
+	handle_automatic_hardware_breakpoints (loc);
+    }
+
   std::sort (bp_locations, bp_locations + bp_locations_count,
 	     bp_location_is_less_than);
 
@@ -11776,6 +11846,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 		    {
 		      struct bp_location *loc2 = *loc2p;
 
+		      if (loc2 == old_loc)
+			continue;
+
 		      if (breakpoint_locations_match (loc2, old_loc))
 			{
 			  /* Read watchpoint locations are switched to
@@ -11790,8 +11863,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			  /* loc2 is a duplicated location. We need to check
 			     if it should be inserted in case it will be
 			     unduplicated.  */
-			  if (loc2 != old_loc
-			      && unduplicated_should_be_inserted (loc2))
+			  if (unduplicated_should_be_inserted (loc2))
 			    {
 			      swap_insertion (old_loc, loc2);
 			      keep_in_target = 1;
@@ -13508,11 +13580,20 @@ update_breakpoint_locations (struct breakpoint *b,
 	    if (have_ambiguous_names)
 	      {
 		for (; l; l = l->next)
-		  if (breakpoint_locations_match (e, l))
-		    {
-		      l->enabled = 0;
-		      break;
-		    }
+		  {
+		    /* Ignore software vs hardware location type at
+		       this point, because with "set breakpoint
+		       auto-hw", after a re-set, locations that were
+		       hardware can end up as software, or vice versa.
+		       As mentioned above, this is an heuristic and in
+		       practice should give the correct answer often
+		       enough.  */
+		    if (breakpoint_locations_match (e, l, true))
+		      {
+			l->enabled = 0;
+			break;
+		      }
+		  }
 	      }
 	    else
 	      {
diff --git a/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp b/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp
new file mode 100644
index 00000000000..92896ff4db5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp
@@ -0,0 +1,73 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that inserting a hardware and a software breakpoint at the same
+# address behaves as expected.  GDB used to consider hw and sw
+# breakpoint locations as duplicate locations, which would lead to bad
+# behavior.  See PR gdb/25741.
+
+if {[skip_hw_breakpoint_tests]} {
+    return 0
+}
+
+set test hbreak
+set srcfile ${test}.c
+if { [prepare_for_testing "failed to prepare" ${test} ${srcfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return -1
+}
+
+delete_breakpoints
+
+gdb_test_no_output "set breakpoint always-inserted on"
+gdb_test_no_output "set breakpoint condition-evaluation host"
+gdb_test_no_output "set confirm off"
+
+# Test inserting a hw breakpoint first, then a sw breakpoint at the
+# same address.
+with_test_prefix "hw-sw" {
+    gdb_test "hbreak main" \
+	"Hardware assisted breakpoint .* at .*" \
+	"hbreak"
+
+    gdb_test "break main" \
+	"Note: breakpoint .* also set at .*\r\nBreakpoint .* at .*" \
+	"break"
+
+    # A bad GDB debugging against GDBserver would output a warning
+    # here:
+    #  delete breakpoints
+    #  warning: Error removing breakpoint 3
+    #  (gdb) FAIL: gdb.base/hw-sw-break-same-address.exp: hw-sw: delete breakpoints
+    gdb_test_no_output "delete breakpoints"
+}
+
+# Now the opposite: test inserting a sw breakpoint first, then a hw
+# breakpoint at the same address.
+with_test_prefix "sw-hw" {
+    gdb_test "break main" \
+	"Breakpoint .* at .*" \
+	"break"
+
+    gdb_test "hbreak main" \
+	"Note: breakpoint .* also set at .*\r\nHardware assisted breakpoint .* at .*" \
+	"hbreak"
+
+    gdb_test_no_output "delete breakpoints"
+}

base-commit: 8c164434186b471fc43a47055af9632c56affdcd
-- 
2.14.5

Re: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741)

[Pedro Alves]

On 5/2/20 9:13 PM, Pedro Alves via Gdb-patches wrote:

> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 	    Andrew Burgess  <andrew.burgess@embecosm.com>
> 	    Keno Fischer  <keno@juliacomputing.com>
> 
> 	PR gdb/25741
> 	* breakpoint.c (build_target_condition_list): Update comments.
> 	(build_target_command_list): Update comments and skip matching
> 	locations.
> 	(insert_bp_location): Move "set breakpoint auto-hw on" handling to
> 	a separate function.  Simplify "set breakpoint auto-hw off"
> 	handling.
> 	(insert_breakpoints): Update comment.
> 	(tracepoint_locations_match): New parameter.  For breakpoints,
> 	compare location types too, if the caller wants to.
> 	(handle_automatic_hardware_breakpoints): New functions.
> 	(bp_location_is_less_than): Also sort by location type and
> 	hardware breakpoint length.
> 	(update_global_location_list): Handle "set breakpoint auto-hw on"
> 	here.
> 	(update_breakpoint_locations): Ask breakpoint_locations_match to
> 	ignore location types.
> 
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/25741
> 	* gdb.base/hw-sw-break-same-address.exp: New file.

I've merged this now.

Thanks,
Pedro Alves

Re: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741)

[Keno Fischer]

On Sun, May 17, 2020, 14:25 Pedro Alves <palves@redhat.com> wrote:
>
> I've merged this now.
>
> Thanks,
> Pedro Alves

Thanks folks. Appreciate all the work that went into this.

Keno