[binutils-gdb] Make breakpoint handling in record-full idempotent

[Yao Qi <qiyao at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e390720bdc6ddee752992537259d18d1ae8d2eb1

commit e390720bdc6ddee752992537259d18d1ae8d2eb1
Author: Yao Qi <yao.qi@linaro.org>
Date:   Thu Apr 7 16:47:26 2016 +0100

    Make breakpoint handling in record-full idempotent
    
    Some test fails in gdb.reverse/break-reverse.exp on arm-linux lead me
    seeing the following error message,
    
    continue^M
    Continuing.^M
    Cannot remove breakpoints because program is no longer writable.^M
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    Further execution is probably impossible.^M
    ^M
    Breakpoint 3, bar () at /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.reverse/break-reverse.c:22^M
    22        xyz = 2; /* break in bar */^M
    (gdb) PASS: gdb.reverse/break-reverse.exp: continue to breakpoint: bar backward
    
    this is caused by two entries in record_full_breakpoints, and their addr
    is the same, but in_target_beneath is different.
    
    during the record, we do continue,
    
    Continuing.
    infrun: clear_proceed_status_thread (Thread 13772.13772)
    infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT)
    infrun: step-over queue now empty
    infrun: resuming [Thread 13772.13772] for step-over
    infrun: skipping breakpoint: stepping past insn at: 0x8620
    Sending packet: $Z0,85f4,4#1d...Packet received: OK  <----
    .....
    Sending packet: $vCont;c#a8...infrun: target_wait (-1.0.0, status) =
    infrun:   -1.0.0 [process -1],
    infrun:   status->kind = ignore
    infrun: TARGET_WAITKIND_IGNORE
    infrun: prepare_to_wait
    infrun: target_wait (-1.0.0, status) =
    infrun:   -1.0.0 [process -1],
    infrun:   status->kind = ignore
    infrun: TARGET_WAITKIND_IGNORE
    infrun: prepare_to_wait
    Packet received: T05swbreak:;0b:9cf5ffbe;0d:9cf5ffbe;0f:f4850000;thread:p35cc.35cc;core:1;
    Sending packet: $Z0,85f4,4#1d...Packet received: OK <-----
    ....
    Sending packet: $z0,85f4,4#3d...Packet received: OK <-----
    
    we can see breakpoint on 0x85f4 are inserted *twice*, but only removed
    once.  That is fine to remote target, because Z/z packets are
    idempotent, but there is a leftover in record_full_breakpoints
    in record-full target.  The flow can be described as below,
    
                                    record_full_breakpoints   remote target
      -----------------------------------------------------------------------
      forward execution, continue,    in_target_beneath 1     breakpoint inserted
      insert breakpoints on 0x85f4    in_target_beneath 1
      twice
    
      program stops,
      remove breakpoint on 0x85f4     in_target_beneath 1     breakpoint removed
    
      reverse execution, continue,    in_target_beneath 1     none is requested
      insert breakpoints on 0x85f4,   in_target_beneath 0
    
      program stops,
      remote breakpoint on 0x85f4,    in_target_beneath 0     request to remove,
                                                              but GDBserver
    							  doesn't know
    
    now, the question is why breakoint on 0x85f4 is inserted twice?  One
    is the normal breakpoint, and the other is the single step breakpoint.
    GDB inserts single step breakpoint to do single step.  When program
    stops at 0x85f4, both of them are set on 0x85f4, and GDB deletes
    single step breakpoint, so in update_global_location_list, this
    breakpoint location is no longer found, GDB call
    force_breakpoint_reinsertion to mark it condition_updated, and insert
    it again.
    
    The reason force_breakpoint_reinsertion is called to update the
    conditions in the target side, because the conditions may be
    changed.  My original fix is to not call force_breakpoint_reinsertion
    if OLD_LOC->cond is NULL, but it is not correct if another location
    on the same address has condition, GDB doesn't produce condition for
    target side, but GDB should do.
    
    Then, I change my mind back to make record-full handling breakpoint
    idempotent, to align with remote target.  Before insert a new entry
    into record_full_breakpoints, look for existing one on the same
    address first.  I also add an assert on
    "bp->in_target_beneath == in_target_beneath", to be safer.
    
    gdb:
    
    2016-04-07  Yao Qi  <yao.qi@linaro.org>
    
    	* record-full.c (record_full_insert_breakpoint): Return
    	early if entry on the address is found in
    	record_full_breakpoints.

Diff:
---
 gdb/ChangeLog     |  6 ++++++
 gdb/record-full.c | 17 +++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 39f77f8..dfd49b9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2016-04-07  Yao Qi  <yao.qi@linaro.org>
 
+	* record-full.c (record_full_insert_breakpoint): Return
+	early if entry on the address is found in
+	record_full_breakpoints.
+
+2016-04-07  Yao Qi  <yao.qi@linaro.org>
+
 	* record-full.c (record_full_insert_breakpoint): Set
 	bp_tgt->reqstd_address and bp_tgt->placed_size.
 
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 066a8e7..d8aa89c 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1650,6 +1650,7 @@ record_full_insert_breakpoint (struct target_ops *ops,
 {
   struct record_full_breakpoint *bp;
   int in_target_beneath = 0;
+  int ix;
 
   if (!RECORD_FULL_IS_REPLAY)
     {
@@ -1681,6 +1682,22 @@ record_full_insert_breakpoint (struct target_ops *ops,
       bp_tgt->placed_size = bplen;
     }
 
+  /* Use the existing entries if found in order to avoid duplication
+     in record_full_breakpoints.  */
+
+  for (ix = 0;
+       VEC_iterate (record_full_breakpoint_p,
+		    record_full_breakpoints, ix, bp);
+       ++ix)
+    {
+      if (bp->addr == bp_tgt->placed_address
+	  && bp->address_space == bp_tgt->placed_address_space)
+	{
+	  gdb_assert (bp->in_target_beneath == in_target_beneath);
+	  return 0;
+	}
+    }
+
   bp = XNEW (struct record_full_breakpoint);
   bp->addr = bp_tgt->placed_address;
   bp->address_space = bp_tgt->placed_address_space;