Okay, first try. Sorry for the delay...
On Tuesday 19 May 2009 18:28:35, Ross Morley wrote:
+/* Returns non-zero if we were stopped by a trap or break instruction,
+ whether or not it is a software break planted by GDB. If size != 0,
+ sets *size to that of the instruction or 0 if it need not be skipped. */
+
+#ifndef STOPPED_BY_TRAP_INSTRUCTION
+#define STOPPED_BY_TRAP_INSTRUCTION(size) \
+ (*current_target.to_stopped_by_trap_instruction) (size)
+#endif
#define target_get_ada_task_ptid(lwp, tid) \
(*current_target.to_get_ada_task_ptid) (lwp,tid)
Please rename this to target_stopped_by_trap_instruction, and
make it unconditionally defined. We don't support overriding
of these target macros anymore.
+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
+ {
+ struct thread_info *tp = inferior_thread ();
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+ && count > 0)
Sorry, `read_pc' is now gone from the sources. Replace that by
regcache_read_pc (get_current_regcache ()), or get_frame_pc if there's a
frame handy.
+ /* "stepi" off program breakpoint: the first step is to just increment
+ the PC past the break, then there are count-1 steps to go.
+ Note proceed() won't be called the first time, and on subsequent
+ steps the PC will already be off the break, so the entire handling
+ of "stepi" off a program breakpoint is done here. If stopping after
+ the break, display location information as for normal_stop. */
+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
+ {
+ struct thread_info *tp = inferior_thread ();
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+ && count > 0)
+ {
+ count--;
+ write_pc (read_pc () + tp->program_breakpoint_size);
+ if (count == 0)
+ {
+ reinit_frame_cache ();
+ if (ui_out_is_mi_like_p (uiout))
+ {
+ ui_out_field_string
+ (uiout, "reason",
+ async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE));
+ ui_out_field_int (uiout, "thread-id",
+ pid_to_thread_id (inferior_ptid));
+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
+ }
+ else
+ print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE);
+ }
+ }
+ }
+
This is broken for the !single_inst cases (step/next). step N is mishandled.
It also doesn't take into account that there may be a
breakpoint at ORIG_PC + program_breakpoint_size.
What if a "program breakpoint" is reported for a GDB breakpoint? You shouldn't
just move the PC in that case. I don't see where that is handled. See comment
on next hunk. I suspect this short circuit should be done elsewhere, somewhere
where it will be possible to pass through handle_inferior_event after
moving the PC forward.
Shouldn't you do something in prepare_to_proceed as well?
Use case is:
<thread 1 hit program breakpoint>
(gdb) thread 2
(gdb) continue
GDB is expected to make progress here, instead of reporting
the same program breakpoint again.
+ /* Handle case of hitting a program breakpoint (break instruction
+ in target program, not planted by or known to GDB). */
+
+ if (ecs->event_thread->program_breakpoint_hit)
+ {
+ stop_print_frame = 1;
+
+ /* We are about to nuke the step_resume_breakpoint and
+ through_sigtramp_breakpoint via the cleanup chain, so
+ no need to worry about it here. */
+
+ stop_stepping (ecs);
+ return;
+ }
+
/* Handle cases caused by hitting a breakpoint. */
{
Hmmm, confused: can't the target report a program breakpoint hit
for a PC where there's a gdb breakpoint inserted as well? Wouldn't
it better to have program breakpoints checked from inside
stop_bpstat/bpstop_status/bpstat_explains_signal/bpstat_print?
Then you wouldn't need to add
those if (program_breakpoint) do_this; else handle bpstat;
+static int
+linux_stopped_by_trap_instruction (int *size)
+{
+ CORE_ADDR stop_pc = get_stop_pc();
+ int trap_size = 0;
+
+ if (the_low_target.trap_size_at != NULL)
+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
+ else if (the_low_target.breakpoint_at != NULL
+ && (*the_low_target.breakpoint_at) (stop_pc))
+ trap_size = the_low_target.breakpoint_len;
+
+ if (trap_size != 0)
+ {
+ *size = (the_low_target.get_pc != NULL
+ && (*the_low_target.get_pc) () == stop_pc)
+ ? trap_size : 0;
I think I got this bit, but it would be nice to have a
comment here.
+ return 1;
+ }
+ else
+ return 0;
+}
Before we stick with this forever, it would be very
important to have tests covering corner cases like:
single-stepping through consecutive
"program breakpoint"->gdb breakpoint
single-stepping through consecutive
"program breakpoint"->"program breakpoint"
single- stepping through consecutive
gdb breakpoint->"program breakpoint"
(step, stepi, stepi n)
insn1
"program breakpoint"
foo: gdb breakpoint
insn2
insn3
jmp foo
"step" over this:
function ()
line 1:
insn1
"program breakpoint"
insn2
insn3
line 2:
...
... and have "program breakpoint" be reported, instead
of end-stepping-range at line 2.
From the looks of it, you don't have to do much, if anything
else at all, to have these cases covered on x86; I'd really
like to make sure this is sane on decr_pc_after_break archs, as
this is the way to fix the "step over range with trap
doesn't stop at trap" "bug".
If I read the code correctly, on x86, if you continue
this:
0001 insn1
0002 "program breakpoint"
... you'll report a "program breakpoint" hit at 0003, with
program breakpoint size 0. If you instead stepi through that same
sequence, you'll get a "program breakpoint" hit at 0002, with
program breakpoint size 1. That sounds good, but, if there's a
GDB breakpoint installed at "0002", gdb shouldn't be moving the PC
forward magically, without executing the instruction that
was under the breakpoint originally.
So, if I get this correctly, adjust_pc_after_break should
never try to roll back if program size > 0, even on
decr_pc_after_break archs, but we don't need to check this,
because this will only happen if single-stepping, and it
already does nothing in that case. Right?