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

[Fernando Nasser <fnasser at redhat dot com>]

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