[RFA/7.8] user breakpoint not inserted if software-single-step at same location

[RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

Hello,

with the following code...

    12    Nested;   -- break #1
    13    return I; -- break #2
    14  end;

(line 12 is a call to function Nested)

... we have noticed the following errorneous behavior on ppc-aix,
where, after having inserted a breakpoint at line 12 and line 13,
and continuing from the breakpoint at line 12, the program never
stops at line 13, running away until the program terminates:

    % gdb -q func
    (gdb) b func.adb:12
    Breakpoint 1 at 0x10000a24: file func.adb, line 12.
    (gdb) b func.adb:13
    Breakpoint 2 at 0x10000a28: file func.adb, line 13.
    (gdb) run
    Starting program: /[...]/func

    Breakpoint 1, func () at func.adb:12
    12        Nested;   -- break #1
    (gdb) c
    Continuing.
    [Inferior 1 (process 4128872) exited with code 02]

When resuming from the first breakpoint, GDB first tries to step
out of that first breakpoint. We rely on software single-stepping
on this platform, and it just so happens that the address of the
first software single-step breakpoint is the same as the user's
breakpoint #2 (0x10000a28). So, with infrun and target traces
turned on (but uninteresting traces snip'ed off), the "continue"
operation looks like this:

    (gdb) c
    ### First, we insert the user breakpoints (the second one is an internal
    ### breakpoint on __pthread_init). The first user breakpoint is not
    ### inserted as we need to step out of it first.
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000d03f3800, xxx) = 0
    ### Then we proceed with the step-out-of-breakpoint...
    infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [process 15335610] at 0x10000a24
    ### That's when we insert the SSS breakpoints...
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000100009ac, xxx) = 0
    ### ... then let the inferior resume...
    target_resume (15335610, continue, 0)
    infrun: wait_for_inferior ()
    target_wait (-1, status, options={}) = 15335610,   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: target_wait (-1, status) =
    infrun:   15335610 [process 15335610],
    infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    infrun: stop_pc = 0x100009ac
    ### At this point, we stopped at the second SSS breakpoint...
    target_stopped_by_watchpoint () = 0
    ### We remove the SSS breakpoints...
    target_remove_breakpoint (0x0000000010000a28, xxx) = 0
    target_remove_breakpoint (0x00000000100009ac, xxx) = 0
    target_stopped_by_watchpoint () = 0
    ### We find that we're not done, so we resume....
    infrun: no stepping, continue
    ### And thus insert the user breakpoints again, except we're not
    ### inserting the second breakpoint?!?
    target_insert_breakpoint (0x0000000010000a24, xxx) = 0
    infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 15335610] at 0x100009ac
    target_resume (-1, continue, 0)
    infrun: prepare_to_wait
    target_wait (-1, status, options={}) = 15335610,   status->kind = exited, status = 2

What happens is that the removal of the software single-step breakpoints
effectively removed the breakpoint instruction from inferior memory.
But because such breakpoints are inserted directly as raw breakpoints
rather than through the normal chain of breakpoints, we fail to notice
that one of the user breakpoints points to the same address and that
this user breakpoint is therefore effectively un-inserted. When
resuming after the single-step, GDB thinks that the user breakpoint
is still inserted and therefore does not need to insert it again.

This patch fixes the problem by avoiding the creation of a raw
breakpoint for any of the software single-step breakpoints when
their address correspond to a user breakpoint which has already
been inserted and whose address matches. The rest of the patch
adjusts the code removing the software single-step breakpoints
to take this possibility into account. As it happens, it simplifies
the code a little bit.

This patch does make one assumption, however, which is the fact
that user breakpoints get inserted before software single-step
breakpoints. I think this is a fair assumption, as software
single-step breakpoints get created as part of the target-step
operation (resume with step=1), which logically can only happen
after we've inserted all breakpoints.

For the record, this behavior started appearing after commit
31e77af205cf6564c2bf4c18400b4ca16bdf92cd (PR breakpoints/7143 -
Watchpoint does not trigger when first set) was applied, but
I have not checked whether it was really directly related to
that commit, or a latent issue.

gdb/ChangeLog:

        PR breakpoints/7143
        * breakpoint.c (non_sss_software_breakpoint_inserted_here_p):
        New function, extracted from software_breakpoint_inserted_here_p.
        (software_breakpoint_inserted_here_p): Remove factored out code
        by call to non_sss_software_breakpoint_inserted_here_p.
        (insert_single_step_breakpoint): Do nothing if a non-software-
        single-step breakpoint was already inserted at the same
        address.
        (remove_single_step_breakpoints): Adjust to take into account
        the fact that the first software single-step may not have been
        inserted.

Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux
with the official testsuite.

OK to commit?

Also, due to the nature of the regression (compared to 7.7), I think
we should wait for a resolution of this issue before creating the
gdb 7.8 release branch.

Thanks!
-- 
Joel

---
 gdb/breakpoint.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 676c7b8..f9dc413 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4153,12 +4153,12 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
   return 0;
 }
 
-/* This function returns non-zero iff there is a software breakpoint
-   inserted at PC.  */
+/* Ignoring software single-step breakpoints, return non-zero iff
+   there is a software breakpoint inserted at PC.  */
 
-int
-software_breakpoint_inserted_here_p (struct address_space *aspace,
-				     CORE_ADDR pc)
+static int
+non_sss_software_breakpoint_inserted_here_p (struct address_space *aspace,
+					     CORE_ADDR pc)
 {
   struct bp_location *bl, **blp_tmp;
 
@@ -4180,6 +4180,19 @@ software_breakpoint_inserted_here_p (struct address_space *aspace,
 	}
     }
 
