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