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

[Pedro Alves <palves@redhat.com>]

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