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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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