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

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

[Kevin Buettner]

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;

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

[Fernando Nasser]

Way to go Kevin!  Thank you very much for tracking this down.

I agree with your solution.  Actually, one day, in the lost past, this
function did have more arguments.  Look at the comments for
set_raw_breakpoint():

/* Low level routine to set a breakpoint.
   Takes as args the three things that every breakpoint must have.

BTW, when you check in you can change the comment as well to reflect the
current version.


Michael, Jim: I have reviewed Kevin's patch and it looks really nice. 

Regards to all,
Fernando


Kevin Buettner wrote:
> 
> 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;

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Jim Blandy]

This change is approved.

(Thank you for finding and fixing my bug!)

Kevin Buettner <kevinb@cygnus.com> writes:

> 
> 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;
> 
> 

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

[Michael Snyder]

Kevin, I like it, but I noticed that set_raw_breakpoint is not static.
It was made extern in 1998, and is used by gdbtk-cmds.c.

Can you check to see if that call needs a bp_type argument, and if that
call can't be eliminated, maybe you should add a prototype to breakpoint.h.

				Thanks,
				Michael


Kevin Buettner wrote:
> 
> 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;

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

[Fernando Nasser]

Michael Snyder wrote:
> 
> Kevin, I like it, but I noticed that set_raw_breakpoint is not static.
> It was made extern in 1998, and is used by gdbtk-cmds.c.
> 
> Can you check to see if that call needs a bp_type argument, and if that
> call can't be eliminated, maybe you should add a prototype to breakpoint.h.
> 

I believe it can be made static now.  I don't think anyone should be
using it outside breakpoint.c.

Keith, does your new gdbtk-bp.c uses this function?

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Kevin Buettner]

On May 11,  1:27pm, Fernando Nasser wrote:

> > Kevin, I like it, but I noticed that set_raw_breakpoint is not static.
> > It was made extern in 1998, and is used by gdbtk-cmds.c.
> > 
> > Can you check to see if that call needs a bp_type argument, and if that
> > call can't be eliminated, maybe you should add a prototype to breakpoint.h.
> 
> I believe it can be made static now.  I don't think anyone should be
> using it outside breakpoint.c.
> 
> Keith, does your new gdbtk-bp.c uses this function?

Does Keith have a version that's not checked in yet?

I did an update a moment ago and notice that set_raw_breakpoint is
still used by gdbtk-bp.c.  Here's a telling comment from this file...

    /*
     * These are routines we need from breakpoint.c.
     * at some point make these static in breakpoint.c and move GUI code there
     */

    extern struct breakpoint *set_raw_breakpoint (struct symtab_and_line sal);
    extern void set_breakpoint_count (int);
    extern int breakpoint_count;

I will adjust the set_raw_breakpoint() calls in gdbtk-bp.c to pass the
extra argument.  I'll let Keith (or someone else more familiar with
these issues than I am) decide whether a prototype ought to be added
to breakpoint.h or if the calls in gdbtk-bp.c can be eliminated (thus
allowing breakpoint.c to make set_raw_breakpoint() static again).

Kevin

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

[Kevin Buettner]

On May 11, 11:37am, Fernando Nasser wrote:

> I agree with your solution.  Actually, one day, in the lost past, this
> function did have more arguments.  Look at the comments for
> set_raw_breakpoint():
> 
> /* Low level routine to set a breakpoint.
>    Takes as args the three things that every breakpoint must have.
> 
> BTW, when you check in you can change the comment as well to reflect the
> current version.

Fernando,

Thanks for calling my attention to the set_raw_breakpoint() comment.
I have rewritten it from:

/* Low level routine to set a breakpoint.
   Takes as args the three things that every breakpoint must have.
   Returns the breakpoint object so caller can set other things.
   Does not set the breakpoint number!
   Does not print anything.

   ==> This routine should not be called if there is a chance of later
   error(); otherwise it leaves a bogus breakpoint on the chain.  Validate
   your arguments BEFORE calling this routine!  */

To:

/* set_raw_breakpoint() is a low level routine for allocating and
   partially initializing a breakpoint of type BPTYPE.  The newly
   created breakpoint's address, section, source file name, and line
   number are provided by SAL.  The newly created and partially
   initialized breakpoint is added to the breakpoint chain and
   is also returned as the value of this function.

   It is expected that the caller will complete the initialization of
   the newly created breakpoint struct as well as output any status
   information regarding the creation of a new breakpoint.  In
   particular, set_raw_breakpoint() does NOT set the breakpoint
   number!  Care should be taken to not allow an error() to occur
   prior to completing the initializtion of the breakpoint.  If this
   should happen, a bogus breakpoint will be left on the chain.  */

Kevin

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

[Kevin Buettner]

On May 11, 12:50am, Kevin Buettner wrote:

> 	* 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().

Committed.

Thanks to Fernando Nasser, Jim Blandy, and Michael Snyder for their
comments on this patch.

Kevin

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

[Keith Seitz]

On Fri, 11 May 2001, Fernando Nasser wrote:

> I believe it can be made static now.  I don't think anyone should be
> using it outside breakpoint.c.
>
> Keith, does your new gdbtk-bp.c uses this function?

gdbtk-bp.c is just a copy of the original routines from gdbtk-cmds.c. It
is still needed. I was monitoring this thread for a check-in approval,
ready to check in a patch for Insight...

Keith