+  return 0;
+}
+
+/* This function returns non-zero iff there is a software breakpoint
+   inserted at PC.  */
+
+int
+software_breakpoint_inserted_here_p (struct address_space *aspace,
+				     CORE_ADDR pc)
+{
+  if (non_sss_software_breakpoint_inserted_here_p (aspace, pc))
+    return 1;
+
   /* Also check for software single-step breakpoints.  */
   if (single_step_breakpoint_inserted_here_p (aspace, pc))
     return 1;
@@ -15149,6 +15162,13 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
 {
   void **bpt_p;
 
+  /* If a non-software-single-step breakpoint is inserted at the same
+     location as our next_pc, no need to insert an additional
+     raw breakpoint.  */
+
+  if (non_sss_software_breakpoint_inserted_here_p (aspace, next_pc))
+    return;
+
   if (single_step_breakpoints[0] == NULL)
     {
       bpt_p = &single_step_breakpoints[0];
@@ -15189,22 +15209,18 @@ single_step_breakpoints_inserted (void)
 void
 remove_single_step_breakpoints (void)
 {
-  gdb_assert (single_step_breakpoints[0] != NULL);
-
-  /* See insert_single_step_breakpoint for more about this deprecated
-     call.  */
-  deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
-				    single_step_breakpoints[0]);
-  single_step_gdbarch[0] = NULL;
-  single_step_breakpoints[0] = NULL;
+  int i;
 
-  if (single_step_breakpoints[1] != NULL)
-    {
-      deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
-					single_step_breakpoints[1]);
-      single_step_gdbarch[1] = NULL;
-      single_step_breakpoints[1] = NULL;
-    }
+  for (i = 0; i < 2; i++)
+    if (single_step_breakpoints[i] != NULL)
+      {
+	/* See insert_single_step_breakpoint for more about
+	   this deprecated call.  */
+	deprecated_remove_raw_breakpoint (single_step_gdbarch[i],
+					  single_step_breakpoints[i]);
+	single_step_gdbarch[i] = NULL;
+	single_step_breakpoints[i] = NULL;
+      }
 }
 
 /* Delete software single step breakpoints without removing them from
-- 
1.9.1

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 05/29/2014 09:11 PM, Joel Brobecker wrote:
> Hello,
> 
> with the following code...
> 
>     12    Nested;   -- break #1
>     13    return I; -- break #2
>     14  end;
> 
> (line 12 is a call to function Nested)
> 
> ... we have noticed the following errorneous behavior on ppc-aix,

"erroneous"

> This patch does make one assumption, however, which is the fact
> that user breakpoints get inserted before software single-step
> breakpoints. I think this is a fair assumption, as software
> single-step breakpoints get created as part of the target-step
> operation (resume with step=1), which logically can only happen
> after we've inserted all breakpoints.

Async/background execution breaks that assumption though.

E.g., say you do:

 (gdb) b *0xsame_addr_as_sss_bkpt
 (gdb) s &
 (gdb) del

That "del" runs while the target is single-stepping.  And it
might just delete the breakpoint that was at the same address
as a single-step breakpoint, before the single-step breakpoint
traps or gdb collects the trap.

If you try that on native GNU/Linux, it'll fail because
then GDB can't poke at memory while the program is running.
You can still trigger it with using two threads:

 (gdb) b *0xsame_addr_as_sss_bkpt
 (gdb) set scheduler-locking on
 (gdb) thread 1
 (gdb) s &
 (gdb) thread 2  // this thread is stopped, so we can poke at memory.
 (gdb) del

That's why I thought it'd be easier to do this in the "remove" path.

We can still bypass actually inserting the sss breakpoint in
the "insert" path if there's already another breakpoint there,
but we'll need to create/clone the location and its shadow buffer,
and then still handle the issue in the "remove" path.  I'm now
thinking that indeed we should implement that optimization, but not
for efficiency (this being a corner case, it's doubtful it
matters), but to cater for remote targets that might not handle
duplicate Z0 packets well.  They're supposed to be handled in
an idempotent manner, but even GDBserver had related issues
recently.

Oh, _but_!  If the target supports breakpoint evaluating
breakpoint conditions itself (target_supports_evaluation_of_breakpoint_conditions),
such as GDBserver, then we _need_ to send the duplicate
software single-step Z0 breakpoint, in case the non-sss breakpoint
that is already inserted at the same address was conditional, otherwise
the target is only going to report the breakpoint hit if the
non-sss breakpoint's condition evaluates true, while we need the
sss breakpoint to be unconditional.  (In this case we know for
sure that the target knows what to do with the duplicate Z0
packets, it's a requirement of the feature.)

In sum, in the "insert" path:

 - if !target_supports_evaluation_of_breakpoint_conditions; then
     optimize out the sss breakpoint if there's already a
     non-sss breakpoint inserted at the same address.
   else
     make sure to resend/reinsert the breakpoint sss breakpoint,
     even if there's already a non-sss in place, in case that other
     breakpoint was conditional.
   fi

And in the "remove" path:

 - if there's still a non-sss breakpoint inserted at the
   same address, then don't actually remove the breakpoint
   off of the target, just wipe it from gdb's list.

> Also, due to the nature of the regression (compared to 7.7), I think
> we should wait for a resolution of this issue before creating the
> gdb 7.8 release branch.

I agree.

Thanks,
-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

Hi Pedro,

Thanks for the super quick review!

> Async/background execution breaks that assumption though.

Hmmm, you are right. I had a feeling that this assumption was
going to come back to bite us one day. I didn't realize that
it was going to be today! ;-)

> We can still bypass actually inserting the sss breakpoint in
> the "insert" path if there's already another breakpoint there,
> but we'll need to create/clone the location and its shadow buffer,
> and then still handle the issue in the "remove" path.
[...]
> In sum, in the "insert" path:
> 
>  - if !target_supports_evaluation_of_breakpoint_conditions; then
>      optimize out the sss breakpoint if there's already a
>      non-sss breakpoint inserted at the same address.
>    else
>      make sure to resend/reinsert the breakpoint sss breakpoint,
>      even if there's already a non-sss in place, in case that other
>      breakpoint was conditional.
>    fi
> 
> And in the "remove" path:
> 
>  - if there's still a non-sss breakpoint inserted at the
>    same address, then don't actually remove the breakpoint
>    off of the target, just wipe it from gdb's list.

It seems to me that we'd need to merge your initial recommendation
into your summary above, right? Otherwise, wouldn't we fail in
the async example you provided? Actually, wouldn't it fail
regardless? Even if we inserted the SSS breakpoint, when the user
deletes his breakpoints, since the breakpoint chain doesn't know
about the SSS breakpoint, wouldn't it remove our SSS breakpoint
insn?

I am wondering whether the simpler approach that you initially
suggested, which is to just handle the issue in the "remove"
path for 7.8 wouldn't be a little safer, while we also look
at actually enhancing SSS breakpoints via the normal breakpoint
chain. I am wondering what's going to be needed for that...

WDYT?

Thanks!
-- 
Joel

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 05/30/2014 01:22 PM, Joel Brobecker wrote:

>> And in the "remove" path:
>>
>>  - if there's still a non-sss breakpoint inserted at the
>>    same address, then don't actually remove the breakpoint
>>    off of the target, just wipe it from gdb's list.
> 
> It seems to me that we'd need to merge your initial recommendation
> into your summary above, right?

I admit I don't know what recommendation you're referring to.  :-)

 Otherwise, wouldn't we fail in
> the async example you provided? Actually, wouldn't it fail
> regardless? Even if we inserted the SSS breakpoint, when the user
> deletes his breakpoints, since the breakpoint chain doesn't know
> about the SSS breakpoint, wouldn't it remove our SSS breakpoint
> insn?

Ah, yes.  We'd need the mirror treatment in bkpt_remove_location.

> 
> I am wondering whether the simpler approach that you initially
> suggested, which is to just handle the issue in the "remove"
> path for 7.8 wouldn't be a little safer

For 7.8, I'm thinking it's really safer to avoid resending
duplicate Z0 packets to stubs that don't support conditional
breakpoint evaluation on the target end.  So I think we should
handle the "insert" path too.

BTW, did you try creating a test for the issue you discovered?
I don't think we have anything that triggers this already in
the tree, otherwise I think I'd have seen it with my
software-single-step-on-x86 branch.

> , while we also look
> at actually enhancing SSS breakpoints via the normal breakpoint
> chain. I am wondering what's going to be needed for that...

I have done that at least two or three times before, and I was
never that happy with the result.  This was before the patch
that led to this regression, and that one and the surrounding
patches were actually result of exactly wanting to modernize
software single-step.  :-)  The main issue is that SSS breakpoints
and all other breakpoints need to be inserted/removed at
different times, and that is surprisingly tricky to handle.
But I'm hoping/assuming the codebase is a little bit more
ready now for the next attempt.  My main motivation is to
be able to enable non-stop without forcing displaced-stepping
for all single-steps (even those that don't step over a bkpt).

-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

> >>  - if there's still a non-sss breakpoint inserted at the
> >>    same address, then don't actually remove the breakpoint
> >>    off of the target, just wipe it from gdb's list.
> > 
> > It seems to me that we'd need to merge your initial recommendation
> > into your summary above, right?
> 
> I admit I don't know what recommendation you're referring to.  :-)

Sorry! This one:

    | but we'll need to create/clone the location and its shadow buffer,
    | and then still handle the issue in the "remove" path.

> For 7.8, I'm thinking it's really safer to avoid resending
> duplicate Z0 packets to stubs that don't support conditional
> breakpoint evaluation on the target end.  So I think we should
> handle the "insert" path too.

OK - I will take care of that.

> BTW, did you try creating a test for the issue you discovered?
> I don't think we have anything that triggers this already in
> the tree, otherwise I think I'd have seen it with my
> software-single-step-on-x86 branch.

I am wondering how to create that test, because it would be
a little tricky. We need to set ourselves into a situation
where we single-step out of a breakpoint with the second SSS
breakpoint being at the same address as one of the user breakpoints,
that second SSS not being the one that gets hit during that
first single-step-out-of-breakpoint.  The only way I can see
to achieve that, at the moment, is with a function call.
The only reliable way to do that, I think, is with an assembly
file, which means it'd be limited to a certain architecture.

> I have done that at least two or three times before, and I was
> never that happy with the result.  This was before the patch
> that led to this regression, and that one and the surrounding
> patches were actually result of exactly wanting to modernize
> software single-step.  :-)  The main issue is that SSS breakpoints
> and all other breakpoints need to be inserted/removed at
> different times, and that is surprisingly tricky to handle.
> But I'm hoping/assuming the codebase is a little bit more
> ready now for the next attempt.  My main motivation is to
> be able to enable non-stop without forcing displaced-stepping
> for all single-steps (even those that don't step over a bkpt).

Ugh! Much trickier than I thought! :-(

So, to summarize, I'll start by working on a new patch, which I'll
send here. I'll also try to put the new testcase on the list, but
today is a little bit of a full day for me, so that part might have
to wait until Monday depending on how well my day goes.

Thanks, Pedro!
-- 
Joel

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 05/30/2014 02:26 PM, Joel Brobecker wrote:
>>>>  - if there's still a non-sss breakpoint inserted at the
>>>>    same address, then don't actually remove the breakpoint
>>>>    off of the target, just wipe it from gdb's list.
>>>
>>> It seems to me that we'd need to merge your initial recommendation
>>> into your summary above, right?
>>
>> I admit I don't know what recommendation you're referring to.  :-)
> 
> Sorry! This one:
> 
>     | but we'll need to create/clone the location and its shadow buffer,
>     | and then still handle the issue in the "remove" path.

Ah.  Yes.

> I am wondering how to create that test, because it would be
> a little tricky. We need to set ourselves into a situation
> where we single-step out of a breakpoint with the second SSS
> breakpoint being at the same address as one of the user breakpoints,
> that second SSS not being the one that gets hit during that
> first single-step-out-of-breakpoint.  

Yeah.  Hmmm.  

*thinks*

I have a very simple idea, around "jump" + "always-inserted".

E.g., with, where b+ indicates a user breakpoint:

       00001 nop        <- PC
    b+ 00002 nop        

 - enable breakpoints always inserted mode
 - step to 00002
 - gdb removes the sss breakpoint.
 - due to always inserted mode, gdb does not remove b+, but
   due to the bug, it's actually no longer planted.
 - the b+ breakpoint should be reported to the user.
 - now do "jump $pc".
 - expected:
    The breakpoint should trigger immediately again.
 - what we get on sss targets:
    GDB loses control, and program runs to end.

Ah, I just went ahead and tried that against my by sss-on-x86
branch, and indeed it fails here, while it passes on
pristine mainline / hardware stepping.

8<----------
From e13bf4d64bf299111193a1f27a0bbc194d9b34f4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 30 May 2014 16:52:36 +0100
Subject: [PATCH] test for sss breakpoints bug

---
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.c   | 30 ++++++++++++++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp | 51 ++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
 create mode 100644 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp

diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
new file mode 100644
index 0000000..ff82051
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <signal.h>
+#include <unistd.h>
+
+int
+main (void)
+{
+  /* Assume writes to integers compile to a single instruction.  */
+  volatile int i = 0;
+
+  i = 1;     /* set foo break here */
+  i = 2;     /* set bar break here */
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
new file mode 100644
index 0000000..bb63d3f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
@@ -0,0 +1,51 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that removing a single-step breakpoint that is placed at the
+# same address as another regular breakpoint leaves the regular
+# breakpoint inserted.
+
+standard_testfile
+set executable ${testfile}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_breakpoint [gdb_get_line_number "set foo break here"]
+gdb_continue_to_breakpoint "first breakpoint" ".* set foo break here .*"
+
+gdb_breakpoint [gdb_get_line_number "set bar break here"]
+
+# So that GDB doesn't try to remove the regular breakpoint when the
+# step finishes.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# On software single-step targets, this step will want to momentarily
+# place a single-step breakpoint over the bar breakpoint, and then
+# remove it.  But, a regular breakpoint it planted there already, and
+# with always-inserted on, should remain planted when the step
+# finishes.
+gdb_test "si" "Breakpoint .* bar break .*"
+
+# If the breakpoint is still correctly inserted, then this jump should
+# re-trigger it.  Otherwise, GDB will lose control and the program
+# will exit.
+gdb_test "jump *\$pc" "Breakpoint .* bar break .*"
-- 
1.9.0

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

> I have a very simple idea, around "jump" + "always-inserted".
> 
> E.g., with, where b+ indicates a user breakpoint:
> 
>        00001 nop        <- PC
>     b+ 00002 nop        
> 
>  - enable breakpoints always inserted mode
>  - step to 00002
>  - gdb removes the sss breakpoint.
>  - due to always inserted mode, gdb does not remove b+, but
>    due to the bug, it's actually no longer planted.
>  - the b+ breakpoint should be reported to the user.
>  - now do "jump $pc".
>  - expected:
>     The breakpoint should trigger immediately again.
>  - what we get on sss targets:
>     GDB loses control, and program runs to end.
> 
> Ah, I just went ahead and tried that against my by sss-on-x86
> branch, and indeed it fails here, while it passes on
> pristine mainline / hardware stepping.

That is very clever! WDYT about pushing this new testcase now?

-- 
Joel

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 05/30/2014 04:57 PM, Pedro Alves wrote:

> Ah, I just went ahead and tried that against my by sss-on-x86
> branch, and indeed it fails here, while it passes on
> pristine mainline / hardware stepping.

I now filed bug 17000 for this , and went ahead and pushed the test
in, kfailed.

8<--------------
Subject: [PATCH] PR breakpoints/17000: user breakpoint not inserted if
 software-single-step at same location - test

GDB gets confused when removing a software single-step breakpoint that
is at the same address as another breakpoint.  Add a kfailed test.

gdb/testsuite/
2014-05-30  Pedro Alves  <palves@redhat.com>

	PR breakpoints/17000
	* gdb.base/sss-bp-on-user-bp.c: New file.
	* gdb.base/sss-bp-on-user-bp.exp: New file.
---
 gdb/testsuite/ChangeLog                      |  6 ++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.c   | 30 ++++++++++++++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp | 52 ++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
 create mode 100644 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b09e86e..334fd83 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2014-05-30  Pedro Alves  <palves@redhat.com>
+
+	PR breakpoints/17000
+	* gdb.base/sss-bp-on-user-bp.c: New file.
+	* gdb.base/sss-bp-on-user-bp.exp: New file.
+
 2014-05-30  David Blaikie  <dblaikie@gmail.com>
 
 	* gdb.opt/inline-break.c: Fix clang compatibility by specifying
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
new file mode 100644
index 0000000..ff82051
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <signal.h>
+#include <unistd.h>
+
+int
+main (void)
+{
+  /* Assume writes to integers compile to a single instruction.  */
+  volatile int i = 0;
+
+  i = 1;     /* set foo break here */
+  i = 2;     /* set bar break here */
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
new file mode 100644
index 0000000..62415e7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
@@ -0,0 +1,52 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that removing a single-step breakpoint that is placed at the
+# same address as another regular breakpoint leaves the regular
+# breakpoint inserted.
+
+standard_testfile
+set executable ${testfile}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_breakpoint [gdb_get_line_number "set foo break here"]
+gdb_continue_to_breakpoint "first breakpoint" ".* set foo break here .*"
+
+gdb_breakpoint [gdb_get_line_number "set bar break here"]
+
+# So that GDB doesn't try to remove the regular breakpoint when the
+# step finishes.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# On software single-step targets, this step will want to momentarily
+# place a single-step breakpoint over the bar breakpoint, and then
+# remove it.  But, a regular breakpoint is planted there already, and
+# with always-inserted on, should remain planted when the step
+# finishes.
+gdb_test "si" "Breakpoint .* bar break .*"
+
+# If the breakpoint is still correctly inserted, then this jump should
+# re-trigger it.  Otherwise, GDB will lose control and the program
+# will exit.
+setup_kfail "breakpoints/17000" "*-*-*"
+gdb_test "jump *\$pc" "Breakpoint .* bar break .*"
-- 
1.9.0

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 05/30/2014 05:19 PM, Joel Brobecker wrote:

> That is very clever! WDYT about pushing this new testcase now?

Done.  :-)

-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

Hi Pedro,

> > For 7.8, I'm thinking it's really safer to avoid resending
> > duplicate Z0 packets to stubs that don't support conditional
> > breakpoint evaluation on the target end.  So I think we should
> > handle the "insert" path too.
> 
> OK - I will take care of that.

New patch attached...

gdb/ChangeLog:

        PR breakpoints/17000
        * breakpoint.c (non_sss_software_breakpoint_inserted_here_p):
        New function, extracted from software_breakpoint_inserted_here_p.
        (software_breakpoint_inserted_here_p): Remove factored out code
        by call to non_sss_software_breakpoint_inserted_here_p.
        (insert_single_step_breakpoint): Do nothing if the target
        does not support target-side breakpoint condition evaluation,
        a a non-software- single-step breakpoint was already inserted
        at the same address.
        (remove_single_step_breakpoints): Adjust to take into account
        the fact that the first software single-step may not have been
        inserted.  Do not remove the raw breakpoint is a user software
        breakpoint is still inserted at the same location.

Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux
with the official testsuite. Also tested on x86_64-linux through
Pedro's branch enabling software single-stepping on that platform.

Does it look better to you? One thing that we might have to consider
is the fact that z0 and Z0 packets are no longer balanced; not sure
if it would matter to any stubb in practice. I think we're into
grey waters anyway...

Thanks!
-- 
Joel

[-- Attachment #2: 0001-User-breakpoint-ignored-if-software-single-step-at-s.patch --]
[-- Type: text/x-diff, Size: 9229 bytes --]

From 0fdd06aff3439fb5769f485f465f15cc2c6f7841 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 30 May 2014 11:27:19 -0700
Subject: [PATCH] User breakpoint ignored if software-single-step at same
 location

with the following code...

    12    Nested;   -- break #1
    13    return I; -- break #2
    14  end;

(line 12 is a call to function Nested)

... we have noticed the following errorneous behavior on ppc-aix,
where, after having inserted a breakpoint at line 12 and line 13,
and continuing from the breakpoint at line 12, the program never
stops at line 13, running away until the program terminates:

    % gdb -q func
    (gdb) b func.adb:12
    Breakpoint 1 at 0x10000a24: file func.adb, line 12.
    (gdb) b func.adb:13
    Breakpoint 2 at 0x10000a28: file func.adb, line 13.
    (gdb) run
    Starting program: /[...]/func

    Breakpoint 1, func () at func.adb:12
    12        Nested;   -- break #1
    (gdb) c
    Continuing.
    [Inferior 1 (process 4128872) exited with code 02]

When resuming from the first breakpoint, GDB first tries to step
out of that first breakpoint. We rely on software single-stepping
on this platform, and it just so happens that the address of the
first software single-step breakpoint is the same as the user's
breakpoint #2 (0x10000a28). So, with infrun and target traces
turned on (but uninteresting traces snip'ed off), the "continue"
operation looks like this:

    (gdb) c
    ### First, we insert the user breakpoints (the second one is an internal
    ### breakpoint on __pthread_init). The first user breakpoint is not
    ### inserted as we need to step out of it first.
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000d03f3800, xxx) = 0
    ### Then we proceed with the step-out-of-breakpoint...
    infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [process 15335610] at 0x10000a24
    ### That's when we insert the SSS breakpoints...
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000100009ac, xxx) = 0
    ### ... then let the inferior resume...
    target_resume (15335610, continue, 0)
    infrun: wait_for_inferior ()
    target_wait (-1, status, options={}) = 15335610,   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: target_wait (-1, status) =
    infrun:   15335610 [process 15335610],
    infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    infrun: stop_pc = 0x100009ac
    ### At this point, we stopped at the second SSS breakpoint...
    target_stopped_by_watchpoint () = 0
    ### We remove the SSS breakpoints...
    target_remove_breakpoint (0x0000000010000a28, xxx) = 0
    target_remove_breakpoint (0x00000000100009ac, xxx) = 0
    target_stopped_by_watchpoint () = 0
    ### We find that we're not done, so we resume....
    infrun: no stepping, continue
    ### And thus insert the user breakpoints again, except we're not
    ### inserting the second breakpoint?!?
    target_insert_breakpoint (0x0000000010000a24, xxx) = 0
    infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 15335610] at 0x100009ac
    target_resume (-1, continue, 0)
    infrun: prepare_to_wait
    target_wait (-1, status, options={}) = 15335610,   status->kind = exited, status = 2

