[PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()

[Kevin Buettner <kevinb at cygnus dot com>]

The following patch fixes the regressions reported by Fernando Nasser
in:

    http://sources.redhat.com/ml/gdb/2001-05/msg00230.html

The change reponsible for these regressions was recently posted by Jim
Blandy:

    http://sources.redhat.com/ml/gdb-patches/2001-05/msg00062.html

Here's the relevant ChangeLog entry:

	* breakpoint.c (check_duplicates): Use the breakpoint's type, not
	its address, to decide whether it's a watchpoint or not.  Zero
	is a valid code address.

I think Jim's change to check_duplicates() improves the clarity and
correctness of the code and should *not* be reverted.  Instead,
there's a bit more that needs to be done elsewhere to make sure that
it will work as expected.

The old check_duplicates() code was testing to see if the breakpoint
address was 0.  If it was, it was considered to be a watchpoint
causing check_duplicates() to return early.  Here's what the old
code looked like:

      if (address == 0)         /* Watchpoints are uninteresting */
        return;

The reason that this code worked is due to the fact that
watch_command_1() zeroes out the struct which is eventually used to
set the breakpoint's address.  This was, IMO, an overly subtle way of
using a zero valued address as a flag to indicate that a breakpoint
was actually a watchpoint.

Jim changed the early-return test in check_duplicates() to actually
check to see if the ``type'' member of the breakpoint struct under
consideration is a watchpoint:

      /* Watchpoints are uninteresting.  */
      if (bpt->type == bp_watchpoint
          || bpt->type == bp_hardware_watchpoint
          || bpt->type == bp_read_watchpoint
          || bpt->type == bp_access_watchpoint)
        return;

Jim's change improves the clarity of the code by removing the reliance
on the undocumented, and nonobvious trick of using zero valued
addresses as a watchpoint flag.  It also improves correctness by
making it possible for the zero address to be used as the address of a
breakpoint.

The only fly in the ointment is that check_duplicates() is called from
within set_raw_breakpoint() before ``type'' has been set.  The
memset() in this function was causing ``type'' to appear as bp_none,
which indicates a deleted breakpoint.  (If you ever actually see a
breakpoint with type bp_none, it most likely means that there's a
problem (internal error) in GDB.)

Jim's early-return test doesn't test for bp_none.  (Nor should it,
though my first inclination was to add a bp_none test.)  This means
that the watchpoints and any other zero valued breakpoints would get
marked as duplicates of each other.  In the testsuite test in which
the regressions showed up, the first watchpoint would work, but
subsequent ones wouldn't due to being marked as duplicates of the
first.

There are a number of possible solutions to this problem.  One
solution would be to move the check_duplicates call out of
set_raw_breakpoint() and make sure it gets called after ``type'' has
been set.  I don't like this solution because it increases the number
of locations from which check_duplicates() must be called.

A better solution, I think, is to move the setting of the breakpoint's
type into set_raw_breakpoint().  The patch below implements this
change by adding a type parameter to set_raw_breakpoint(), and
changing each caller to pass the breakpoint's type.  Now, by the time
check_duplicates() gets called from set_raw_breakpoint(), the ``type''
member has been set correctly and Jim's early-return test in
check_duplicates() will work as expected.

I like this solution better because, in addition to solving the
problem, it also decreases the number of places that a breakpoint
struct's ``type'' member is initialized.  Also, it moves the
initialization of a member that must be set into the function whose
job it is to perform such initializations.  This means that it will be
impossible to inadvertently forget to do this initialization should any
new calls to set_raw_breakpoint() be added.

Okay to apply?

	* breakpoint.c (set_raw_breakpoint): Add new parameter
	representing the breakpoint's type.  Adjust all callers.
	(create_longjmp_breakpoint, create_temp_exception_breakpoint)
	(create_thread_event_breakpoint): Don't test for zero return
	value from set_raw_breakpoint().  It can never be zero.
	(create_exception_catchpoint, watch_command_1): Move logic
	which calculates the breakpoint type prior to the call to
	set_raw_breakpoint().
	
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.35
diff -u -p -r1.35 breakpoint.c
--- breakpoint.c	2001/05/06 22:22:02	1.35
+++ breakpoint.c	2001/05/11 06:21:01
@@ -91,7 +91,7 @@ static void break_command_1 (char *, int
 
 static void mention (struct breakpoint *);
 
-struct breakpoint *set_raw_breakpoint (struct symtab_and_line);
+struct breakpoint *set_raw_breakpoint (struct symtab_and_line, enum bptype);
 
 static void check_duplicates (struct breakpoint *);
 
@@ -3817,7 +3817,7 @@ check_duplicates (struct breakpoint *bpt
    your arguments BEFORE calling this routine!  */
 
 struct breakpoint *
-set_raw_breakpoint (struct symtab_and_line sal)
+set_raw_breakpoint (struct symtab_and_line sal, enum bptype bptype)
 {
   register struct breakpoint *b, *b1;
 
@@ -3830,6 +3830,7 @@ set_raw_breakpoint (struct symtab_and_li
     b->source_file = savestring (sal.symtab->filename,
 				 strlen (sal.symtab->filename));
   b->section = sal.section;
+  b->type = bptype;
   b->language = current_language->la_language;
   b->input_radix = input_radix;
   b->thread = -1;
@@ -3898,11 +3899,9 @@ create_longjmp_breakpoint (char *func_na
 	return;
     }
   sal.section = find_pc_overlay (sal.pc);
-  b = set_raw_breakpoint (sal);
-  if (!b)
-    return;
+  b = set_raw_breakpoint (sal,
+                          func_name != NULL ? bp_longjmp : bp_longjmp_resume);
 
-  b->type = func_name != NULL ? bp_longjmp : bp_longjmp_resume;
   b->disposition = donttouch;
   b->enable = disabled;
   b->silent = 1;
@@ -3954,13 +3953,10 @@ create_thread_event_breakpoint (CORE_ADD
   INIT_SAL (&sal);		/* initialize to zeroes */
   sal.pc = address;
   sal.section = find_pc_overlay (sal.pc);
-  if ((b = set_raw_breakpoint (sal)) == NULL)
-    return NULL;
+  b = set_raw_breakpoint (sal, bp_thread_event);
   
   b->number = internal_breakpoint_number--;
   b->disposition = donttouch;
-  b->type = bp_thread_event;	/* XXX: do we need a new type? 
-				   bp_thread_event */
   b->enable = enabled;
   /* addr_string has to be used or breakpoint_re_set will delete me.  */
   sprintf (addr_string, "*0x%s", paddr (b->address));
@@ -3999,10 +3995,9 @@ create_solib_event_breakpoint (CORE_ADDR
   INIT_SAL (&sal);		/* initialize to zeroes */
   sal.pc = address;
   sal.section = find_pc_overlay (sal.pc);
-  b = set_raw_breakpoint (sal);
+  b = set_raw_breakpoint (sal, bp_shlib_event);
   b->number = internal_breakpoint_number--;
   b->disposition = donttouch;
-  b->type = bp_shlib_event;
 
   return b;
 }
@@ -4110,7 +4105,7 @@ solib_load_unload_1 (char *hookname, int
   if (canonical != (char **) NULL)
     discard_cleanups (canonical_strings_chain);
 
-  b = set_raw_breakpoint (sals.sals[0]);
+  b = set_raw_breakpoint (sals.sals[0], bp_kind);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
   b->cond = NULL;
@@ -4133,7 +4128,6 @@ solib_load_unload_1 (char *hookname, int
       b->dll_pathname = (char *) xmalloc (strlen (dll_pathname) + 1);
       strcpy (b->dll_pathname, dll_pathname);
     }
-  b->type = bp_kind;
 
   mention (b);
   do_cleanups (old_chain);
@@ -4168,7 +4162,7 @@ create_fork_vfork_event_catchpoint (int 
   sal.symtab = NULL;
   sal.line = 0;
 
-  b = set_raw_breakpoint (sal);
+  b = set_raw_breakpoint (sal, bp_kind);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
   b->cond = NULL;
@@ -4180,8 +4174,6 @@ create_fork_vfork_event_catchpoint (int 
   b->disposition = tempflag ? del : donttouch;
   b->forked_inferior_pid = 0;
 
-  b->type = bp_kind;
-
   mention (b);
 }
 
@@ -4209,7 +4201,7 @@ create_exec_event_catchpoint (int tempfl
   sal.symtab = NULL;
   sal.line = 0;
 
-  b = set_raw_breakpoint (sal);
+  b = set_raw_breakpoint (sal, bp_catch_exec);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
   b->cond = NULL;
@@ -4220,8 +4212,6 @@ create_exec_event_catchpoint (int tempfl
   b->enable = enabled;
   b->disposition = tempflag ? del : donttouch;
 
-  b->type = bp_catch_exec;
-
   mention (b);
 }
 
@@ -4338,8 +4328,7 @@ set_momentary_breakpoint (struct symtab_
 			  enum bptype type)
 {
   register struct breakpoint *b;
-  b = set_raw_breakpoint (sal);
-  b->type = type;
+  b = set_raw_breakpoint (sal, type);
   b->enable = enabled;
   b->disposition = donttouch;
   b->frame = (frame ? frame->frame : 0);
@@ -4563,10 +4552,9 @@ create_breakpoints (struct symtabs_and_l
 	if (from_tty)
 	  describe_other_breakpoints (sal.pc, sal.section);
 	
-	b = set_raw_breakpoint (sal);
+	b = set_raw_breakpoint (sal, type);
 	set_breakpoint_count (breakpoint_count + 1);
 	b->number = breakpoint_count;
-	b->type = type;
 	b->cond = cond[i];
 	b->thread = thread;
 	b->addr_string = addr_string[i];
@@ -5366,8 +5354,13 @@ watch_command_1 (char *arg, int accessfl
     }
 #endif /* HPUXHPPA */
 
+  /* Change the type of breakpoint to an ordinary watchpoint if a hardware
+     watchpoint could not be set.  */
+  if (!mem_cnt || target_resources_ok <= 0)
+    bp_type = bp_watchpoint;
+
   /* Now set up the breakpoint.  */
-  b = set_raw_breakpoint (sal);
+  b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
   b->disposition = donttouch;
@@ -5390,11 +5383,6 @@ watch_command_1 (char *arg, int accessfl
   else
     b->watchpoint_frame = (CORE_ADDR) 0;
 
-  if (mem_cnt && target_resources_ok > 0)
-    b->type = bp_type;
-  else
-    b->type = bp_watchpoint;
-
   /* If the expression is "local", then set up a "watchpoint scope"
      breakpoint at the point where we've left the scope of the watchpoint
      expression.  */
@@ -5409,11 +5397,11 @@ watch_command_1 (char *arg, int accessfl
 	  scope_sal.pc = get_frame_pc (prev_frame);
 	  scope_sal.section = find_pc_overlay (scope_sal.pc);
 
-	  scope_breakpoint = set_raw_breakpoint (scope_sal);
+	  scope_breakpoint = set_raw_breakpoint (scope_sal,
+	                                         bp_watchpoint_scope);
 	  set_breakpoint_count (breakpoint_count + 1);
 	  scope_breakpoint->number = breakpoint_count;
 
-	  scope_breakpoint->type = bp_watchpoint_scope;
 	  scope_breakpoint->enable = enabled;
 
 	  /* Automatically delete the breakpoint when it hits.  */
@@ -6143,33 +6131,33 @@ create_exception_catchpoint (int tempfla
 {
   struct breakpoint *b;
   int thread = -1;		/* All threads. */
+  enum bptype bptype;
 
   if (!sal)			/* no exception support? */
     return;
 
-  b = set_raw_breakpoint (*sal);
-  set_breakpoint_count (breakpoint_count + 1);
-  b->number = breakpoint_count;
-  b->cond = NULL;
-  b->cond_string = (cond_string == NULL) ? 
-    NULL : savestring (cond_string, strlen (cond_string));
-  b->thread = thread;
-  b->addr_string = NULL;
-  b->enable = enabled;
-  b->disposition = tempflag ? del : donttouch;
   switch (ex_event)
     {
     case EX_EVENT_THROW:
-      b->type = bp_catch_throw;
+      bptype = bp_catch_throw;
       break;
     case EX_EVENT_CATCH:
-      b->type = bp_catch_catch;
+      bptype = bp_catch_catch;
       break;
     default:			/* error condition */
-      b->type = bp_none;
-      b->enable = disabled;
       error ("Internal error -- invalid catchpoint kind");
     }
+
+  b = set_raw_breakpoint (*sal, bptype);
+  set_breakpoint_count (breakpoint_count + 1);
+  b->number = breakpoint_count;
+  b->cond = NULL;
+  b->cond_string = (cond_string == NULL) ? 
+    NULL : savestring (cond_string, strlen (cond_string));
+  b->thread = thread;
+  b->addr_string = NULL;
+  b->enable = enabled;
+  b->disposition = tempflag ? del : donttouch;
   mention (b);
 }
 
@@ -6322,16 +6310,14 @@ handle_gnu_4_16_catch_command (char *arg
       if (from_tty)
 	describe_other_breakpoints (sal.pc, sal.section);
 
-      b = set_raw_breakpoint (sal);
-      set_breakpoint_count (breakpoint_count + 1);
-      b->number = breakpoint_count;
-
       /* Important -- this is an ordinary breakpoint.  For platforms
 	 with callback support for exceptions,
 	 create_exception_catchpoint() will create special bp types
 	 (bp_catch_catch and bp_catch_throw), and there is code in
 	 insert_breakpoints() and elsewhere that depends on that. */
-      b->type = bp_breakpoint;	
+      b = set_raw_breakpoint (sal, bp_breakpoint);
+      set_breakpoint_count (breakpoint_count + 1);
+      b->number = breakpoint_count;
 
       b->cond = cond;
       b->enable = enabled;
@@ -6362,11 +6348,8 @@ create_temp_exception_breakpoint (CORE_A
   sal.symtab = NULL;
   sal.line = 0;
 
-  b = set_raw_breakpoint (sal);
-  if (!b)
-    error ("Internal error -- couldn't set temp exception breakpoint");
+  b = set_raw_breakpoint (sal, bp_breakpoint);
 
-  b->type = bp_breakpoint;
   b->disposition = del;
   b->enable = enabled;
   b->silent = 1;
@@ -6502,10 +6485,9 @@ struct breakpoint *
 set_breakpoint_sal (struct symtab_and_line sal)
 {
   struct breakpoint *b;
-  b = set_raw_breakpoint (sal);
+  b = set_raw_breakpoint (sal, bp_breakpoint);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->type = bp_breakpoint;
   b->cond = 0;
   b->thread = -1;
   return b;