[rfc] Fix infrun.c fall-out from the software single-step logic change

Ulrich Weigand
2007-06-15T19:26:36Z

Hello,

this fixes one outstanding issue after the ppc software single-step patch.

adjust_pc_after_break was using the assumption that SOFTWARE_SINGLE_STEP_P
was true if and only if we're using software single-step. With the new
logic this is no longer true; instead the arch can decide on a case-by-case
basis whether to use software or hardware single-stepping.

This patch re-works the adjust_pc_after_break logic to remove that
assumption. It also simplifies the logic a bit, due to the fact that
now the software single-step breakpoints also show up via
software_breakpoint_inserted_here_p.

I've also removed a couple of checks for SOFTWARE_SINGLE_STEP_P in
handle_inferior_event -- simply checking for singlestep_breakpoints_inserted_p
should be OK now.

Tested on powerpc64-linux and spu-elf.

Any comments? I don't actually have a test case where the old code
shows a bug, but it is clearly wrong ...

Bye,
Ulrich

ChangeLog:

* infrun.c (adjust_pc_after_break): Do not assume software single-step
is always active if SOFTWARE_SINGLE_STEP_P is true.
(handle_inferior_event): Do not check for SOFTWARE_SINGLE_STEP_P.

diff -urNp gdb-orig/gdb/infrun.c gdb-head/gdb/infrun.c
--- gdb-orig/gdb/infrun.c 2007-06-15 19:20:34.030743111 +0200
+++ gdb-head/gdb/infrun.c 2007-06-15 21:08:14.534043469 +0200
@@ -1183,53 +1183,34 @@ adjust_pc_after_break (struct execution_
breakpoint_pc = read_pc_pid (ecs->ptid) - gdbarch_decr_pc_after_break
(current_gdbarch);

- if (SOFTWARE_SINGLE_STEP_P ())
- {
- /* When using software single-step, a SIGTRAP can only indicate
- an inserted breakpoint. This actually makes things
- easier. */
- if (singlestep_breakpoints_inserted_p)
- /* When software single stepping, the instruction at [prev_pc]
- is never a breakpoint, but the instruction following
- [prev_pc] (in program execution order) always is. Assume
- that following instruction was reached and hence a software
- breakpoint was hit. */
- write_pc_pid (breakpoint_pc, ecs->ptid);
- else if (software_breakpoint_inserted_here_p (breakpoint_pc))
- /* The inferior was free running (i.e., no single-step
- breakpoints inserted) and it hit a software breakpoint. */
+ /* Check whether there actually is a software breakpoint inserted
+ at that location. */
+ if (software_breakpoint_inserted_here_p (breakpoint_pc))
+ {
+ /* When using hardware single-step, a SIGTRAP is reported for both
+ a completed single-step and a software breakpoint. Need to
+ differentiate between the two, as the latter needs adjusting
+ but the former does not.
+
+ The SIGTRAP can be due to a completed hardware single-step only if
+ - we didn't insert software single-step breakpoints
+ - the thread to be examined is still the current thread
+ - this thread is currently being stepped
+
+ If any of these events did not occur, we must have stopped
+ due to hitting a the software breakpoint, and have to back
+ up the breakpoint address.
+
+ As a special case, we could have hardware single-stepped a
+ a software breakpoint. In this case (prev_pc == breakpoint_pc),
+ we also need to back up to the breakpoint address. */
+
+ if (singlestep_breakpoints_inserted_p
+ || !ptid_equal (ecs->ptid, inferior_ptid)
+ || !currently_stepping (ecs)
+ || prev_pc == breakpoint_pc)
write_pc_pid (breakpoint_pc, ecs->ptid);
}
- else
- {
- /* When using hardware single-step, a SIGTRAP is reported for
- both a completed single-step and a software breakpoint. Need
- to differentiate between the two as the latter needs
- adjusting but the former does not.
-
- When the thread to be examined does not match the current thread
- context we can't use currently_stepping, so assume no
- single-stepping in this case. */
- if (ptid_equal (ecs->ptid, inferior_ptid) && currently_stepping (ecs))
- {
- if (prev_pc == breakpoint_pc
- && software_breakpoint_inserted_here_p (breakpoint_pc))
- /* Hardware single-stepped a software breakpoint (as
- occures when the inferior is resumed with PC pointing
- at not-yet-hit software breakpoint). Since the
- breakpoint really is executed, the inferior needs to be
- backed up to the breakpoint address. */
- write_pc_pid (breakpoint_pc, ecs->ptid);
- }
- else
- {
- if (software_breakpoint_inserted_here_p (breakpoint_pc))
- /* The inferior was free running (i.e., no hardware
- single-step and no possibility of a false SIGTRAP) and
- hit a software breakpoint. */
- write_pc_pid (breakpoint_pc, ecs->ptid);
- }
- }
}

/* Given an execution control state that has been freshly filled in
@@ -1372,7 +1353,7 @@ handle_inferior_event (struct execution_
(LONGEST) ecs->ws.value.integer));
gdb_flush (gdb_stdout);
target_mourn_inferior ();
- singlestep_breakpoints_inserted_p = 0; /* SOFTWARE_SINGLE_STEP_P() */
+ singlestep_breakpoints_inserted_p = 0;
stop_print_frame = 0;
stop_stepping (ecs);
return;
@@ -1392,7 +1373,7 @@ handle_inferior_event (struct execution_
target_mourn_inferior ();

print_stop_reason (SIGNAL_EXITED, stop_signal);
- singlestep_breakpoints_inserted_p = 0; /* SOFTWARE_SINGLE_STEP_P() */
+ singlestep_breakpoints_inserted_p = 0;
stop_stepping (ecs);
return;

@@ -1548,8 +1529,7 @@ handle_inferior_event (struct execution_

if (stepping_past_singlestep_breakpoint)
{
- gdb_assert (SOFTWARE_SINGLE_STEP_P ()
- && singlestep_breakpoints_inserted_p);
+ gdb_assert (singlestep_breakpoints_inserted_p);
gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));

@@ -1598,7 +1578,7 @@ handle_inferior_event (struct execution_
if (!breakpoint_thread_match (stop_pc, ecs->ptid))
thread_hop_needed = 1;
}
- else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ else if (singlestep_breakpoints_inserted_p)
{
/* We have not context switched yet, so this should be true
no matter which thread hit the singlestep breakpoint. */
@@ -1669,7 +1649,7 @@ handle_inferior_event (struct execution_
/* Saw a breakpoint, but it was hit by the wrong thread.
Just continue. */

- if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ if (singlestep_breakpoints_inserted_p)
{
/* Pull the single step breakpoints out of the target. */
remove_single_step_breakpoints ();
@@ -1718,7 +1698,7 @@ handle_inferior_event (struct execution_
return;
}
}
- else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ else if (singlestep_breakpoints_inserted_p)
{
sw_single_step_trap_p = 1;
ecs->random_signal = 0;
@@ -1740,7 +1720,7 @@ handle_inferior_event (struct execution_
deprecated_context_hook (pid_to_thread_id (ecs->ptid));
}

- if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ if (singlestep_breakpoints_inserted_p)
{
/* Pull the single step breakpoints out of the target. */
remove_single_step_breakpoints ();