What happens is that the removal of the software single-step breakpoints
effectively removed the breakpoint instruction from inferior memory.
But because such breakpoints are inserted directly as raw breakpoints
rather than through the normal chain of breakpoints, we fail to notice
that one of the user breakpoints points to the same address and that
this user breakpoint is therefore effectively un-inserted. When
resuming after the single-step, GDB thinks that the user breakpoint
is still inserted and therefore does not need to insert it again.

This patch fixes the problem by skipping the removal of the software
single-step raw breakpoints if at least one non-software-single-step
breakpoint is still inserted at that location.

Additionally, this patch also avoids inserting the single-step
breakpoint when possible, mostly to avoid issues with targets
not supporting multiple breakpoints at same address.  For instance,
some remote stubs might not support multiple Z0 packets at the same
address.

gdb/ChangeLog:

        PR breakpoints/17000
        * breakpoint.c (non_sss_software_breakpoint_inserted_here_p):
        New function, extracted from software_breakpoint_inserted_here_p.
        (software_breakpoint_inserted_here_p): Remove factored out code
        by call to non_sss_software_breakpoint_inserted_here_p.
        (insert_single_step_breakpoint): Do nothing if the target
        does not support target-side breakpoint condition evaluation,
        a a non-software- single-step breakpoint was already inserted
        at the same address.
        (remove_single_step_breakpoints): Adjust to take into account
        the fact that the first software single-step may not have been
        inserted.  Do not remove the raw breakpoint is a user software
        breakpoint is still inserted at the same location.

Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux
with the official testsuite. Also tested on x86_64-linux through
Pedro's branch enabling software single-stepping on that platform.
---
 gdb/breakpoint.c | 73 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 676c7b8..bddbca0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4153,12 +4153,12 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
   return 0;
 }
 
-/* This function returns non-zero iff there is a software breakpoint
-   inserted at PC.  */
+/* Ignoring software single-step breakpoints, return non-zero iff
+   there is a software breakpoint inserted at PC.  */
 
-int
-software_breakpoint_inserted_here_p (struct address_space *aspace,
-				     CORE_ADDR pc)
+static int
+non_sss_software_breakpoint_inserted_here_p (struct address_space *aspace,
+					     CORE_ADDR pc)
 {
   struct bp_location *bl, **blp_tmp;
 
@@ -4180,6 +4180,19 @@ software_breakpoint_inserted_here_p (struct address_space *aspace,
 	}
     }
 
+  return 0;
+}
+
+/* This function returns non-zero iff there is a software breakpoint
+   inserted at PC.  */
+
+int
+software_breakpoint_inserted_here_p (struct address_space *aspace,
+				     CORE_ADDR pc)
+{
+  if (non_sss_software_breakpoint_inserted_here_p (aspace, pc))
+    return 1;
+
   /* Also check for software single-step breakpoints.  */
   if (single_step_breakpoint_inserted_here_p (aspace, pc))
     return 1;
@@ -15149,6 +15162,20 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
 {
   void **bpt_p;
 
+  /* Unless the target supports target-side evaluation of breakpoint
+     conditions, there is no need to insert an additional raw breakpoint
+     if a non-software-single-step breakpoint is already inserted
+     at that location.
+
+     For targets supporting target-side evaluation of breakpoint
+     conditions, the issue is that those breakpoints cannot be
+     relied on to trigger during our software-single-step operation.
+     So we have to insert our own to make sure.  */
+
+  if (!target_supports_evaluation_of_breakpoint_conditions ()
+      && non_sss_software_breakpoint_inserted_here_p (aspace, next_pc))
+    return;
+
   if (single_step_breakpoints[0] == NULL)
     {
       bpt_p = &single_step_breakpoints[0];
@@ -15189,22 +15216,28 @@ single_step_breakpoints_inserted (void)
 void
 remove_single_step_breakpoints (void)
 {
-  gdb_assert (single_step_breakpoints[0] != NULL);
-
-  /* See insert_single_step_breakpoint for more about this deprecated
-     call.  */
-  deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
-				    single_step_breakpoints[0]);
-  single_step_gdbarch[0] = NULL;
-  single_step_breakpoints[0] = NULL;
+  int i;
 
-  if (single_step_breakpoints[1] != NULL)
-    {
-      deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
-					single_step_breakpoints[1]);
-      single_step_gdbarch[1] = NULL;
-      single_step_breakpoints[1] = NULL;
-    }
+  for (i = 0; i < 2; i++)
+    if (single_step_breakpoints[i] != NULL)
+      {
+	struct bp_target_info *bp_tgt = single_step_breakpoints[i];
+	CORE_ADDR pc = bp_tgt->placed_address;
+	struct address_space *aspace = bp_tgt->placed_address_space;
+
+	/* Only remove the raw breakpoint if there are no other
+	   non-software-single-step breakpoints still inserted
+	   at this location.  Otherwise, we would be effectively
+	   disabling those breakpoints.  */
+	if (!non_sss_software_breakpoint_inserted_here_p (aspace, pc))
+	  {
+	    /* See insert_single_step_breakpoint for more about
+	       this deprecated call.  */
+	    deprecated_remove_raw_breakpoint (single_step_gdbarch[i], bp_tgt);
+	  }
+	single_step_gdbarch[i] = NULL;
+	single_step_breakpoints[i] = NULL;
+      }
 }
 
 /* Delete software single step breakpoints without removing them from
-- 
1.9.1

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 05/30/2014 08:35 PM, Joel Brobecker wrote:
> Hi Pedro,
> 
>>> For 7.8, I'm thinking it's really safer to avoid resending
>>> duplicate Z0 packets to stubs that don't support conditional
>>> breakpoint evaluation on the target end.  So I think we should
>>> handle the "insert" path too.
>>
>> OK - I will take care of that.
> 
> New patch attached...
> 
> gdb/ChangeLog:
> 
>         PR breakpoints/17000
>         * breakpoint.c (non_sss_software_breakpoint_inserted_here_p):
>         New function, extracted from software_breakpoint_inserted_here_p.
>         (software_breakpoint_inserted_here_p): Remove factored out code
>         by call to non_sss_software_breakpoint_inserted_here_p.
>         (insert_single_step_breakpoint): Do nothing if the target
>         does not support target-side breakpoint condition evaluation,
>         a a non-software- single-step breakpoint was already inserted
>         at the same address.
>         (remove_single_step_breakpoints): Adjust to take into account
>         the fact that the first software single-step may not have been
>         inserted.  Do not remove the raw breakpoint is a user software
>         breakpoint is still inserted at the same location.
> 
> Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux
> with the official testsuite. Also tested on x86_64-linux through
> Pedro's branch enabling software single-stepping on that platform.
> 
> Does it look better to you? 

Hmm, this doesn't handle the background execution cases we
discussed earlier.  That is, with:

> @@ -15149,6 +15162,20 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
>  {
>    void **bpt_p;
>
> +  /* Unless the target supports target-side evaluation of breakpoint
> +     conditions, there is no need to insert an additional raw breakpoint
> +     if a non-software-single-step breakpoint is already inserted
> +     at that location.
> +
> +     For targets supporting target-side evaluation of breakpoint
> +     conditions, the issue is that those breakpoints cannot be
> +     relied on to trigger during our software-single-step operation.
> +     So we have to insert our own to make sure.  */
> +
> +  if (!target_supports_evaluation_of_breakpoint_conditions ()
> +      && non_sss_software_breakpoint_inserted_here_p (aspace, next_pc))
> +    return;

... when we hit this early return, the "b *0xaddr_where_sss_will_be_placed; s&; del"
case may well result in removing the sss breakpoint from the target before
the "step" actually finishes.  So I think we should still create a bp_target_info
for the single-step breakpoint, even though we don't actually insert it.
I think it'd then be better if we do this in
deprecated_insert_raw_breakpoint/deprecated_remove_raw_breakpoint instead,
and to handle the mirror scenario you identified ("when the user deletes
his breakpoints, since the breakpoint chain doesn't know about the SSS
breakpoint, wouldn't it remove our SSS breakpoint insn?")
in bkpt_insert_location/bkpt_remove_location.

Note that with that early return as it is in the patch,
single_step_breakpoints_inserted returns the wrong thing:

 int
 single_step_breakpoints_inserted (void)
 {
   return (single_step_breakpoints[0] != NULL
           || single_step_breakpoints[1] != NULL);
 }

E.g., assume we only have inserted one sss breakpoint,
and it happened to be on top of another breakpoint.  Then
single_step_breakpoints_inserted returns false, which confuses
the callers.

> One thing that we might have to consider
> is the fact that z0 and Z0 packets are no longer balanced; not sure
> if it would matter to any stubb in practice. I think we're into
> grey waters anyway...

About unbalanced: you're seeing that against gdbserver, right?  If so,
that's OK.  However, we shouldn't see unbalanced Z0/z0 with
"set remote conditional-breakpoints-packet off".  If we do, that'd
better be addressed.  See also
<https://sourceware.org/ml/gdb-patches/2014-04/msg00480.html>.

> @@ -15189,22 +15216,28 @@ single_step_breakpoints_inserted (void)
>  void
>  remove_single_step_breakpoints (void)
>  {
> -  gdb_assert (single_step_breakpoints[0] != NULL);

I think it's best if this assertion is preserved.

> -
> -  /* See insert_single_step_breakpoint for more about this deprecated
> -     call.  */
> -  deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
> -				    single_step_breakpoints[0]);
> -  single_step_gdbarch[0] = NULL;
> -  single_step_breakpoints[0] = NULL;
> +  int i;
>
> -  if (single_step_breakpoints[1] != NULL)
> -    {
> -      deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
> -					single_step_breakpoints[1]);
> -      single_step_gdbarch[1] = NULL;
> -      single_step_breakpoints[1] = NULL;
> -    }
> +  for (i = 0; i < 2; i++)
> +    if (single_step_breakpoints[i] != NULL)
> +      {
> +	struct bp_target_info *bp_tgt = single_step_breakpoints[i];
> +	CORE_ADDR pc = bp_tgt->placed_address;
> +	struct address_space *aspace = bp_tgt->placed_address_space;
> +
> +	/* Only remove the raw breakpoint if there are no other
> +	   non-software-single-step breakpoints still inserted
> +	   at this location.  Otherwise, we would be effectively
> +	   disabling those breakpoints.  */
> +	if (!non_sss_software_breakpoint_inserted_here_p (aspace, pc))
> +	  {
> +	    /* See insert_single_step_breakpoint for more about
> +	       this deprecated call.  */
> +	    deprecated_remove_raw_breakpoint (single_step_gdbarch[i], bp_tgt);
> +	  }

there should be an "else xfree" here, otherwise it's a leak.
But, the patch below ends up not touching this function though.

Let me know what you think, and feel free to push if it
looks OK.  Tested on all combinations of
native|gdbserver X hardware-|software- stepping.  At least,
I think I did.  If not this exact version, some other minor
variation.  :-)

8<--------
Subject: [PATCH] User breakpoint ignored if software-single-step at same
 location

with the following code...

    12    Nested;   -- break #1
    13    return I; -- break #2
    14  end;

(line 12 is a call to function Nested)

... we have noticed the following errorneous behavior on ppc-aix,
where, after having inserted a breakpoint at line 12 and line 13,
and continuing from the breakpoint at line 12, the program never
stops at line 13, running away until the program terminates:

    % gdb -q func
    (gdb) b func.adb:12
    Breakpoint 1 at 0x10000a24: file func.adb, line 12.
    (gdb) b func.adb:13
    Breakpoint 2 at 0x10000a28: file func.adb, line 13.
    (gdb) run
    Starting program: /[...]/func

    Breakpoint 1, func () at func.adb:12
    12        Nested;   -- break #1
    (gdb) c
    Continuing.
    [Inferior 1 (process 4128872) exited with code 02]

When resuming from the first breakpoint, GDB first tries to step out
of that first breakpoint.  We rely on software single-stepping on this
platform, and it just so happens that the address of the first
software single-step breakpoint is the same as the user's breakpoint
#2 (0x10000a28).  So, with infrun and target traces turned on (but
uninteresting traces snip'ed off), the "continue" operation looks like
this:

    (gdb) c
    ### First, we insert the user breakpoints (the second one is an internal
    ### breakpoint on __pthread_init). The first user breakpoint is not
    ### inserted as we need to step out of it first.
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000d03f3800, xxx) = 0
    ### Then we proceed with the step-out-of-breakpoint...
    infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [process 15335610] at 0x10000a24
    ### That's when we insert the SSS breakpoints...
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000100009ac, xxx) = 0
    ### ... then let the inferior resume...
    target_resume (15335610, continue, 0)
    infrun: wait_for_inferior ()
    target_wait (-1, status, options={}) = 15335610,   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: target_wait (-1, status) =
    infrun:   15335610 [process 15335610],
    infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    infrun: stop_pc = 0x100009ac
    ### At this point, we stopped at the second SSS breakpoint...
    target_stopped_by_watchpoint () = 0
    ### We remove the SSS breakpoints...
    target_remove_breakpoint (0x0000000010000a28, xxx) = 0
    target_remove_breakpoint (0x00000000100009ac, xxx) = 0
    target_stopped_by_watchpoint () = 0
    ### We find that we're not done, so we resume....
    infrun: no stepping, continue
    ### And thus insert the user breakpoints again, except we're not
    ### inserting the second breakpoint?!?
    target_insert_breakpoint (0x0000000010000a24, xxx) = 0
    infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 15335610] at 0x100009ac
    target_resume (-1, continue, 0)
    infrun: prepare_to_wait
    target_wait (-1, status, options={}) = 15335610,   status->kind = exited, status = 2

What happens is that the removal of the software single-step
breakpoints effectively removed the breakpoint instruction from
inferior memory.  But because such breakpoints are inserted directly
as raw breakpoints rather than through the normal chain of
breakpoints, we fail to notice that one of the user breakpoints points
to the same address and that this user breakpoint is therefore
effectively un-inserted.  When resuming after the single-step, GDB
thinks that the user breakpoint is still inserted and therefore does
not need to insert it again.

This patch teaches the insert and remove routines of both regular and
raw breakpoints to be aware of each other.  Special care needs to be
applied in case the target supports evaluation of breakpoint
conditions or commands.

gdb/ChangeLog:

	PR breakpoints/17000
	* breakpoint.c (find_non_raw_software_breakpoint_inserted_here):
	New function, extracted from software_breakpoint_inserted_here_p.
	(software_breakpoint_inserted_here_p): Replace factored out code
	by call to find_non_raw_software_breakpoint_inserted_here.
	(bkpt_insert_location): Handle the case of a single-step
	breakpoint already inserted at the same address.
	(bkpt_remove_location): Handle the case of a single-step
	breakpoint still inserted at the same address.
	(deprecated_insert_raw_breakpoint): Handle the case of non-raw
	breakpoint already inserted at the same address.
	(deprecated_remove_raw_breakpoint): Handle the case of a
	non-raw breakpoint still inserted at the same address.
	(single_step_breakpoint_inserted_here_p):
	(find_single_step_breakpoint_inserted_here): New function, fatored
	out from single_step_breakpoint_inserted_here_p.
	(single_step_breakpoint_inserted_here_p): Reimplement.

gdb/testsuite/ChangeLog:

	PR breakpoints/17000
	* gdb.base/sss-bp-on-user-bp.exp: Remove kfail.

Tested on ppc-aix with AdaCore's testsuite.  Tested on x86_64-linux,
(native and gdbserver) with the official testsuite.  Also tested on
x86_64-linux through Pedro's branch enabling software single-stepping
on that platform (native and gdbserver).
---
 gdb/breakpoint.c                             | 91 +++++++++++++++++++++++++---
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp |  3 +-
 2 files changed, 82 insertions(+), 12 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4e6c627..0043b06 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4165,12 +4165,12 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
   return 0;
 }

-/* This function returns non-zero iff there is a software breakpoint
-   inserted at PC.  */
+/* Ignoring deprecated raw breakpoints, return non-zero iff there is a
+   software breakpoint inserted at PC.  */

-int
-software_breakpoint_inserted_here_p (struct address_space *aspace,
-				     CORE_ADDR pc)
+static struct bp_location *
+find_non_raw_software_breakpoint_inserted_here (struct address_space *aspace,
+						CORE_ADDR pc)
 {
   struct bp_location *bl, **blp_tmp;

@@ -4188,10 +4188,23 @@ software_breakpoint_inserted_here_p (struct address_space *aspace,
 	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
 	  else
-	    return 1;
+	    return bl;
 	}
     }

+  return NULL;
+}
+
+/* This function returns non-zero iff there is a software breakpoint
+   inserted at PC.  */
+
+int
+software_breakpoint_inserted_here_p (struct address_space *aspace,
+				     CORE_ADDR pc)
+{
+  if (find_non_raw_software_breakpoint_inserted_here (aspace, pc) != NULL)
+    return 1;
+
   /* Also check for software single-step breakpoints.  */
   if (single_step_breakpoint_inserted_here_p (aspace, pc))
     return 1;
@@ -13119,8 +13132,18 @@ bkpt_insert_location (struct bp_location *bl)
     return target_insert_hw_breakpoint (bl->gdbarch,
 					&bl->target_info);
   else
-    return target_insert_breakpoint (bl->gdbarch,
-				     &bl->target_info);
+    {
+      struct bp_target_info *bp_tgt = &bl->target_info;
+      int ret;
+
+      /* There is no need to insert a breakpoint if an unconditional
+	 raw/sss breakpoint is already inserted at that location.  */
+      if (single_step_breakpoint_inserted_here_p (bp_tgt->placed_address_space,
+						  bp_tgt->placed_address))
+	return 0;
+
+      return target_insert_breakpoint (bl->gdbarch, bp_tgt);
+    }
 }

 static int
@@ -13129,7 +13152,19 @@ bkpt_remove_location (struct bp_location *bl)
   if (bl->loc_type == bp_loc_hardware_breakpoint)
     return target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info);
   else
-    return target_remove_breakpoint (bl->gdbarch, &bl->target_info);
+    {
+      struct bp_target_info *bp_tgt = &bl->target_info;
+      struct address_space *aspace = bp_tgt->placed_address_space;
+      CORE_ADDR address = bp_tgt->placed_address;
+
+      /* Only remove the breakpoint if there is no raw/sss breakpoint
+	 still inserted at this location.  Otherwise, we would be
+	 effectively disabling the raw/sss breakpoint.  */
+      if (single_step_breakpoint_inserted_here_p (aspace, address))
+	return 0;
+
+      return target_remove_breakpoint (bl->gdbarch, bp_tgt);
+    }
 }

 static int
@@ -15138,12 +15173,27 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
 				  struct address_space *aspace, CORE_ADDR pc)
 {
   struct bp_target_info *bp_tgt;
+  struct bp_location *bl;

   bp_tgt = XCNEW (struct bp_target_info);

   bp_tgt->placed_address_space = aspace;
   bp_tgt->placed_address = pc;

+  /* If an unconditional non-raw breakpoint is already inserted at
+     that location, there's no need to insert another.  However, with
+     target-side evaluation of breakpoint conditions, if the
+     breakpoint that is currently inserted on the target is
+     conditional, we need to make it unconditional.  Note that a
+     breakpoint with target-side commands is not reported even if
+     unconditional, so we need to remove the commands from the target
+     as well.  */
+  bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
+  if (bl != NULL
+      && VEC_empty (agent_expr_p, bl->target_info.conditions)
+      && VEC_empty (agent_expr_p, bl->target_info.tcommands))
+    return bp_tgt;
+
   if (target_insert_breakpoint (gdbarch, bp_tgt) != 0)
     {
       /* Could not insert the breakpoint.  */
@@ -15161,9 +15211,30 @@ int
 deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
 {
   struct bp_target_info *bp_tgt = bp;
+  struct address_space *aspace = bp_tgt->placed_address_space;
+  CORE_ADDR address = bp_tgt->placed_address;
+  struct bp_location *bl;
   int ret;

-  ret = target_remove_breakpoint (gdbarch, bp_tgt);
+  bl = find_non_raw_software_breakpoint_inserted_here (aspace, address);
+
+  /* Only remove the raw breakpoint if there are no other non-raw
+     breakpoints still inserted at this location.  Otherwise, we would
+     be effectively disabling those breakpoints.  */
+  if (bl == NULL)
+    ret = target_remove_breakpoint (gdbarch, bp_tgt);
+  else if (!VEC_empty (agent_expr_p, bl->target_info.conditions)
+	   || !VEC_empty (agent_expr_p, bl->target_info.tcommands))
+    {
+      /* The target is evaluating conditions, and when we inserted the
+	 software single-step breakpoint, we had made the breakpoint
+	 unconditional and command-less on the target side.  Reinsert
+	 to restore the conditions/commands.  */
+      ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info);
+    }
+  else
+    ret = 0;
+
   xfree (bp_tgt);

   return ret;
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
index 62415e7..2a12ad6 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
@@ -47,6 +47,5 @@ gdb_test "si" "Breakpoint .* bar break .*"

 # If the breakpoint is still correctly inserted, then this jump should
 # re-trigger it.  Otherwise, GDB will lose control and the program
-# will exit.
-setup_kfail "breakpoints/17000" "*-*-*"
+# will exit.  See PR breakpoints/17000.
 gdb_test "jump *\$pc" "Breakpoint .* bar break .*"
-- 
1.9.0

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/03/2014 12:16 AM, Pedro Alves wrote:

> Let me know what you think, and feel free to push if it
> looks OK.  Tested on all combinations of
> native|gdbserver X hardware-|software- stepping.  At least,
> I think I did.  If not this exact version, some other minor
> variation.  :-)

Bah, I woke up realizing that the version I posted forgets to
clone the shadow buffer!  Let me fix that and repost...

-- 
Pedro Alves

[pushed] PR breakpoints/17000: user breakpoint not inserted if software-single-step at same location - another test

[Pedro Alves]

On 06/03/2014 09:22 AM, Pedro Alves wrote:
> On 06/03/2014 12:16 AM, Pedro Alves wrote:
> 
>> Let me know what you think, and feel free to push if it
>> looks OK.  Tested on all combinations of
>> native|gdbserver X hardware-|software- stepping.  At least,
>> I think I did.  If not this exact version, some other minor
>> variation.  :-)
> 
> Bah, I woke up realizing that the version I posted forgets to
> clone the shadow buffer!  Let me fix that and repost...
> 

