[PATCH 1/2] Set bp_tgt->reqstd_address and bp_tgt->placed_size in record_full_insert_breakpoint

[PATCH 1/2] Set bp_tgt->reqstd_address and bp_tgt->placed_size in record_full_insert_breakpoint

[Yao Qi]

I notice that bp_tgt won't be fully initialized if to_insert_breakpoint
isn't called in record_full_insert_breakpoint, and bp_tgt->reqstd_address
is zero, so an entry is added to record_full_breakpoints, but its address
is zero, which is wrong.  This patch is to call gdbarch_breakpoint_from_pc
in the else branch to set bp_tgt->reqstd_address and bp_tgt->placed_size.

gdb:

2016-04-04  Yao Qi  <yao.qi@linaro.org>

	* record-full.c (record_full_insert_breakpoint): Set
	bp_tgt->reqstd_address and bp_tgt->placed_size.
---
 gdb/record-full.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index f6023bf..066a8e7 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1670,6 +1670,16 @@ record_full_insert_breakpoint (struct target_ops *ops,
 
       in_target_beneath = 1;
     }
+  else
+    {
+      CORE_ADDR addr = bp_tgt->reqstd_address;
+      int bplen;
+
+      gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen);
+
+      bp_tgt->placed_address = addr;
+      bp_tgt->placed_size = bplen;
+    }
 
   bp = XNEW (struct record_full_breakpoint);
   bp->addr = bp_tgt->placed_address;
-- 
1.9.1

[PATCH 2/2] Make breakpoint handling in record-full idempotent

[Yao Qi]

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-04  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.
---
 gdb/record-full.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 066a8e7..9e7694e 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,20 @@ record_full_insert_breakpoint (struct target_ops *ops,
       bp_tgt->placed_size = bplen;
     }
 
+  /* Find any existing entries.  */
+  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;
-- 
1.9.1

[PATCH 0/2] Make breakpoint handling in record-full idempotent

[Yao Qi]

Software single step target (such as arm-linux) exposes the problem of
breakpoint handling in record target.  Patch 2 is the fix the problem,
and see details in it.  When patch 2 is applied, a new problem is
found, and patch 1 is the fix.

The whole series is tested on x86_64-linux and arm-linux.

*** BLURB HERE ***

Yao Qi (2):
  Set bp_tgt->reqstd_address and bp_tgt->placed_size in
    record_full_insert_breakpoint
  Make breakpoint handling in record-full idempotent

 gdb/record-full.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

-- 
1.9.1

Re: [PATCH 1/2] Set bp_tgt->reqstd_address and bp_tgt->placed_size in record_full_insert_breakpoint

[Luis Machado]

On 04/04/2016 03:38 AM, Yao Qi wrote:
> I notice that bp_tgt won't be fully initialized if to_insert_breakpoint
> isn't called in record_full_insert_breakpoint, and bp_tgt->reqstd_address
> is zero, so an entry is added to record_full_breakpoints, but its address
> is zero, which is wrong.  This patch is to call gdbarch_breakpoint_from_pc
> in the else branch to set bp_tgt->reqstd_address and bp_tgt->placed_size.
>
> gdb:
>
> 2016-04-04  Yao Qi  <yao.qi@linaro.org>
>
> 	* record-full.c (record_full_insert_breakpoint): Set
> 	bp_tgt->reqstd_address and bp_tgt->placed_size.
> ---
>   gdb/record-full.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index f6023bf..066a8e7 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -1670,6 +1670,16 @@ record_full_insert_breakpoint (struct target_ops *ops,
>
>         in_target_beneath = 1;
>       }
> +  else
> +    {
> +      CORE_ADDR addr = bp_tgt->reqstd_address;

Do we really need to get this initialized?

> +      int bplen;
> +
> +      gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen);
> +
> +      bp_tgt->placed_address = addr;
> +      bp_tgt->placed_size = bplen;
> +    }
>
>     bp = XNEW (struct record_full_breakpoint);
>     bp->addr = bp_tgt->placed_address;
>

Otherwise looks good.

Re: [PATCH 2/2] Make breakpoint handling in record-full idempotent

[Luis Machado]

On 04/04/2016 03:38 AM, Yao Qi wrote:
> 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-04  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.
> ---
>   gdb/record-full.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 066a8e7..9e7694e 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,20 @@ record_full_insert_breakpoint (struct target_ops *ops,
>         bp_tgt->placed_size = bplen;
>       }
>
> +  /* Find any existing entries.  */

Should this say...

/* Make sure there are no duplicate breakpoint entries.  */

... instead?

Otherwise it looks good to me.

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

Re: [PATCH 1/2] Set bp_tgt->reqstd_address and bp_tgt->placed_size in record_full_insert_breakpoint

[Yao Qi]

Luis Machado <lgustavo@codesourcery.com> writes:

>> +      CORE_ADDR addr = bp_tgt->reqstd_address;
>
> Do we really need to get this initialized?
>

Yes, 'addr' is passed to gdbarch_breakpoint_from_pc below.

>> +      int bplen;
>> +
>> +      gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen);
>> +
>> +      bp_tgt->placed_address = addr;
>> +      bp_tgt->placed_size = bplen;

-- 
Yao (齐尧)

Re: [PATCH 2/2] Make breakpoint handling in record-full idempotent

[Yao Qi]

Luis Machado <lgustavo@codesourcery.com> writes:

Hi Luis,

>> +  /* Find any existing entries.  */
>
> Should this say...
>
> /* Make sure there are no duplicate breakpoint entries.  */
>
> ... instead?

I put tow comments together,

    /* Use the existing entries if found in order to avoid duplication
       in record_full_breakpoints.  */

-- 
Yao (齐尧)

Re: [PATCH 2/2] Make breakpoint handling in record-full idempotent

[Luis Machado]

On 04/06/2016 03:50 AM, Yao Qi wrote:
> Luis Machado <lgustavo@codesourcery.com> writes:
>
> Hi Luis,
>
>>> +  /* Find any existing entries.  */
>>
>> Should this say...
>>
>> /* Make sure there are no duplicate breakpoint entries.  */
>>
>> ... instead?
>
> I put tow comments together,
>
>      /* Use the existing entries if found in order to avoid duplication
>         in record_full_breakpoints.  */
>

Thanks. That reads better.

Re: [PATCH 1/2] Set bp_tgt->reqstd_address and bp_tgt->placed_size in record_full_insert_breakpoint

[Luis Machado]

On 04/06/2016 03:36 AM, Yao Qi wrote:
> Luis Machado <lgustavo@codesourcery.com> writes:
>
>>> +      CORE_ADDR addr = bp_tgt->reqstd_address;
>>
>> Do we really need to get this initialized?
>>
>
> Yes, 'addr' is passed to gdbarch_breakpoint_from_pc below.
>

Ah, indeed. For a moment i thought addr was initialized within 
gdbarch_breakpoint_from_pc.

>>> +      int bplen;
>>> +
>>> +      gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen);
>>> +
>>> +      bp_tgt->placed_address = addr;
>>> +      bp_tgt->placed_size = bplen;
>

Re: [PATCH 0/2] Make breakpoint handling in record-full idempotent

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

> Software single step target (such as arm-linux) exposes the problem of
> breakpoint handling in record target.  Patch 2 is the fix the problem,
> and see details in it.  When patch 2 is applied, a new problem is
> found, and patch 1 is the fix.
>
> The whole series is tested on x86_64-linux and arm-linux.

I pushed them in.

-- 
Yao (齐尧)