[PATCH] breakpoint remove fail handle bug fix

[Hui Zhu <hui_zhu@mentor.com>]

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

Hi,

I sent this patch with the autoload-breakpoints patches because I found 
this issue when I test the autoload-breakpoints.  But I think sent it 
together with a feature patch list is not a good idea.  So I move it out 
for review.

This is bug was found when I when I test autoload-breakpint code.  And I 
found that it affect target-async too.
It can be reproduce:
(gdb) set target-async on
(gdb) start
Temporary breakpoint 1 at 0x4004c8: file 1.c, line 4.
Starting program: /home/teawater/tmp/a.out

Temporary breakpoint 1, main () at 1.c:4
4		sleep (20);
(gdb) disassemble
Dump of assembler code for function main:
    0x00000000004004c4 <+0>:	push   %rbp
    0x00000000004004c5 <+1>:	mov    %rsp,%rbp
=> 0x00000000004004c8 <+4>:	mov    $0x14,%edi
    0x00000000004004cd <+9>:	mov    $0x0,%eax
    0x00000000004004d2 <+14>:	callq  0x4003d0 <sleep@plt>
    0x00000000004004d7 <+19>:	mov    $0x0,%eax
    0x00000000004004dc <+24>:	pop    %rbp
    0x00000000004004dd <+25>:	retq
End of assembler dump.
(gdb) list
1	int
2	main()
3	{
4		sleep (20);
5	
6		return 0;
7	}
8	
(gdb) b 6
Breakpoint 2 at 0x4004d7: file 1.c, line 6.
(gdb) c&
Continuing.
(gdb) d
Delete all breakpoints? (y or n) y
warning: Error removing breakpoint 2
(gdb)
Program received signal SIGTRAP, Trace/breakpoint trap.
0x00000000004004d8 in main () at 1.c:6
6		return 0;
c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x00000000004004d8 in main () at 1.c:6
6		return 0;
(gdb) info reg pc
pc: 0x4004d8
(gdb) disassemble main
Dump of assembler code for function main:
    0x00000000004004c4 <+0>:	push   %rbp
    0x00000000004004c5 <+1>:	mov    %rsp,%rbp
    0x00000000004004c8 <+4>:	mov    $0x14,%edi
    0x00000000004004cd <+9>:	mov    $0x0,%eax
    0x00000000004004d2 <+14>:	callq  0x4003d0 <sleep@plt>
    0x00000000004004d7 <+19>:	int3
=> 0x00000000004004d8 <+20>:	add    %al,(%rax)
    0x00000000004004da <+22>:	add    %al,(%rax)
    0x00000000004004dc <+24>:	pop    %rbp
    0x00000000004004dd <+25>:	retq
End of assembler dump.

This because is when GDB got fail when it remove the breakpoint, it give 
up the control of this breakpoint.
There are 2 issues about it:
1. When the GDB stop, this breakpoint is not be removed.
2. If inferior is stoped by this breakpoint, adjust_pc_after_break 
didn't know this stop is beauce the breakpoint, so the pc address will 
not be adjust to the right value.

I add a list called bp_location_remove_fail_chain, when GDB got fail 
with remove a breakpoint, add it to this list.  When 
adjust_pc_after_break check if this address is the breakpint, check this 
list too.  And when gdb remve all breakpoints, try remove breakpint in 
this list.

Thanks,
Hui

2012-04-11  Hui Zhu  <hui_zhu@mentor.com>

	* breakpoint.c (ALL_BP_LOCATION_REMOV_FAIL): New macro.
	(ALL_BP_LOCATION_REMOV_FAIL_SAFE): New macro.
	(bp_location_remove_fail_chain_inserted_here_p): New function.
	(bp_location_remove_fail_chain_insert): New function.
	(bp_location_remove_fail_chain_remove): New function.
	(remove_breakpoints): Call remove_breakpoint with the bp_locations
	inside the ALL_BP_LOCATION_REMOV_FAIL_SAFE.
	(software_breakpoint_inserted_here_p): Call
	bp_location_remove_fail_chain_inserted_here_p.
	(update_global_location_list): Call
	bp_location_remove_fail_chain_insert.
	* breakpoint.h (bp_location_remove_fail_chain_remove): New extern.
	* target.c (target_kill): Call bp_location_remove_fail_chain_remove.
	(target_detach): Ditto.
	(target_disconnect): Ditto.

