* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 0:51 [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint() Kevin Buettner
@ 2001-05-11 8:39 ` Fernando Nasser
2001-05-11 12:04 ` Kevin Buettner
2001-05-11 10:16 ` Jim Blandy
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Fernando Nasser @ 2001-05-11 8:39 UTC (permalink / raw)
To: Kevin Buettner; +Cc: Michael Snyder, Jim Blandy, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 8:39 ` Fernando Nasser
@ 2001-05-11 12:04 ` Kevin Buettner
0 siblings, 0 replies; 9+ messages in thread
From: Kevin Buettner @ 2001-05-11 12:04 UTC (permalink / raw)
To: Fernando Nasser, Kevin Buettner; +Cc: Michael Snyder, Jim Blandy, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 0:51 [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint() Kevin Buettner
2001-05-11 8:39 ` Fernando Nasser
@ 2001-05-11 10:16 ` Jim Blandy
2001-05-11 10:18 ` Michael Snyder
2001-05-11 13:10 ` Kevin Buettner
3 siblings, 0 replies; 9+ messages in thread
From: Jim Blandy @ 2001-05-11 10:16 UTC (permalink / raw)
To: Kevin Buettner; +Cc: Michael Snyder, Jim Blandy, gdb-patches
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;
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 0:51 [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint() Kevin Buettner
2001-05-11 8:39 ` Fernando Nasser
2001-05-11 10:16 ` Jim Blandy
@ 2001-05-11 10:18 ` Michael Snyder
2001-05-11 10:29 ` Fernando Nasser
2001-05-11 13:10 ` Kevin Buettner
3 siblings, 1 reply; 9+ messages in thread
From: Michael Snyder @ 2001-05-11 10:18 UTC (permalink / raw)
To: Kevin Buettner; +Cc: Jim Blandy, gdb-patches
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;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 10:18 ` Michael Snyder
@ 2001-05-11 10:29 ` Fernando Nasser
2001-05-11 11:35 ` Kevin Buettner
2001-05-11 16:52 ` Keith Seitz
0 siblings, 2 replies; 9+ messages in thread
From: Fernando Nasser @ 2001-05-11 10:29 UTC (permalink / raw)
To: Michael Snyder; +Cc: Kevin Buettner, Jim Blandy, gdb-patches, Keith Seitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 10:29 ` Fernando Nasser
@ 2001-05-11 11:35 ` Kevin Buettner
2001-05-11 16:52 ` Keith Seitz
1 sibling, 0 replies; 9+ messages in thread
From: Kevin Buettner @ 2001-05-11 11:35 UTC (permalink / raw)
To: Fernando Nasser, Michael Snyder
Cc: Kevin Buettner, Jim Blandy, gdb-patches, Keith Seitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 10:29 ` Fernando Nasser
2001-05-11 11:35 ` Kevin Buettner
@ 2001-05-11 16:52 ` Keith Seitz
1 sibling, 0 replies; 9+ messages in thread
From: Keith Seitz @ 2001-05-11 16:52 UTC (permalink / raw)
To: Fernando Nasser; +Cc: Michael Snyder, Kevin Buettner, Jim Blandy, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint()
2001-05-11 0:51 [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint() Kevin Buettner
` (2 preceding siblings ...)
2001-05-11 10:18 ` Michael Snyder
@ 2001-05-11 13:10 ` Kevin Buettner
3 siblings, 0 replies; 9+ messages in thread
From: Kevin Buettner @ 2001-05-11 13:10 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread