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

[Joel Brobecker <brobecker at adacore dot com>]

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

>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