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

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

* 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