[-- Attachment #2: break-remove-error-change.txt --]
[-- Type: text/plain, Size: 5910 bytes --]

---
 breakpoint.c |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 breakpoint.h |    2 
 target.c     |   15 +++++-
 3 files changed, 139 insertions(+), 10 deletions(-)

--- a/breakpoint.c
+++ b/breakpoint.c
@@ -569,6 +569,100 @@ static struct cmd_list_element *breakpoi
 static struct cmd_list_element *breakpoint_show_cmdlist;
 struct cmd_list_element *save_cmdlist;
 
+/* Chains of bp_location that have remove issue. */
+static struct bp_location *bp_location_remove_fail_chain = NULL;
+
+#define ALL_BP_LOCATION_REMOV_FAIL(BP)			\
+	for (BP = bp_location_remove_fail_chain;	\
+	     BP;					\
+	     BP = BP->next)
+
+#define ALL_BP_LOCATION_REMOV_FAIL_SAFE(BP,TMP)		\
+	for (BP = bp_location_remove_fail_chain;	\
+	     BP ? (TMP=BP->next, 1): 0;			\
+	     BP = TMP)
+
+static int
+bp_location_remove_fail_chain_inserted_here_p (struct address_space *aspace,
+					       CORE_ADDR pc)
+{
+  struct bp_location *bl;
+
+  ALL_BP_LOCATION_REMOV_FAIL (bl)
+    {
+      if (bl->loc_type != bp_loc_software_breakpoint)
+	continue;
+
+      if (bl->inserted
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
+				       aspace, pc))
+	{
+	  if (overlay_debugging
+	      && section_is_overlay (bl->section)
+	      && !section_is_mapped (bl->section))
+	    continue;		/* unmapped overlay -- can't be a match */
+	  else
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
+static void
+bp_location_remove_fail_chain_insert (struct bp_location *bl)
+{
+  struct bp_location *bltmp;
+  struct breakpoint *b;
+
+  if (!bl->owner)
+    return;
+
+  ALL_BP_LOCATION_REMOV_FAIL (bltmp)
+    {
+      if (bltmp == bl)
+	return;
+    }
+
+  b = (struct breakpoint *) xzalloc (sizeof (struct breakpoint));
+  *b = *bl->owner;
+  bl->owner = b;
+
+  incref_bp_location (bl);
+  bl->next = bp_location_remove_fail_chain;
+  bp_location_remove_fail_chain = bl;
+}
+
+void
+bp_location_remove_fail_chain_remove (struct bp_location *bl)
+{
+  struct bp_location *bltmp, *bltmp2, *blprev = NULL;
+
+  ALL_BP_LOCATION_REMOV_FAIL_SAFE (bltmp, bltmp2)
+    {
+      if (!bl || bltmp == bl)
+	{
+	  if (bl)
+	    {
+	      if (blprev)
+		blprev->next = bl->next;
+	      else
+		bp_location_remove_fail_chain = bl->next;
+	    }
+	  xfree (bltmp->owner);
+	  bltmp->owner = NULL;
+	  decref_bp_location (&bltmp);
+	  if (bl)
+	    break;
+	}
+      else
+	blprev = bltmp;
+    }
+
+  if (!bl)
+    bp_location_remove_fail_chain = NULL;
+}
+
 /* Return whether a breakpoint is an active enabled breakpoint.  */
 static int
 breakpoint_enabled (struct breakpoint *b)
@@ -2594,7 +2688,7 @@ You may have requested too many hardware
 int
 remove_breakpoints (void)
 {
-  struct bp_location *bl, **blp_tmp;
+  struct bp_location *bl, *bltmp, **blp_tmp;
   int val = 0;
 
   ALL_BP_LOCATIONS (bl, blp_tmp)
@@ -2602,6 +2696,20 @@ remove_breakpoints (void)
     if (bl->inserted && !is_tracepoint (bl->owner))
       val |= remove_breakpoint (bl, mark_uninserted);
   }
+
+  ALL_BP_LOCATION_REMOV_FAIL_SAFE (bl, bltmp)
+    {
+      volatile struct gdb_exception e;
+      int ret;
+
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  ret = remove_breakpoint (bl, mark_uninserted);
+	}
+      if (!ret && e.reason >= 0)
+	bp_location_remove_fail_chain_remove (bl);
+    }
+
   return val;
 }
 
@@ -3536,6 +3644,9 @@ software_breakpoint_inserted_here_p (str
   if (single_step_breakpoint_inserted_here_p (aspace, pc))
     return 1;
 
+  if (bp_location_remove_fail_chain_inserted_here_p (aspace, pc))
+    return 1;
+
   return 0;
 }
 
@@ -11650,7 +11761,14 @@ update_global_location_list (int should_
 
 	  if (!keep_in_target)
 	    {
-	      if (remove_breakpoint (old_loc, mark_uninserted))
+	      volatile struct gdb_exception e;
+	      int ret;
+
+	      TRY_CATCH (e, RETURN_MASK_ERROR)
+		{
+		  ret = remove_breakpoint (old_loc, mark_uninserted);
+		}
+	      if (ret || e.reason < 0)
 		{
 		  /* This is just about all we can do.  We could keep
 		     this location on the global list, and try to
@@ -11663,8 +11781,11 @@ update_global_location_list (int should_
 		  printf_filtered (_("warning: Error removing "
 				     "breakpoint %d\n"), 
 				   old_loc->owner->number);
+		  removed = 0;
+		  bp_location_remove_fail_chain_insert (old_loc);
 		}
-	      removed = 1;
+	      else
+		removed = 1;
 	    }
 	}
 
@@ -11725,10 +11846,7 @@ update_global_location_list (int should_
 	      VEC_safe_push (bp_location_p, moribund_locations, old_loc);
 	    }
 	  else
-	    {
-	      old_loc->owner = NULL;
-	      decref_bp_location (&old_loc);
-	    }
+	    decref_bp_location (&old_loc);
 	}
     }
 
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -1480,4 +1480,6 @@ extern struct gdbarch *get_sal_arch (str
 
 extern void handle_solib_event (void);
 
+extern void bp_location_remove_fail_chain_remove (struct bp_location *bl);
+
 #endif /* !defined (BREAKPOINT_H) */
--- a/target.c
+++ b/target.c
@@ -454,6 +454,9 @@ target_kill (void)
 	  fprintf_unfiltered (gdb_stdlog, "target_kill ()\n");
 
         t->to_kill (t);
+
+	bp_location_remove_fail_chain_remove (NULL);
+
 	return;
       }
 
@@ -2568,9 +2571,13 @@ target_detach (char *args, int from_tty)
        disconnection from the target.  */
     ;
   else
-    /* If we're in breakpoints-always-inserted mode, have to remove
-       them before detaching.  */
-    remove_breakpoints_pid (PIDGET (inferior_ptid));
+    {
+      /* If we're in breakpoints-always-inserted mode, have to remove
+	 them before detaching.  */
+      remove_breakpoints_pid (PIDGET (inferior_ptid));
+
+      bp_location_remove_fail_chain_remove (NULL);
+    }
 
   prepare_for_detach ();
 
@@ -2599,6 +2606,8 @@ target_disconnect (char *args, int from_
      disconnecting.  */
   remove_breakpoints ();
 
+  bp_location_remove_fail_chain_remove (NULL);
+
   for (t = current_target.beneath; t != NULL; t = t->beneath)
     if (t->to_disconnect != NULL)
 	{