Re: [RFA] Keep breakpoints always inserted.

[Pedro Alves <pedro at codesourcery dot com>]

> On Thu, Apr 10, 2008 at 02:27:00AM +0400, Vladimir Prus wrote:
> > I attach the revised patch, together with the delta relative to the
> > previous one. This has no regressions, neither in always-inserted nor
> > default mode. OK?
>

Testing with this patch applied and with displaced stepping on top
revealed one problem.

One should be able to call insert_breakpoints several times
in succession and the effect should be as if only one call was made.

There is a bug in the patch where if there is more than
one breakpoint at the same address, the second insert_breakpoints
call will remove the breakpoint location from the target.

This happens because should_insert_location returns false if the
location is already inserted.  

 /* Returns 1 iff breakpoint location should be
    inserted in the inferior.  */
 static int
 should_insert_location (struct bp_location *bpt)
 {
   if (!breakpoint_enabled (bpt->owner))
     return 0;

   if (bpt->owner->disposition == disp_del_at_next_stop)
     return 0;

   if (!bpt->enabled || bpt->shlib_disabled || bpt->inserted ||  
bpt->duplicate)
     return 0;

   return 1;
 }

On the case were we have two locations at the same address, when we
get to the loop the patch is touching looking for another location
at the same address, we'll fail to mark this location as to be kept,
and go on to remove the breakpoint from the target,

	  if (!keep)
	    if (remove_breakpoint (loc, mark_uninserted))
	      {

The attached patch inlines the tests we're really interested in
at this point (I'm not sure about the disposition test), and removes
the need for the (incomplete) hack of setting loc2->duplicate = 0
before calling should_insert_location.

This failed with the displaced stepping patch on top,
because with that patch there are now several code paths
that call insert_breakpoints without a remove_breakpoints
call in between.  You can also reproduce this easilly
by applying something like this:


---
 gdb/infrun.c |    2 ++
 1 file changed, 2 insertions(+)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c       2008-04-23 18:38:33.000000000 +0100
+++ src/gdb/infrun.c    2008-04-23 18:38:47.000000000 +0100
@@ -2921,6 +2921,8 @@ keep_going (struct execution_control_sta
          TRY_CATCH (e, RETURN_MASK_ERROR)
            {
              insert_breakpoints ();
+             insert_breakpoints ();
+             insert_breakpoints ();
            }
          if (e.reason < 0)
            {

The test case that made this visible was thread-specific.exp,
which inserts more than one breakpoint at the same location
(although marked for different threads).

The symptom is that the inferior will not stop at the
breakpoint.
Easilly tested in any test, by "b main; b main; run".

I've tested this patch on top of Vladimir's and with displaced
stepping on, on x86-pc-linux-gnu.  Luis tested it on PPC, where
it also fixed things.

P.S.:
Anyone object to adding debug output support to the
breakpoint module?  I've writen something similar twice
already, and I'd like to not have to a third time.  Something
like "set debug breakpoint 1", where it prints
"breakpoints inserted", "breakpoints removed", etc.

-- 
Pedro Alves

---
 gdb/breakpoint.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2008-04-23 18:26:53.000000000 +0100
+++ src/gdb/breakpoint.c	2008-04-23 18:28:43.000000000 +0100
@@ -7019,12 +7019,12 @@ update_global_location_list (void)
 	      if (breakpoint_address_is_meaningful (loc->owner))
 		for (loc2 = bp_location_chain; loc2; loc2 = loc2->global_next)
 		  {
-		    /* For the sake of should_insert_location.  The
-		       call to check_duplicates will fix up this later.  */
-		    loc2->duplicate = 0;
-		    if (should_insert_location (loc2)
-			&& loc2 != loc && loc2->address == loc->address)
-		      {		  
+		    if (loc2 != loc
+			&& loc2->address == loc->address
+			&& breakpoint_enabled (loc2->owner)
+			&& loc2->enabled && !loc2->shlib_disabled
+			&& loc2->owner->disposition != disp_del_at_next_stop)
+		      {
 			loc2->inserted = 1;
 			loc2->target_info = loc->target_info;
 			keep = 1;