I managed to come up with a test that exposed this.  Basically, it does:

(gdb) disassemble test
Dump of assembler code for function test:
   0x0000000000400766 <+0>:     push   %rbp
   0x0000000000400767 <+1>:     mov    %rsp,%rbp
=> 0x000000000040076a <+4>:     nop
   0x000000000040076b <+5>:     nop
   0x000000000040076c <+6>:     pop    %rbp
   0x000000000040076d <+7>:     retq   
End of assembler dump.

(gdb) si&
(gdb) del $bpnum

(gdb) disassemble test
Dump of assembler code for function test:
   0x0000000000400766 <+0>:     push   %rbp
   0x0000000000400767 <+1>:     mov    %rsp,%rbp
   0x000000000040076a <+4>:     nop
=> 0x000000000040076b <+5>:     int3   
   0x000000000040076c <+6>:     pop    %rbp
   0x000000000040076d <+7>:     retq   
End of assembler dump.

And note the bogus "int3" in the second disassemble output.

The test actually fails in a different way currently against
gdbserver:

(gdb) PASS: gdb.base/sss-bp-on-user-bp-2.exp: define stepi_del_break
stepi_del_break
Cannot execute this command while the target is running.
(gdb) FAIL: gdb.base/sss-bp-on-user-bp-2.exp: stepi_del_break

The error is because GDB tried to remove the breakpoint from
memory while the thread was running, and we can't talk to the
server in the all-stop RSP until we get a stop reply, but in any
case, GDB shouldn't even be attempting to remove the breakpoint,
exactly because there was a sss breakpoint at the same address.

I've added a kfail, and pushed it.

8<--------------
PR breakpoints/17000: user breakpoint not inserted if software-single-step at same location - test

GDB gets confused when removing a software single-step breakpoint that
is at the same address as another breakpoint.  Add another kfailed
test.

gdb/testsuite/
2014-06-03  Pedro Alves  <palves@redhat.com>

	PR breakpoints/17000
	* gdb.base/sss-bp-on-user-bp-2.c: New file.
	* gdb.base/sss-bp-on-user-bp-2.exp: New file.
---
 gdb/testsuite/ChangeLog                        |   6 ++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.c   |  29 +++++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp | 109 +++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.c
 create mode 100644 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 9ded6eb..644f5ac 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2014-06-03  Pedro Alves  <palves@redhat.com>
+
+	PR breakpoints/17000
+	* gdb.base/sss-bp-on-user-bp-2.c: New file.
+	* gdb.base/sss-bp-on-user-bp-2.exp: New file.
+
 2014-06-02  Doug Evans  <xdje42@gmail.com>
 
 	* gdb.guile/scm-parameter.exp: New file.
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.c b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.c
new file mode 100644
index 0000000..c3c26fe
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+static void
+test (void)
+{
+  label: asm ("  nop"); label2: asm ("  nop"); /* must be a single line */
+}
+
+int
+main (void)
+{
+  test ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
new file mode 100644
index 0000000..a129bb7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -0,0 +1,109 @@
+# Copyright (C) 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that GDB doesn't get confused in the following scenario
+# (PR breakpoints/17000).  Say, we have this program:
+#
+#  =>  0xff000001  INSN1
+#      0xff000002  INSN2
+#
+# The PC currently points at INSN1.
+#
+# 1 - User sets a breakpoint at 0xff000002 (INSN2).
+#
+# 2 - User steps.  On software single-step archs, this sets a software
+#     single-step breakpoint at 0xff000002 (INSN2) too.
+#
+# 3 - User deletes breakpoint (INSN2) before the single-step finishes.
+#
+# 4 - The single-step finishes, and GDB removes the single-step
+#     breakpoint.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main"
+    return 0
+}
+
+set line_re "\[^\r\n\]*"
+
+gdb_test "b test:label" "Breakpoint .*"
+gdb_continue_to_breakpoint "run past setup"
+delete_breakpoints
+
+# So we can precisely control breakpoint insertion order.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# Capture disassembly output.  PREFIX is used as test prefix.  The
+# current instruction indicator (=>) is stripped away.
+proc disassemble { prefix } {
+    with_test_prefix "$prefix" {
+	set output [capture_command_output "disassemble test" ""]
+	return [string map {"=>" "  "} $output]
+    }
+}
+
+# Issue a stepi and immediately delete the user breakpoint that is set
+# at the same address as the software single-step breakpoint.  Do this
+# in a user defined command, so that the stepi's trap doesn't have a
+# chance to be handled before further input is processed.  We then
+# compare before/after disassembly.  GDB should be able to handle
+# deleting the user breakpoint before deleting the single-step
+# breakpoint.  E.g., we shouldn't see breakpoint instructions in the
+# disassembly.
+
+set disasm_before [disassemble "before"]
+
+gdb_test "b test:label2" ".*" "set breakpoint where si will land"
+
+set test "define stepi_del_break"
+gdb_test_multiple $test $test {
+    -re "Type commands for definition of \"stepi_del_break\".\r\nEnd with a line saying just \"end\".\r\n>$" {
+	gdb_test "si&\ndel \$bpnum\nend" "" $test
+    }
+}
+
+set command "stepi_del_break"
+set test $command
+setup_kfail "breakpoints/17000" "*-*-*"
+gdb_test_multiple $command $test {
+    -re "^$command\r\n$gdb_prompt " {
+	# Note no end anchor, because "si&" finishes and prints the
+	# current frame/line after the prompt is printed.
+	pass $test
+    }
+}
+
+# Now consume the output of the finished "si&".
+set test "si& finished"
+gdb_test_multiple "" $test {
+    -re "must be a single line \\\*/\r\n" {
+	pass $test
+    }
+}
+
+set disasm_after [disassemble "after"]
+
+set test "before/after disassembly matches"
+if ![string compare $disasm_before $disasm_after]  {
+    pass $test
+} else {
+    fail $test
+}
-- 
1.9.0

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Yao Qi]

On 05/30/2014 11:57 PM, Pedro Alves wrote:
> +int
> +main (void)
> +{
> +  /* Assume writes to integers compile to a single instruction.  */

This assumption is wrong on arm at least.

> +  volatile int i = 0;
> +
> +  i = 1;     /* set foo break here */
> +  i = 2;     /* set bar break here */
> +  return 0;
> +}

Each line is compiled to two instructions.

