[PATCH 2/3] make "permanent breakpoints" per location and disableable

[Pedro Alves <palves at redhat dot com>]

"permanent"-ness is currently a property of the breakpoint.  But, it
should actually be an implementation detail of a _location_.  Consider
this bit in infrun.c:

  /* Normally, by the time we reach `resume', the breakpoints are either
     removed or inserted, as appropriate.  The exception is if we're sitting
     at a permanent breakpoint; we need to step over it, but permanent
     breakpoints can't be removed.  So we have to test for it here.  */
  if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here)
    {
      if (gdbarch_skip_permanent_breakpoint_p (gdbarch))
	gdbarch_skip_permanent_breakpoint (gdbarch, regcache);
      else
	error (_("\
The program is stopped at a permanent breakpoint, but GDB does not know\n\
how to step past a permanent breakpoint on this architecture.  Try using\n\
a command like `return' or `jump' to continue execution."));
    }

This will wrongly skip a non-breakpoint instruction if we have a
multiple location breakpoint where the whole breakpoint was set to
"permanent" because one of the locations happened to be permanent,
even if the one GDB is resuming from is not.

Related, because the permanent breakpoints are only marked as such in
init_breakpoint_sal, we currently miss marking momentary breakpoints
as permanent.  A test added by a following patch trips on that.
Making permanent-ness be per-location, and marking locations as such
in add_location_to_breakpoint, the natural place to do this, fixes
this issue...

... and then exposes a latent issue with mark_breakpoints_out.  It's
clearing the inserted flag of permanent breakpoints.  This results in
assertions failing like this:

 Breakpoint 1, main () at testsuite/gdb.base/callexit.c:32
 32        return 0;
 (gdb) call callexit()
 [Inferior 1 (process 15849) exited normally]
 gdb/breakpoint.c:12854: internal-error: allegedly permanent breakpoint is not actually inserted
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.

The call dummy breakpoint, which is a momentary breakpoint, is set on
top of a manually inserted breakpoint instruction, and so is now
rightfully marked as a permanent breakpoint.  See "Write a legitimate
instruction at the point where the infcall breakpoint is going to be
inserted." comment in infcall.c.

Re. make_breakpoint_permanent.  That's only called by solib-pa64.c.
Permanent breakpoints were actually originally invented for HP-UX [1].
I believe that that call (the only one in the tree) is unnecessary
nowadays, given that nowadays the core breakpoints code analyzes the
instruction under the breakpoint to automatically detect whether it's
setting a breakpoint on top of a breakpoint instruction in the
program.  I know close to nothing about HP-PA/HP-UX, though.

[1] https://sourceware.org/ml/gdb-patches/1999-q3/msg00245.html, and
    https://sourceware.org/ml/gdb-patches/1999-q3/msg00242.html

In addition to the per-location issue, "permanent breakpoints" are
currently always displayed as enabled=='n':

 (gdb) b main
 Breakpoint 3 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29.
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 3       breakpoint     keep n   0x000000000040053c ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29

But OTOH they're always enabled; there's no way to disable them...

In turn, this means that if one adds commands to such a breakpoint,
they're _always_ run:

 (gdb) start
 Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.arch/i386-permbkpt
 ...
 Temporary breakpoint 1, main () at ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29
 29              int3
 (gdb) b main
 Breakpoint 2 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29.
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 2       breakpoint     keep n   0x000000000040053c ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29
 (gdb) commands
 Type commands for breakpoint(s) 2, one per line.
 End with a line saying just "end".
 >echo "hello!"
 >end
 (gdb) disable 2
 (gdb) start
 The program being debugged has been started already.
 Start it from the beginning? (y or n) y
 Temporary breakpoint 3 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29.
 Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.arch/i386-permbkpt

 Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29
 29              int3
 "hello!"(gdb)

IMO, one should be able to disable such a breakpoint, and GDB should
then behave just like if the user hadn't created the breakpoint in the
first place (that is, report a SIGTRAP).

By making permanent-ness a property of the location, and eliminating
the bp_permanent enum enable_state state ends up fixing that as well.

No tests are added for these changes yet; they'll be added in a follow
up patch, as skipping permanent breakpoints is currently broken and
trips on an assertion in infrun.

Tested on x86_64 Fedora 20.

gdb/ChangeLog:
2014-11-04  Pedro Alves  <palves@redhat.com>

	Mark locations as permanent, not the whole breakpoint.
	* breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust.
	(mark_breakpoints_out): Don't mark permanent breakpoints as
	uninserted.
	(breakpoint_init_inferior): Use mark_breakpoints_out.
	(breakpoint_here_p): Adjust.
	(bpstat_stop_status, describe_other_breakpoints): Remove handling
	of permanent breakpoints.
	(make_breakpoint_permanent): Mark each location as permanent,
	instead of marking the breakpoint.
	(add_location_to_breakpoint): If the location is permanent, mark
	it as such, and as inserted.
	(init_breakpoint_sal): Don't make the breakpoint permanent here.
	(bp_location_compare, update_global_location_list): Adjust.
	(update_breakpoint_locations): Don't make the breakpoint permanent
	here.
	(disable_breakpoint, enable_breakpoint_disp): Don't skip permanent
	breakpoints.
	* breakpoint.h (enum enable_state) <bp_permanent>: Delete field.
	(struct bp_location) <permanent>: New field.
