From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Snyder To: Kevin Buettner Cc: Jim Blandy , gdb-patches@sources.redhat.com Subject: Re: [PATCH RFA] breakpoint.c: Pass breakpoint type to set_raw_breakpoint() Date: Fri, 11 May 2001 10:18:00 -0000 Message-id: <3AFC1ED7.9458AEE1@cygnus.com> References: <1010511075057.ZM27226@ocotillo.lan> X-SW-Source: 2001-05/msg00221.html 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;