27        i = 1;     /* set foo break here */
   0x0000025c <+20>:    mov     r3, #1
   0x00000260 <+24>:    str     r3, [r11, #-8]

28        i = 2;     /* set bar break here */
   0x00000264 <+28>:    mov     r3, #2
   0x00000268 <+32>:    str     r3, [r11, #-8]

> +# On software single-step targets, this step will want to momentarily
> +# place a single-step breakpoint over the bar breakpoint, and then
> +# remove it.  But, a regular breakpoint it planted there already, and
> +# with always-inserted on, should remain planted when the step
> +# finishes.
> +gdb_test "si" "Breakpoint .* bar break .*"

this test will fail, because it still stops at "foo break".

-- 
Yao (齐尧)

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/03/2014 12:53 PM, Yao Qi wrote:
> On 05/30/2014 11:57 PM, Pedro Alves wrote:
>> +int
>> +main (void)
>> +{
>> +  /* Assume writes to integers compile to a single instruction.  */
> 
> This assumption is wrong on arm at least.

Ah, thanks.  We need to replace then with asm("nop") then.

-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Andreas Schwab]

Pedro Alves <palves@redhat.com> writes:

> Ah, thanks.  We need to replace then with asm("nop") then.

nop isn't portable.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/03/2014 01:12 PM, Andreas Schwab wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Ah, thanks.  We need to replace then with asm("nop") then.
> 
> nop isn't portable.

Yes, but it doesn't matter what the instruction is as
long as it's a single instruction that doesn't do much.
For archs that don't have "nop" (like e.g., IA64), we can
just use #ifdef to pick another insn.

-- 
Pedro Alves

Re: [pushed] PR breakpoints/17000: user breakpoint not inserted if software-single-step at same location - another test

[Pedro Alves]

On 06/03/2014 12:53 PM, Pedro Alves wrote:

> (gdb) PASS: gdb.base/sss-bp-on-user-bp-2.exp: define stepi_del_break
> stepi_del_break
> Cannot execute this command while the target is running.
> (gdb) FAIL: gdb.base/sss-bp-on-user-bp-2.exp: stepi_del_break
> 
> The error is because GDB tried to remove the breakpoint from
> memory while the thread was running, and we can't talk to the
> server in the all-stop RSP until we get a stop reply, but in any
> case, GDB shouldn't even be attempting to remove the breakpoint,
> exactly because there was a sss breakpoint at the same address.
> 
> I've added a kfail, and pushed it.

Gosh, so many different mode combinations...  So the fix for
PR17000 doesn't fix that failure when testing against gdbserver
_and_ hardware stepping.  Well, of course it doesn't, because in
that case GDB will really try to remove the breakpoint, because
there's no overlapping software single-step breakpoint.

I've pushed this to detect the scenario and skip the test.

8<---------------------
Subject: [PATCH] Skip sss-bp-on-user-bp-2.exp on remote hardware step targets.

gdb/testsuite/
2014-06-03  Pedro Alves  <palves@redhat.com>

	* gdb.base/sss-bp-on-user-bp-2.exp: Skip if testing with a remote
	target that doesn't use software single-stepping.
---
 gdb/testsuite/ChangeLog                        |  5 ++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp | 39 ++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 644f5ac..8217403 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2014-06-03  Pedro Alves  <palves@redhat.com>
 
+	* gdb.base/sss-bp-on-user-bp-2.exp: Skip if testing with a remote
+	target that doesn't use software single-stepping.
+
+2014-06-03  Pedro Alves  <palves@redhat.com>
+
 	PR breakpoints/17000
 	* gdb.base/sss-bp-on-user-bp-2.c: New file.
 	* gdb.base/sss-bp-on-user-bp-2.exp: New file.
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index a129bb7..b41f86e 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -42,6 +42,45 @@ if ![runto_main] {
     return 0
 }
 
+delete_breakpoints
+
+# With the all-stop RSP, we can't talk to the target while it's
+# running, until we get back the stop reply.  If not using single-step
+# breakpoints, then the "del" in stepi_del_break below will try to
+# delete the user breakpoint from the target, which will fail, with
+# "Cannot execute this command while the target is running.".  On
+# software single-step targets, that del shouldn't trigger any RSP
+# traffic.  Just skip the test if testing against a remove target and
+# not using software single-stepping.  IOW, skip the test if we see a
+# 'vCont;s' or 's' in the RSP traffic.
+
+gdb_test_no_output "set debug remote 1"
+
+set rsp_hardware_step 0
+
+# Probe for software single-step breakpoint use.
+set test "probe RSP hardware step"
+gdb_test_multiple "si" $test {
+    -re "\\\$vCont;s.*$gdb_prompt $" {
+	set rsp_hardware_step 1
+	pass $test
+    }
+    -re "\\\$s#.*$gdb_prompt $" {
+	set rsp_hardware_step 1
+	pass $test
+    }
+    -re "$gdb_prompt $" {
+	pass $test
+    }
+}
+
+if { $rsp_hardware_step } {
+    unsupported "remote target doesn't use software single-stepping"
+    return
+}
+
+gdb_test_no_output "set debug remote 0"
+
 set line_re "\[^\r\n\]*"
 
 gdb_test "b test:label" "Breakpoint .*"
-- 
1.9.0



-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/03/2014 09:22 AM, Pedro Alves wrote:
> On 06/03/2014 12:16 AM, Pedro Alves wrote:
> 
>> Let me know what you think, and feel free to push if it
>> looks OK.  Tested on all combinations of
>> native|gdbserver X hardware-|software- stepping.  At least,
>> I think I did.  If not this exact version, some other minor
>> variation.  :-)
> 
> Bah, I woke up realizing that the version I posted forgets to
> clone the shadow buffer!  Let me fix that and repost...
> 

Here is the fixed version.  Retested on all combinations of
native|gdbserver X hardware-|software- stepping on x86_64 Fedora 20.

Fixes both new tests.

8<------------
Subject: [PATCH] User breakpoint ignored if software-single-step at same
 location

with the following code...

    12    Nested;   -- break #1
    13    return I; -- break #2
    14  end;

(line 12 is a call to function Nested)

... we have noticed the following errorneous behavior on ppc-aix,
where, after having inserted a breakpoint at line 12 and line 13,
and continuing from the breakpoint at line 12, the program never
stops at line 13, running away until the program terminates:

    % gdb -q func
    (gdb) b func.adb:12
    Breakpoint 1 at 0x10000a24: file func.adb, line 12.
    (gdb) b func.adb:13
    Breakpoint 2 at 0x10000a28: file func.adb, line 13.
    (gdb) run
    Starting program: /[...]/func

    Breakpoint 1, func () at func.adb:12
    12        Nested;   -- break #1
    (gdb) c
    Continuing.
    [Inferior 1 (process 4128872) exited with code 02]

When resuming from the first breakpoint, GDB first tries to step out
of that first breakpoint.  We rely on software single-stepping on this
platform, and it just so happens that the address of the first
software single-step breakpoint is the same as the user's breakpoint
#2 (0x10000a28).  So, with infrun and target traces turned on (but
uninteresting traces snip'ed off), the "continue" operation looks like
this:

    (gdb) c
    ### First, we insert the user breakpoints (the second one is an internal
    ### breakpoint on __pthread_init). The first user breakpoint is not
    ### inserted as we need to step out of it first.
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000d03f3800, xxx) = 0
    ### Then we proceed with the step-out-of-breakpoint...
    infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [process 15335610] at 0x10000a24
    ### That's when we insert the SSS breakpoints...
    target_insert_breakpoint (0x0000000010000a28, xxx) = 0
    target_insert_breakpoint (0x00000000100009ac, xxx) = 0
    ### ... then let the inferior resume...
    target_resume (15335610, continue, 0)
    infrun: wait_for_inferior ()
    target_wait (-1, status, options={}) = 15335610,   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: target_wait (-1, status) =
    infrun:   15335610 [process 15335610],
    infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    infrun: stop_pc = 0x100009ac
    ### At this point, we stopped at the second SSS breakpoint...
    target_stopped_by_watchpoint () = 0
    ### We remove the SSS breakpoints...
    target_remove_breakpoint (0x0000000010000a28, xxx) = 0
    target_remove_breakpoint (0x00000000100009ac, xxx) = 0
    target_stopped_by_watchpoint () = 0
    ### We find that we're not done, so we resume....
    infrun: no stepping, continue
    ### And thus insert the user breakpoints again, except we're not
    ### inserting the second breakpoint?!?
    target_insert_breakpoint (0x0000000010000a24, xxx) = 0
    infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 15335610] at 0x100009ac
    target_resume (-1, continue, 0)
    infrun: prepare_to_wait
    target_wait (-1, status, options={}) = 15335610,   status->kind = exited, status = 2

What happens is that the removal of the software single-step
breakpoints effectively removed the breakpoint instruction from
inferior memory.  But because such breakpoints are inserted directly
as raw breakpoints rather than through the normal chain of
breakpoints, we fail to notice that one of the user breakpoints points
to the same address and that this user breakpoint is therefore
effectively un-inserted.  When resuming after the single-step, GDB
thinks that the user breakpoint is still inserted and therefore does
not need to insert it again.

This patch teaches the insert and remove routines of both regular and
raw breakpoints to be aware of each other.  Special care needs to be
applied in case the target supports evaluation of breakpoint
conditions or commands.

gdb/ChangeLog:

	PR breakpoints/17000
	* breakpoint.c (find_non_raw_software_breakpoint_inserted_here):
	New function, extracted from software_breakpoint_inserted_here_p.
	(software_breakpoint_inserted_here_p): Replace factored out code
	by call to find_non_raw_software_breakpoint_inserted_here.
	(bp_target_info_copy_insertion_state): New function.
	(bkpt_insert_location): Handle the case of a single-step
	breakpoint already inserted at the same address.
	(bkpt_remove_location): Handle the case of a single-step
	breakpoint still inserted at the same address.
	(deprecated_insert_raw_breakpoint): Handle the case of non-raw
	breakpoint already inserted at the same address.
	(deprecated_remove_raw_breakpoint): Handle the case of a
	non-raw breakpoint still inserted at the same address.
	(find_single_step_breakpoint): New function, extracted from
	single_step_breakpoint_inserted_here_p.
	(find_single_step_breakpoint): New function,
	factored out from single_step_breakpoint_inserted_here_p.
	(single_step_breakpoint_inserted_here_p): Reimplement.

gdb/testsuite/ChangeLog:

	PR breakpoints/17000
	* gdb.base/sss-bp-on-user-bp.exp: Remove kfail.
	* gdb.base/sss-bp-on-user-bp-2.exp: Remove kfail.

Tested on ppc-aix with AdaCore's testsuite.  Tested on x86_64-linux,
(native and gdbserver) with the official testsuite.  Also tested on
x86_64-linux through Pedro's branch enabling software single-stepping
on that platform (native and gdbserver).
---
 gdb/breakpoint.c                               | 141 ++++++++++++++++++++++---
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp |   1 -
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp   |   3 +-
 3 files changed, 125 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4e6c627..23c8895 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -229,6 +229,9 @@ static void tcatch_command (char *arg, int from_tty);
 
 static void detach_single_step_breakpoints (void);
 
+static int find_single_step_breakpoint (struct address_space *aspace,
+					CORE_ADDR pc);
+
 static void free_bp_location (struct bp_location *loc);
 static void incref_bp_location (struct bp_location *loc);
 static void decref_bp_location (struct bp_location **loc);
@@ -4165,12 +4168,12 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
   return 0;
 }
 
-/* This function returns non-zero iff there is a software breakpoint
-   inserted at PC.  */
+/* Ignoring deprecated raw breakpoints, return non-zero iff there is a
+   software breakpoint inserted at PC.  */
 
-int
-software_breakpoint_inserted_here_p (struct address_space *aspace,
-				     CORE_ADDR pc)
+static struct bp_location *
+find_non_raw_software_breakpoint_inserted_here (struct address_space *aspace,
+						CORE_ADDR pc)
 {
   struct bp_location *bl, **blp_tmp;
 
@@ -4188,10 +4191,23 @@ software_breakpoint_inserted_here_p (struct address_space *aspace,
 	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
 	  else
-	    return 1;
+	    return bl;
 	}
     }
 
+  return NULL;
+}
+
+/* This function returns non-zero iff there is a software breakpoint
+   inserted at PC.  */
+
+int
+software_breakpoint_inserted_here_p (struct address_space *aspace,
+				     CORE_ADDR pc)
+{
+  if (find_non_raw_software_breakpoint_inserted_here (aspace, pc) != NULL)
+    return 1;
+
   /* Also check for software single-step breakpoints.  */
   if (single_step_breakpoint_inserted_here_p (aspace, pc))
     return 1;
@@ -13112,6 +13128,19 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+/* Copy SRC's shadow buffer and whatever else we'd set if we actually
+   inserted DEST, so we can remove it later, in case SRC is removed
+   first.  */
+
+static void
+bp_target_info_copy_insertion_state (struct bp_target_info *dest,
+				     const struct bp_target_info *src)
+{
+  dest->shadow_len = src->shadow_len;
+  memcpy (dest->shadow_contents, src->shadow_contents, src->shadow_len);
+  dest->placed_size = src->placed_size;
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -13119,8 +13148,25 @@ bkpt_insert_location (struct bp_location *bl)
     return target_insert_hw_breakpoint (bl->gdbarch,
 					&bl->target_info);
   else
-    return target_insert_breakpoint (bl->gdbarch,
-				     &bl->target_info);
+    {
+      struct bp_target_info *bp_tgt = &bl->target_info;
+      int ret;
+      int sss_slot;
+
+      /* There is no need to insert a breakpoint if an unconditional
+	 raw/sss breakpoint is already inserted at that location.  */
+      sss_slot = find_single_step_breakpoint (bp_tgt->placed_address_space,
+					      bp_tgt->placed_address);
+      if (sss_slot >= 0)
+	{
+	  struct bp_target_info *sss_bp_tgt = single_step_breakpoints[sss_slot];
+
+	  bp_target_info_copy_insertion_state (bp_tgt, sss_bp_tgt);
+	  return 0;
+	}
+
+      return target_insert_breakpoint (bl->gdbarch, bp_tgt);
+    }
 }
 
 static int
@@ -13129,7 +13175,19 @@ bkpt_remove_location (struct bp_location *bl)
   if (bl->loc_type == bp_loc_hardware_breakpoint)
     return target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info);
   else
-    return target_remove_breakpoint (bl->gdbarch, &bl->target_info);
+    {
+      struct bp_target_info *bp_tgt = &bl->target_info;
+      struct address_space *aspace = bp_tgt->placed_address_space;
+      CORE_ADDR address = bp_tgt->placed_address;
+
+      /* Only remove the breakpoint if there is no raw/sss breakpoint
+	 still inserted at this location.  Otherwise, we would be
+	 effectively disabling the raw/sss breakpoint.  */
+      if (single_step_breakpoint_inserted_here_p (aspace, address))
+	return 0;
+
+      return target_remove_breakpoint (bl->gdbarch, bp_tgt);
+    }
 }
 
 static int
@@ -15138,12 +15196,30 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
 				  struct address_space *aspace, CORE_ADDR pc)
 {
   struct bp_target_info *bp_tgt;
+  struct bp_location *bl;
 
   bp_tgt = XCNEW (struct bp_target_info);
 
   bp_tgt->placed_address_space = aspace;
   bp_tgt->placed_address = pc;
 
+  /* If an unconditional non-raw breakpoint is already inserted at
+     that location, there's no need to insert another.  However, with
+     target-side evaluation of breakpoint conditions, if the
+     breakpoint that is currently inserted on the target is
+     conditional, we need to make it unconditional.  Note that a
+     breakpoint with target-side commands is not reported even if
+     unconditional, so we need to remove the commands from the target
+     as well.  */
+  bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
+  if (bl != NULL
+      && VEC_empty (agent_expr_p, bl->target_info.conditions)
+      && VEC_empty (agent_expr_p, bl->target_info.tcommands))
+    {
+      bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
+      return bp_tgt;
+    }
+
   if (target_insert_breakpoint (gdbarch, bp_tgt) != 0)
     {
       /* Could not insert the breakpoint.  */
@@ -15161,9 +15237,30 @@ int
 deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
 {
   struct bp_target_info *bp_tgt = bp;
+  struct address_space *aspace = bp_tgt->placed_address_space;
+  CORE_ADDR address = bp_tgt->placed_address;
+  struct bp_location *bl;
   int ret;
 
-  ret = target_remove_breakpoint (gdbarch, bp_tgt);
+  bl = find_non_raw_software_breakpoint_inserted_here (aspace, address);
+
+  /* Only remove the raw breakpoint if there are no other non-raw
+     breakpoints still inserted at this location.  Otherwise, we would
+     be effectively disabling those breakpoints.  */
+  if (bl == NULL)
+    ret = target_remove_breakpoint (gdbarch, bp_tgt);
+  else if (!VEC_empty (agent_expr_p, bl->target_info.conditions)
+	   || !VEC_empty (agent_expr_p, bl->target_info.tcommands))
+    {
+      /* The target is evaluating conditions, and when we inserted the
+	 software single-step breakpoint, we had made the breakpoint
+	 unconditional and command-less on the target side.  Reinsert
+	 to restore the conditions/commands.  */
+      ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info);
+    }
+  else
+    ret = 0;
+
   xfree (bp_tgt);
 
   return ret;
@@ -15269,12 +15366,12 @@ detach_single_step_breakpoints (void)
 				single_step_breakpoints[i]);
 }
 
