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

[Keno Fischer <keno@juliacomputing.com>]

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
>
>