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

[Joel Brobecker <brobecker at adacore dot com>]

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