[PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
Jim Blandy
jimb@zwingli.cygnus.com
Fri May 11 10:16:00 GMT 2001
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;
>
>
More information about the Gdb-patches
mailing list