-/* Check whether a software single-step breakpoint is inserted at
-   PC.  */
+/* Find the software single-step breakpoint that inserted at PC.
+   Returns its slot if found, and -1 if not found.  */
 
-int
-single_step_breakpoint_inserted_here_p (struct address_space *aspace, 
-					CORE_ADDR pc)
+static int
+find_single_step_breakpoint (struct address_space *aspace,
+			     CORE_ADDR pc)
 {
   int i;
 
@@ -15285,10 +15382,20 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace,
 	  && breakpoint_address_match (bp_tgt->placed_address_space,
 				       bp_tgt->placed_address,
 				       aspace, pc))
-	return 1;
+	return i;
     }
 
-  return 0;
+  return -1;
+}
+
+/* Check whether a software single-step breakpoint is inserted at
+   PC.  */
+
+int
+single_step_breakpoint_inserted_here_p (struct address_space *aspace,
+					CORE_ADDR pc)
+{
+  return find_single_step_breakpoint (aspace, pc) >= 0;
 }
 
 /* Returns 0 if 'bp' is NOT a syscall catchpoint,
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index b41f86e..87fa5f8 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -121,7 +121,6 @@ gdb_test_multiple $test $test {
 
 set command "stepi_del_break"
 set test $command
-setup_kfail "breakpoints/17000" "*-*-*"
 gdb_test_multiple $command $test {
     -re "^$command\r\n$gdb_prompt " {
 	# Note no end anchor, because "si&" finishes and prints the
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
index 62415e7..2a12ad6 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
@@ -47,6 +47,5 @@ gdb_test "si" "Breakpoint .* bar break .*"
 
 # If the breakpoint is still correctly inserted, then this jump should
 # re-trigger it.  Otherwise, GDB will lose control and the program
-# will exit.
-setup_kfail "breakpoints/17000" "*-*-*"
+# will exit.  See PR breakpoints/17000.
 gdb_test "jump *\$pc" "Breakpoint .* bar break .*"
-- 
1.9.0

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

Hi Pedro,

> > Bah, I woke up realizing that the version I posted forgets to
> > clone the shadow buffer!  Let me fix that and repost...

You are producing patches so fast, I am wondering if I will be able
to keep up! :-)

> gdb/ChangeLog:
> 
> 	PR breakpoints/17000
> 	* breakpoint.c (find_non_raw_software_breakpoint_inserted_here):
> 	New function, extracted from software_breakpoint_inserted_here_p.
> 	(software_breakpoint_inserted_here_p): Replace factored out code
> 	by call to find_non_raw_software_breakpoint_inserted_here.
> 	(bp_target_info_copy_insertion_state): New function.
> 	(bkpt_insert_location): Handle the case of a single-step
> 	breakpoint already inserted at the same address.
> 	(bkpt_remove_location): Handle the case of a single-step
> 	breakpoint still inserted at the same address.
> 	(deprecated_insert_raw_breakpoint): Handle the case of non-raw
> 	breakpoint already inserted at the same address.
> 	(deprecated_remove_raw_breakpoint): Handle the case of a
> 	non-raw breakpoint still inserted at the same address.
> 	(find_single_step_breakpoint): New function, extracted from
> 	single_step_breakpoint_inserted_here_p.
> 	(find_single_step_breakpoint): New function,
> 	factored out from single_step_breakpoint_inserted_here_p.
> 	(single_step_breakpoint_inserted_here_p): Reimplement.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR breakpoints/17000
> 	* gdb.base/sss-bp-on-user-bp.exp: Remove kfail.
> 	* gdb.base/sss-bp-on-user-bp-2.exp: Remove kfail.

You are making it us realize that the problem is more and more
complex than we thought! :-(. And I think we'll need a small
adjustment to your patch in order to account for something that
may have been missed. See below:

> @@ -15138,12 +15196,30 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
>  				  struct address_space *aspace, CORE_ADDR pc)
>  {
>    struct bp_target_info *bp_tgt;
> +  struct bp_location *bl;
>  
>    bp_tgt = XCNEW (struct bp_target_info);
>  
>    bp_tgt->placed_address_space = aspace;
>    bp_tgt->placed_address = pc;
>  
> +  /* If an unconditional non-raw breakpoint is already inserted at
> +     that location, there's no need to insert another.  However, with
> +     target-side evaluation of breakpoint conditions, if the
> +     breakpoint that is currently inserted on the target is
> +     conditional, we need to make it unconditional.  Note that a
> +     breakpoint with target-side commands is not reported even if
> +     unconditional, so we need to remove the commands from the target
> +     as well.  */
> +  bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
> +  if (bl != NULL
> +      && VEC_empty (agent_expr_p, bl->target_info.conditions)
> +      && VEC_empty (agent_expr_p, bl->target_info.tcommands))
> +    {
> +      bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
> +      return bp_tgt;
> +    }
> +

ISTM that you are assuming that there would only be one other breakpoint
inserted at this location. What if there were more?

If I am right, I suggest the addition of an extra parameter to
find_non_raw_software_breakpoint_inserted_here which would be
a pointer to a filtering function. If NULL, no filtering is done,
but if not NULL, the filter function must accept the bp_location
for find_non_raw_software_breakpoint_inserted_here to return it.

>  deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
>  {
>    struct bp_target_info *bp_tgt = bp;
> +  struct address_space *aspace = bp_tgt->placed_address_space;
> +  CORE_ADDR address = bp_tgt->placed_address;
> +  struct bp_location *bl;
>    int ret;
>  
> -  ret = target_remove_breakpoint (gdbarch, bp_tgt);
> +  bl = find_non_raw_software_breakpoint_inserted_here (aspace, address);
> +
> +  /* Only remove the raw breakpoint if there are no other non-raw
> +     breakpoints still inserted at this location.  Otherwise, we would
> +     be effectively disabling those breakpoints.  */
> +  if (bl == NULL)
> +    ret = target_remove_breakpoint (gdbarch, bp_tgt);
> +  else if (!VEC_empty (agent_expr_p, bl->target_info.conditions)
> +	   || !VEC_empty (agent_expr_p, bl->target_info.tcommands))
> +    {
> +      /* The target is evaluating conditions, and when we inserted the
> +	 software single-step breakpoint, we had made the breakpoint
> +	 unconditional and command-less on the target side.  Reinsert
> +	 to restore the conditions/commands.  */
> +      ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info);
> +    }
> +  else
> +    ret = 0;

Same here, I think.


-- 
Joel

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/03/2014 02:35 PM, Joel Brobecker wrote:
> Hi Pedro,
> 
>>> Bah, I woke up realizing that the version I posted forgets to
>>> clone the shadow buffer!  Let me fix that and repost...
> 
> You are producing patches so fast, I am wondering if I will be able
> to keep up! :-)

:-)

>> @@ -15138,12 +15196,30 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
>>  				  struct address_space *aspace, CORE_ADDR pc)
>>  {
>>    struct bp_target_info *bp_tgt;
>> +  struct bp_location *bl;
>>  
>>    bp_tgt = XCNEW (struct bp_target_info);
>>  
>>    bp_tgt->placed_address_space = aspace;
>>    bp_tgt->placed_address = pc;
>>  
>> +  /* If an unconditional non-raw breakpoint is already inserted at
>> +     that location, there's no need to insert another.  However, with
>> +     target-side evaluation of breakpoint conditions, if the
>> +     breakpoint that is currently inserted on the target is
>> +     conditional, we need to make it unconditional.  Note that a
>> +     breakpoint with target-side commands is not reported even if
>> +     unconditional, so we need to remove the commands from the target
>> +     as well.  */
>> +  bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
>> +  if (bl != NULL
>> +      && VEC_empty (agent_expr_p, bl->target_info.conditions)
>> +      && VEC_empty (agent_expr_p, bl->target_info.tcommands))
>> +    {
>> +      bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
>> +      return bp_tgt;
>> +    }
>> +
> 
> ISTM that you are assuming that there would only be one other breakpoint
> inserted at this location. What if there were more?

Yep, it's a valid assumption.  Only one of those can be the one that
is actually inserted in the target.  All others breakpoints are considered
duplicates, with bl->duplicate == 1 and bl->inserted == 0, and never reach
the target.  The duplicate location logic in the tail of update_global_location_list
takes care of it:

      /* This and the above ensure the invariant that the first location
	 is not duplicated, and is the inserted one.
	 All following are marked as duplicated, and are not inserted.  */
      if (loc->inserted)
	swap_insertion (loc, *loc_first_p);
      loc->duplicate = 1;

The one that is inserted will hold a merge of all the agent
expressions (in target_info.conditions and target_info.tcommands) of
the target-side conditions and commands of all breakpoints at that
address.  Those are computed just before that single breakpoint
is inserted (build_target_condition_list, build_target_command_list).

-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

> Yep, it's a valid assumption.  Only one of those can be the one that
> is actually inserted in the target.  All others breakpoints are considered
> duplicates, with bl->duplicate == 1 and bl->inserted == 0, and never reach
> the target.  The duplicate location logic in the tail of update_global_location_list
> takes care of it:
> 
>       /* This and the above ensure the invariant that the first location
> 	 is not duplicated, and is the inserted one.
> 	 All following are marked as duplicated, and are not inserted.  */
>       if (loc->inserted)
> 	swap_insertion (loc, *loc_first_p);
>       loc->duplicate = 1;
> 
> The one that is inserted will hold a merge of all the agent
> expressions (in target_info.conditions and target_info.tcommands) of
> the target-side conditions and commands of all breakpoints at that
> address.  Those are computed just before that single breakpoint
> is inserted (build_target_condition_list, build_target_command_list).

Indeed! Thanks for explaining it.

I don't have any other comment on the patch. I ran it through our
testsuite on ppc-aix, JIC, and as expected, found that it fixed
the regression without introducing new ones. I think you can push
that version!

Thanks for your help, Pedro.
-- 
Joel

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/03/2014 05:23 PM, Joel Brobecker wrote:

> I don't have any other comment on the patch. I ran it through our
> testsuite on ppc-aix, JIC, and as expected, found that it fixed
> the regression without introducing new ones. I think you can push
> that version!

Awesome, thank you!  Now pushed.

-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Joel Brobecker]

> Awesome, thank you!  Now pushed.

Yeepee! I see that you marked the PR as fixed. I moved the AI
in the wiki to Done.

Thanks!
-- 
Joel

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Yao Qi]

On 06/03/2014 08:19 PM, Pedro Alves wrote:
> On 06/03/2014 01:12 PM, Andreas Schwab wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>
>>> Ah, thanks.  We need to replace then with asm("nop") then.
>>
>> nop isn't portable.
> 
> Yes, but it doesn't matter what the instruction is as
> long as it's a single instruction that doesn't do much.
> For archs that don't have "nop" (like e.g., IA64), we can
> just use #ifdef to pick another insn.
> 

Pedro, here is the patch to tweak sss-bp-on-user-bp.exp.
I give up on looking for a portable "nop" for various arch.  In
stead, we can use disassemble to get the next instruction address
and set breakpoint there.  See details in the commit log below.

-- 
Yao (齐尧)

Subject: [PATCH] Tweak sss-bp-on-user-bp.exp

