From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2189 invoked by alias); 17 Jan 2004 22:20:09 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 2176 invoked from network); 17 Jan 2004 22:20:07 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sources.redhat.com with SMTP; 17 Jan 2004 22:20:07 -0000 Received: from drow by nevyn.them.org with local (Exim 4.30 #1 (Debian)) id 1AhynX-0007W4-73 for ; Sat, 17 Jan 2004 17:20:07 -0500 Date: Sat, 17 Jan 2004 22:20:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sources.redhat.com Subject: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun Message-ID: <20040117222007.GA23563@nevyn.them.org> Mail-Followup-To: gdb-patches@sources.redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.1i X-SW-Source: 2004-01/txt/msg00454.txt.bz2 The first, possibly most important step in cleaning up DECR_PC_AFTER_BREAK's tentacles. This changes handle_inferior_event to fix the PC immediately, before doing anything else with it. It removes the later decrements, but doesn't remove all the later workarounds for possibly undecremented PC values - that can come separately. One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply removed. There are no targets using this combination, and if one is added, it's non-obvious whether a nonsteppable watchpoint really should be affected by DECR_PC_AFTER_BREAK. A future cleanup will change bpstat_stop_status to only take a CORE_ADDR argument. Also, I believe that both the tests for sigtramps and the use of deprecated_frame_update_pc_hack can go now, but I don't want to mix that in with this - esp. since I'm not sure how to test the former belief. Tested i386-linux, which uses DECR_PC_AFTER_BREAK. I'm not going to check this in just yet; all comments and testing welcome. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer 2004-01-17 Daniel Jacobowitz * breakpoint.c (software_breakpoint_inserted_here_p): New function. (bpstat_stop_status): Don't decrement PC. * breakpoint.h (software_breakpoint_inserted_here_p): Add prototype. * infrun.c (adjust_pc_after_break): New function. (handle_inferior_event): Call it, early. Remove later references to DECR_PC_AFTER_BREAK. (normal_stop): Add commentary. Index: breakpoint.c =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.c,v retrieving revision 1.149 diff -u -p -r1.149 breakpoint.c --- breakpoint.c 17 Jan 2004 21:56:12 -0000 1.149 +++ breakpoint.c 17 Jan 2004 22:05:39 -0000 @@ -1721,6 +1721,37 @@ breakpoint_inserted_here_p (CORE_ADDR pc return 0; } +/* This function returns non-zero iff there is a software breakpoint + inserted at PC. */ + +int +software_breakpoint_inserted_here_p (CORE_ADDR pc) +{ + struct bp_location *bpt; + int any_breakpoint_here = 0; + + ALL_BP_LOCATIONS (bpt) + { + if (bpt->loc_type != bp_loc_software_breakpoint) + continue; + + if ((breakpoint_enabled (bpt->owner) + || bpt->owner->enable_state == bp_permanent) + && bpt->inserted + && bpt->address == pc) /* bp is enabled and matches pc */ + { + if (overlay_debugging + && section_is_overlay (bpt->section) + && !section_is_mapped (bpt->section)) + continue; /* unmapped overlay -- can't be a match */ + else + return 1; + } + } + + return 0; +} + /* Return nonzero if FRAME is a dummy frame. We can't use DEPRECATED_PC_IN_CALL_DUMMY because figuring out the saved SP would take too much time, at least using frame_register() on the 68k. @@ -2578,13 +2609,7 @@ bpstat_stop_status (CORE_ADDR *pc, int n /* Pointer to the last thing in the chain currently. */ bpstat bs = root_bs; - /* Get the address where the breakpoint would have been. The - "not_a_sw_breakpoint" argument is meant to distinguish between a - breakpoint trap event and a trace/singlestep trap event. For a - trace/singlestep trap event, we would not want to subtract - DECR_PC_AFTER_BREAK from the PC. */ - - bp_addr = *pc - (not_a_sw_breakpoint ? 0 : DECR_PC_AFTER_BREAK); + bp_addr = *pc; ALL_BREAKPOINTS_SAFE (b, temp) { @@ -2869,18 +2894,6 @@ bpstat_stop_status (CORE_ADDR *pc, int n bs->next = NULL; /* Terminate the chain */ bs = root_bs->next; /* Re-grab the head of the chain */ - - if (real_breakpoint && bs) - { - if (bs->breakpoint_at->type != bp_hardware_breakpoint) - { - if (DECR_PC_AFTER_BREAK != 0) - { - *pc = bp_addr; - write_pc (bp_addr); - } - } - } /* The value of a hardware watchpoint hasn't changed, but the intermediate memory locations we are watching may have. */ Index: breakpoint.h =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.h,v retrieving revision 1.26 diff -u -p -r1.26 breakpoint.h --- breakpoint.h 6 Nov 2003 18:24:55 -0000 1.26 +++ breakpoint.h 17 Jan 2004 22:05:39 -0000 @@ -605,6 +605,8 @@ extern enum breakpoint_here breakpoint_h extern int breakpoint_inserted_here_p (CORE_ADDR); +extern int software_breakpoint_inserted_here_p (CORE_ADDR); + /* FIXME: cagney/2002-11-10: The current [generic] dummy-frame code implements a functional superset of this function. The only reason it hasn't been removed is because some architectures still don't Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.128 diff -u -p -r1.128 infrun.c --- infrun.c 17 Jan 2004 21:56:12 -0000 1.128 +++ infrun.c 17 Jan 2004 22:05:39 -0000 @@ -1316,6 +1316,59 @@ handle_step_into_function (struct execut return; } +static void +adjust_pc_after_break (struct execution_control_state *ecs) +{ + CORE_ADDR stop_pc; + + /* If this target does not decrement the PC after breakpoints, then + we have nothing to do. */ + if (DECR_PC_AFTER_BREAK == 0) + return; + + /* If we've hit a breakpoint, we'll be stopped with SIGTRAP. */ + if (ecs->ws.kind != TARGET_WAITKIND_STOPPED) + return; + + if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP) + return; + + /* Find the location where (if we've hit a breakpoint) the breakpoint would + be. */ + stop_pc = read_pc_pid (ecs->ptid) - DECR_PC_AFTER_BREAK; + + /* If we're software-single-stepping, then assume this is a breakpoint. + NOTE drow/2004-01-17: This doesn't check that the PC matches, or that + we're even in the right thread. The software-single-step code needs + some modernization. + + If we're not software-single-stepping, then we first check that there + is an enabled software breakpoint at this address. If there is, and + we weren't using hardware-single-step, then we've hit the breakpoint. + + If we were using hardware-single-step, we check prev_pc; if we just + stepped over an inserted software breakpoint, then we should decrement + the PC and eventually report hitting the breakpoint. The prev_pc check + prevents us from decrementing the PC if we just stepped over a jump + instruction and landed on the instruction after a breakpoint. + + The last bit checks that we didn't hit a breakpoint in a signal handler + without an intervening stop in sigtramp, which is detected by a new + stack pointer value below any usual function calling stack adjustments. + + NOTE drow/2004-01-17: I'm not sure that this is necessary. The check + predates checking for software single step at the same time. Also, + if we've moved into a signal handler we should have seen the + signal. */ + + if ((SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p) + || (software_breakpoint_inserted_here_p (stop_pc) + && !(currently_stepping (ecs) + && prev_pc != stop_pc + && !(step_range_end && INNER_THAN (read_sp (), (step_sp - 16)))))) + write_pc_pid (stop_pc, ecs->ptid); +} + /* Given an execution control state that has been freshly filled in by an event from the inferior, figure out what it means and take appropriate action. */ @@ -1335,6 +1388,8 @@ handle_inferior_event (struct execution_ target_last_wait_ptid = ecs->ptid; target_last_waitstatus = *ecs->wp; + adjust_pc_after_break (ecs); + switch (ecs->infwait_state) { case infwait_thread_hop_state: @@ -1688,19 +1743,15 @@ handle_inferior_event (struct execution_ /* Check if a regular breakpoint has been hit before checking for a potential single step breakpoint. Otherwise, GDB will not see this breakpoint hit when stepping onto breakpoints. */ - if (breakpoints_inserted - && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK)) + if (breakpoints_inserted && breakpoint_here_p (stop_pc)) { ecs->random_signal = 0; - if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK, - ecs->ptid)) + if (!breakpoint_thread_match (stop_pc, ecs->ptid)) { int remove_status; /* Saw a breakpoint, but it was hit by the wrong thread. Just continue. */ - if (DECR_PC_AFTER_BREAK) - write_pc_pid (stop_pc - DECR_PC_AFTER_BREAK, ecs->ptid); remove_status = remove_breakpoints (); /* Did we fail to remove breakpoints? If so, try @@ -1713,7 +1764,7 @@ handle_inferior_event (struct execution_ if (remove_status != 0) { /* FIXME! This is obviously non-portable! */ - write_pc_pid (stop_pc - DECR_PC_AFTER_BREAK + 4, ecs->ptid); + write_pc_pid (stop_pc + 4, ecs->ptid); /* We need to restart all the threads now, * unles we're running in scheduler-locked mode. * Use currently_stepping to determine whether to @@ -1747,17 +1798,6 @@ handle_inferior_event (struct execution_ } else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p) { - /* Readjust the stop_pc as it is off by DECR_PC_AFTER_BREAK - compared to the value it would have if the system stepping - capability was used. This allows the rest of the code in - this function to use this address without having to worry - whether software single step is in use or not. */ - if (DECR_PC_AFTER_BREAK) - { - stop_pc -= DECR_PC_AFTER_BREAK; - write_pc_pid (stop_pc, ecs->ptid); - } - sw_single_step_trap_p = 1; ecs->random_signal = 0; } @@ -1889,9 +1929,6 @@ handle_inferior_event (struct execution_ includes evaluating watchpoints, things will come to a stop in the correct manner. */ - if (DECR_PC_AFTER_BREAK) - write_pc (stop_pc - DECR_PC_AFTER_BREAK); - remove_breakpoints (); registers_changed (); target_resume (ecs->ptid, 1, TARGET_SIGNAL_0); /* Single step */ @@ -3101,6 +3138,7 @@ normal_stop (void) previous_inferior_ptid = inferior_ptid; } + /* NOTE drow/2004-01-17: Is this still necessary? */ /* Make sure that the current_frame's pc is correct. This is a correction for setting up the frame info before doing DECR_PC_AFTER_BREAK */