RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun
Daniel Jacobowitz
drow@mvista.com
Sat Jan 31 19:19:00 GMT 2004
On Sat, Jan 17, 2004 at 05:20:07PM -0500, Daniel Jacobowitz wrote:
> 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.
I've checked in this version, with comment revisions (thanks Eli!) and
testing on AIX and Tru64 (thanks Joel!). The latter convinced me that
something is seriously wrong on both of those targets, but the patch
had a non-deterministic affect on the existing failures.
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
2004-01-31 Daniel Jacobowitz <drow@mvista.com>
* 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.152
diff -u -p -r1.152 breakpoint.c
--- breakpoint.c 28 Jan 2004 01:39:51 -0000 1.152
+++ breakpoint.c 31 Jan 2004 17:54:20 -0000
@@ -1717,6 +1717,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.
@@ -2571,13 +2602,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)
{
@@ -2862,18 +2887,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 31 Jan 2004 17:54:20 -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.131
diff -u -p -r1.131 infrun.c
--- infrun.c 21 Jan 2004 16:32:07 -0000 1.131
+++ infrun.c 31 Jan 2004 17:54:20 -0000
@@ -1313,6 +1313,74 @@ 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 normally be stopped with SIGTRAP. If
+ we aren't, just return.
+
+ NOTE drow/2004-01-31: On some targets, breakpoints may generate
+ different signals (SIGILL or SIGEMT for instance), but it is less
+ clear where the PC is pointing afterwards. It may not match
+ DECR_PC_AFTER_BREAK. I don't know any specific target that generates
+ these signals at breakpoints (the code has been in GDB since at least
+ 1992) so I can not guess how to handle them here.
+
+ In earlier versions of GDB, a target with HAVE_NONSTEPPABLE_WATCHPOINTS
+ would have the PC after hitting a watchpoint affected by
+ DECR_PC_AFTER_BREAK. I haven't found any target with both of these set
+ in GDB history, and it seems unlikely to be correct, so
+ HAVE_NONSTEPPABLE_WATCHPOINTS is not checked here. */
+
+ 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. */
@@ -1332,6 +1400,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:
@@ -1685,19 +1755,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
@@ -1710,7 +1776,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
@@ -1744,17 +1810,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;
}
@@ -1886,9 +1941,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 */
@@ -3121,6 +3173,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 */
More information about the Gdb-patches
mailing list