This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun
- From: Daniel Jacobowitz <drow at mvista dot com>
- To: gdb-patches at sources dot redhat dot com
- Date: Sat, 17 Jan 2004 17:20:07 -0500
- Subject: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun
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 <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.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 */