sss-bp-on-user-bp.c has an assumption that write to integer can be
compiled to a single instruction, which isn't true on some arch, such
as arm.  This test requires setting two breakpoints on two consecutive
instructions, so this patch is to get the address of the next
instruction via disassemble and set the 2nd breakpoint there.  This
approach is portable.

This patch fixes the fails in sss-bp-on-user-bp.exp on arm-none-abi
target.  There is no change in x86 test results.  I also revert the
patch to PR breakpoints/17000, and verified that the patched
sss-bp-on-user-bp.exp still triggers the fail on
x86-with-software-single-step.

gdb/testsuite:

2014-06-04  Yao Qi  <yao@codesourcery.com>

	* gdb.base/sss-bp-on-user-bp.c (main): Remove comments.
	* gdb.base/sss-bp-on-user-bp.exp: Don't set breakpoint on
	"set bar break here".  Get the next instruction address and
	set breakpoint there.  Remove "bar break" from the regexp
	patterns.
---
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.c   |  4 ++--
 gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp | 20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
index ff82051..edc2e8c 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.c
@@ -21,10 +21,10 @@
 int
 main (void)
 {
-  /* Assume writes to integers compile to a single instruction.  */
   volatile int i = 0;
 
   i = 1;     /* set foo break here */
-  i = 2;     /* set bar break here */
+  i = 2;
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
index 2a12ad6..0b39fc1 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp
@@ -32,7 +32,21 @@ if ![runto_main] then {
 gdb_breakpoint [gdb_get_line_number "set foo break here"]
 gdb_continue_to_breakpoint "first breakpoint" ".* set foo break here .*"
 
-gdb_breakpoint [gdb_get_line_number "set bar break here"]
+# Get the address of the next instruction and set a breakpoint there.
+set next_insn_addr ""
+set test "disassemble main"
+gdb_test_multiple $test $test {
+    -re ".*=> $hex <\\+$decimal>:\[^\r\n\]+\r\n   ($hex) .*$gdb_prompt $" {
+	set next_insn_addr $expect_out(1,string)
+	pass $test
+    }
+}
+
+if { $next_insn_addr == "" } {
+    return -1
+}
+
+gdb_test "b *$next_insn_addr" "Breakpoint .*"
 
 # So that GDB doesn't try to remove the regular breakpoint when the
 # step finishes.
@@ -43,9 +57,9 @@ gdb_test_no_output "set breakpoint always-inserted on"
 # remove it.  But, a regular breakpoint is planted there already, and
 # with always-inserted on, should remain planted when the step
 # finishes.
-gdb_test "si" "Breakpoint .* bar break .*"
+gdb_test "si" "Breakpoint .*"
 
 # If the breakpoint is still correctly inserted, then this jump should
 # re-trigger it.  Otherwise, GDB will lose control and the program
 # will exit.  See PR breakpoints/17000.
-gdb_test "jump *\$pc" "Breakpoint .* bar break .*"
+gdb_test "jump *\$pc" "Breakpoint .*"
-- 
1.9.0

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Pedro Alves]

On 06/04/2014 06:12 AM, Yao Qi wrote:
> On 06/03/2014 08:19 PM, Pedro Alves wrote:
>> On 06/03/2014 01:12 PM, Andreas Schwab wrote:
>>> Pedro Alves <palves@redhat.com> writes:
>>>
>>>> Ah, thanks.  We need to replace then with asm("nop") then.
>>>
>>> nop isn't portable.
>>
>> Yes, but it doesn't matter what the instruction is as
>> long as it's a single instruction that doesn't do much.
>> For archs that don't have "nop" (like e.g., IA64), we can
>> just use #ifdef to pick another insn.
>>
> 
> Pedro, here is the patch to tweak sss-bp-on-user-bp.exp.
> I give up on looking for a portable "nop" for various arch.  In
> stead, we can use disassemble to get the next instruction address
> and set breakpoint there.  See details in the commit log below.

Good idea.  Patch is OK.

Thanks!

-- 
Pedro Alves

Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

[Yao Qi]

On 06/04/2014 04:01 PM, Pedro Alves wrote:
> Good idea.  Patch is OK.

Patch is pushed in, thanks for the review.

-- 
Yao (齐尧)

[pushed] sss-bp-on-user-bp-2.exp sometimes fails on native GNU/Linux. (was: [pushed] PR breakpoints/17000: user breakpoint not inserted if software-single-step at same location - another test)

[Pedro Alves]

On 06/03/2014 02:08 PM, Pedro Alves wrote:
> On 06/03/2014 12:53 PM, Pedro Alves wrote:
> 
>> (gdb) PASS: gdb.base/sss-bp-on-user-bp-2.exp: define stepi_del_break
>> stepi_del_break
>> Cannot execute this command while the target is running.
>> (gdb) FAIL: gdb.base/sss-bp-on-user-bp-2.exp: stepi_del_break
>>
>> The error is because GDB tried to remove the breakpoint from
>> memory while the thread was running, and we can't talk to the
>> server in the all-stop RSP until we get a stop reply, but in any
>> case, GDB shouldn't even be attempting to remove the breakpoint,
>> exactly because there was a sss breakpoint at the same address.
>>
>> I've added a kfail, and pushed it.
> 
> Gosh, so many different mode combinations...  So the fix for
> PR17000 doesn't fix that failure when testing against gdbserver
> _and_ hardware stepping.  Well, of course it doesn't, because in
> that case GDB will really try to remove the breakpoint, because
> there's no overlapping software single-step breakpoint.
> 

and ... 

> +# With the all-stop RSP, we can't talk to the target while it's
> +# running, until we get back the stop reply.  If not using single-step
> +# breakpoints, then the "del" in stepi_del_break below will try to
> +# delete the user breakpoint from the target, which will fail, with
> +# "Cannot execute this command while the target is running.".  On
> +# software single-step targets, that del shouldn't trigger any RSP
> +# traffic.  Just skip the test if testing against a remove target and
> +# not using software single-stepping.  IOW, skip the test if we see a
> +# 'vCont;s' or 's' in the RSP traffic.

... this is still not sufficient...

I pushed the commit below to skip the test on native targets too.

I was surprised the failure to remove the breakpoint mentioned
below didn't cause a warning.  That'll be the subject of a
separate patch.

8<-----------
From 829155c9adb5f3750c9c612702fdbf26fa7c558e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 6 Jun 2014 19:59:21 +0100
Subject: [PATCH] sss-bp-on-user-bp-2.exp sometimes fails on native GNU/Linux.

I noticed that sss-bp-on-user-bp-2.exp is racy on native GNU/Linux.  I
sometimes still see an int3 in the disassembly:

 (gdb) PASS: gdb.base/sss-bp-on-user-bp-2.exp: set debug target 0
 disassemble test
 Dump of assembler code for function test:
    0x0000000000400590 <+0>:     push   %rbp
    0x0000000000400591 <+1>:     mov    %rsp,%rbp
    0x0000000000400594 <+4>:     nop
 => 0x0000000000400595 <+5>:     int3
    0x0000000000400596 <+6>:     pop    %rbp
    0x0000000000400597 <+7>:     retq
 End of assembler dump.
 (gdb) FAIL: gdb.base/sss-bp-on-user-bp-2.exp: before/after disassembly matches

Enabling infrun/target debug logs, we can see the problem.
Simplified, that's:

 (gdb) PASS: gdb.base/sss-bp-on-user-bp-2.exp: define stepi_del_break
 stepi_del_break
 infrun: clear_proceed_status_thread (process 25311)
 infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 25311] at 0x400594
 LLR: PTRACE_SINGLESTEP process 25311, 0 (resume event thread)
 target_resume (25311, step, 0)
 native:target_xfer_partial (3, (null), 0x0, 0x32dce4c, 0x400595, 1) = 0, 0
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 (gdb) linux_nat_wait: [process -1], [TARGET_WNOHANG]

0x400595 is the address of the breakpoint, and "= 0" is
TARGET_XFER_EOF.  That's default_memory_remove_breakpoint trying to
remove the breakpoint, but failing.

The problem is that we had just resumed the target and the native
GNU/Linux target can't read memory off of a running thread.  Most of
the time, we get "lucky", because we manage to read memory before the
kernel actually schedules the target to run.

So just give up and skip the test on any target that uses hardware
stepping, not just remote targets.

gdb/testsuite/
2014-06-06  Pedro Alves  <palves@redhat.com>

	* gdb.base/sss-bp-on-user-bp-2.exp: Look for target_resume(step)
	in target debug output instead of looking at RSP packets,
	disabling the test on any target that uses hardware stepping.
	Update comments.
---
 gdb/testsuite/ChangeLog                        |  7 ++++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp | 31 ++++++++++++--------------
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 933df0a..327e648 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,12 @@
 2014-06-06  Pedro Alves  <palves@redhat.com>
 
+	* gdb.base/sss-bp-on-user-bp-2.exp: Look for target_resume(step)
+	in target debug output instead of looking at RSP packets,
+	disabling the test on any target that uses hardware stepping.
+	Update comments.
+
+2014-06-06  Pedro Alves  <palves@redhat.com>
+
 	* gdb.base/break-unload-file.exp: Fix typo.
 
 2014-06-06  Yao Qi  <yao@codesourcery.com>
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index 87fa5f8..a196f68 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -50,23 +50,20 @@ delete_breakpoints
 # delete the user breakpoint from the target, which will fail, with
 # "Cannot execute this command while the target is running.".  On
 # software single-step targets, that del shouldn't trigger any RSP
-# traffic.  Just skip the test if testing against a remove target and
-# not using software single-stepping.  IOW, skip the test if we see a
-# 'vCont;s' or 's' in the RSP traffic.
-
-gdb_test_no_output "set debug remote 1"
-
-set rsp_hardware_step 0
+# traffic.  Hardware-step targets that can't access memory while the
+# target is running, either remote or native, are likewise affected.
+# So we just skip the test if not using software single-stepping.  We
+# detect that by looking for 'target_resume (..., step)' in "debug
+# target" output.
 
 # Probe for software single-step breakpoint use.
-set test "probe RSP hardware step"
+
+gdb_test_no_output "set debug target 1"
+set hardware_step 0
+set test "probe target hardware step"
 gdb_test_multiple "si" $test {
-    -re "\\\$vCont;s.*$gdb_prompt $" {
-	set rsp_hardware_step 1
-	pass $test
-    }
-    -re "\\\$s#.*$gdb_prompt $" {
-	set rsp_hardware_step 1
+    -re "target_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
+	set hardware_step 1
 	pass $test
     }
     -re "$gdb_prompt $" {
@@ -74,12 +71,12 @@ gdb_test_multiple "si" $test {
     }
 }
 
-if { $rsp_hardware_step } {
-    unsupported "remote target doesn't use software single-stepping"
+if { $hardware_step } {
+    unsupported "target doesn't use software single-stepping"
     return
 }
 
-gdb_test_no_output "set debug remote 0"
+gdb_test_no_output "set debug target 0"
 
 set line_re "\[^\r\n\]*"
 
-- 
1.9.0

Re: [pushed] sss-bp-on-user-bp-2.exp sometimes fails on native GNU/Linux.

[Pedro Alves]

On 06/06/2014 08:05 PM, Pedro Alves wrote:

> Enabling infrun/target debug logs, we can see the problem.
> Simplified, that's:
> 
>  (gdb) PASS: gdb.base/sss-bp-on-user-bp-2.exp: define stepi_del_break
>  stepi_del_break
>  infrun: clear_proceed_status_thread (process 25311)
>  infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 25311] at 0x400594
>  LLR: PTRACE_SINGLESTEP process 25311, 0 (resume event thread)
>  target_resume (25311, step, 0)
>  native:target_xfer_partial (3, (null), 0x0, 0x32dce4c, 0x400595, 1) = 0, 0
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  (gdb) linux_nat_wait: [process -1], [TARGET_WNOHANG]
> 
> 0x400595 is the address of the breakpoint, and "= 0" is
> TARGET_XFER_EOF.  That's default_memory_remove_breakpoint trying to
> remove the breakpoint, but failing.

I was quite surprised this didn't result in a user visible warning.
Turns out that's a regression compared to 7.7.  This fixes it:

 https://sourceware.org/ml/gdb-patches/2014-06/msg00377.html

-- 
Pedro Alves