---
 gdb/breakpoint.c | 71 ++++++++++++++++++++++----------------------------------
 gdb/breakpoint.h | 13 ++++++-----
 2 files changed, 35 insertions(+), 49 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bd51f5d..3ebe9c9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3883,7 +3883,7 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
   /* BL is never in moribund_locations by our callers.  */
   gdb_assert (bl->owner != NULL);
 
-  if (bl->owner->enable_state == bp_permanent)
+  if (bl->permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
 
@@ -4033,7 +4033,7 @@ remove_breakpoint (struct bp_location *bl, insertion_state_t is)
   /* BL is never in moribund_locations by our callers.  */
   gdb_assert (bl->owner != NULL);
 
-  if (bl->owner->enable_state == bp_permanent)
+  if (bl->permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
 
@@ -4059,7 +4059,8 @@ mark_breakpoints_out (void)
   struct bp_location *bl, **blp_tmp;
 
   ALL_BP_LOCATIONS (bl, blp_tmp)
-    if (bl->pspace == current_program_space)
+    if (bl->pspace == current_program_space
+	&& !bl->permanent)
       bl->inserted = 0;
 }
 
@@ -4088,13 +4089,7 @@ breakpoint_init_inferior (enum inf_context context)
   if (gdbarch_has_global_breakpoints (target_gdbarch ()))
     return;
 
-  ALL_BP_LOCATIONS (bl, blp_tmp)
-  {
-    /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL.  */
-    if (bl->pspace == pspace
-	&& bl->owner->enable_state != bp_permanent)
-      bl->inserted = 0;
-  }
+  mark_breakpoints_out ();
 
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
@@ -4202,14 +4197,14 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
 
       /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL.  */
       if ((breakpoint_enabled (bl->owner)
-	   || bl->owner->enable_state == bp_permanent)
+	   || bl->permanent)
 	  && breakpoint_location_address_match (bl, aspace, pc))
 	{
 	  if (overlay_debugging 
 	      && section_is_overlay (bl->section)
 	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
-	  else if (bl->owner->enable_state == bp_permanent)
+	  else if (bl->permanent)
 	    return permanent_breakpoint_here;
 	  else
 	    any_breakpoint_here = 1;
@@ -5475,7 +5470,7 @@ bpstat_stop_status (struct address_space *aspace,
 
   ALL_BREAKPOINTS (b)
     {
-      if (!breakpoint_enabled (b) && b->enable_state != bp_permanent)
+      if (!breakpoint_enabled (b))
 	continue;
 
       for (bl = b->loc; bl != NULL; bl = bl->next)
@@ -5571,8 +5566,7 @@ bpstat_stop_status (struct address_space *aspace,
 	      if (b->disposition == disp_disable)
 		{
 		  --(b->enable_count);
-		  if (b->enable_count <= 0
-		      && b->enable_state != bp_permanent)
+		  if (b->enable_count <= 0)
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
@@ -6856,8 +6850,6 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 			     ((b->enable_state == bp_disabled
 			       || b->enable_state == bp_call_disabled)
 			      ? " (disabled)"
-			      : b->enable_state == bp_permanent 
-			      ? " (permanent)"
 			      : ""),
 			     (others > 1) ? "," 
 			     : ((others == 1) ? " and" : ""));
@@ -7389,15 +7381,16 @@ make_breakpoint_permanent (struct breakpoint *b)
 {
   struct bp_location *bl;
 
-  b->enable_state = bp_permanent;
-
   /* By definition, permanent breakpoints are already present in the
      code.  Mark all locations as inserted.  For now,
      make_breakpoint_permanent is called in just one place, so it's
      hard to say if it's reasonable to have permanent breakpoint with
      multiple locations or not, but it's easy to implement.  */
   for (bl = b->loc; bl; bl = bl->next)
-    bl->inserted = 1;
+    {
+      bl->permanent = 1;
+      bl->inserted = 1;
+    }
 }
 
 /* Call this routine when stepping and nexting to enable a breakpoint
@@ -9227,6 +9220,8 @@ mention (struct breakpoint *b)
 }
 
 
+static int bp_loc_is_permanent (struct bp_location *loc);
+
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
@@ -9268,6 +9263,13 @@ add_location_to_breakpoint (struct breakpoint *b,
 
   set_breakpoint_location_function (loc,
 				    sal->explicit_pc || sal->explicit_line);
+
+  if (bp_loc_is_permanent (loc))
+    {
+      loc->inserted = 1;
+      loc->permanent = 1;
+    }
+
   return loc;
 }
 
@@ -9510,9 +9512,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	    loc->inserted = 1;
 	}
 
-      if (bp_loc_is_permanent (loc))
-	make_breakpoint_permanent (b);
-
       if (b->cond_string)
 	{
 	  const char *arg = b->cond_string;
@@ -12352,7 +12351,7 @@ breakpoint_auto_delete (bpstat bs)
 /* A comparison function for bp_location AP and BP being interfaced to
    qsort.  Sort elements primarily by their ADDRESS (no matter what
    does breakpoint_address_is_meaningful say for its OWNER),
-   secondarily by ordering first bp_permanent OWNERed elements and
+   secondarily by ordering first permanent elements and
    terciarily just ensuring the array is sorted stable way despite
    qsort being an unstable algorithm.  */
 
@@ -12361,9 +12360,6 @@ bp_location_compare (const void *ap, const void *bp)
 {
   struct bp_location *a = *(void **) ap;
   struct bp_location *b = *(void **) bp;
-  /* A and B come from existing breakpoints having non-NULL OWNER.  */
-  int a_perm = a->owner->enable_state == bp_permanent;
-  int b_perm = b->owner->enable_state == bp_permanent;
 
   if (a->address != b->address)
     return (a->address > b->address) - (a->address < b->address);
@@ -12377,8 +12373,8 @@ bp_location_compare (const void *ap, const void *bp)
 	    - (a->pspace->num < b->pspace->num));
 
   /* Sort permanent breakpoints first.  */
-  if (a_perm != b_perm)
-    return (a_perm < b_perm) - (a_perm > b_perm);
+  if (a->permanent != b->permanent)
+    return (a->permanent < b->permanent) - (a->permanent > b->permanent);
 
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
@@ -12849,7 +12845,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	}
 
       /* Permanent breakpoint should always be inserted.  */
-      if (b->enable_state == bp_permanent && ! loc->inserted)
+      if (loc->permanent && ! loc->inserted)
 	internal_error (__FILE__, __LINE__,
 			_("allegedly permanent breakpoint is not "
 			"actually inserted"));
@@ -12890,8 +12886,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
       /* Clear the condition modification flag.  */
       loc->condition_changed = condition_unchanged;
 
-      if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted
-	  && b->enable_state != bp_permanent)
+      if (loc->inserted && !loc->permanent
+	  && (*loc_first_p)->permanent)
 	internal_error (__FILE__, __LINE__,
 			_("another breakpoint was inserted on top of "
 			"a permanent breakpoint"));
@@ -14437,10 +14433,6 @@ update_breakpoint_locations (struct breakpoint *b,
 	}
     }
 
-  /* Update locations of permanent breakpoints.  */
-  if (b->enable_state == bp_permanent)
-    make_breakpoint_permanent (b);
-
   /* If possible, carry over 'disable' status from existing
      breakpoints.  */
   {
@@ -14923,10 +14915,6 @@ disable_breakpoint (struct breakpoint *bpt)
   if (bpt->type == bp_watchpoint_scope)
     return;
 
-  /* You can't disable permanent breakpoints.  */
-  if (bpt->enable_state == bp_permanent)
-    return;
-
   bpt->enable_state = bp_disabled;
 
   /* Mark breakpoint locations modified.  */
@@ -15047,9 +15035,6 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
 	}
     }
 
-  if (bpt->enable_state != bp_permanent)
-    bpt->enable_state = bp_enabled;
-
   bpt->enable_state = bp_enabled;
 
   /* Mark breakpoint locations modified.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 118a37f..c383cd4 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -193,12 +193,6 @@ enum enable_state
 			    automatically enabled and reset when the
 			    call "lands" (either completes, or stops
 			    at another eventpoint).  */
-    bp_permanent	 /* There is a breakpoint instruction
-			    hard-wired into the target's code.  Don't
-			    try to write another breakpoint
-			    instruction on top of it, or restore its
-			    value.  Step over it using the
-			    architecture's SKIP_INSN macro.  */
   };
 
 
@@ -376,6 +370,13 @@ struct bp_location
   /* Nonzero if this breakpoint is now inserted.  */
   char inserted;
 
+  /* Nonzero if this is a permanent breakpoint.  There is a breakpoint
+     instruction hard-wired into the target's code.  Don't try to
+     write another breakpoint instruction on top of it, or restore its
+     value.  Step over it using the architecture's
+     gdbarch_skip_permanent_breakpoint method.  */
+  char permanent;
+
   /* Nonzero if this is not the first breakpoint in the list
      for the given address.  location of tracepoint can _never_
      be duplicated with other locations of tracepoints and other
-- 
1.9.3