This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Fwd: Re: [PATCH] Program Breakpoints]


Thanks Pedro. This is the kind of feedback I was looking for
in regard to effects on threads and other archs. I will have
to go through your comments and think about those scenarios
you mentioned with stepping. Before I do that I do want to
clarify a couple of things for you.

First, the intention is that a program breakpoint is not
reported in the case of hitting a GDB breakpoint. That is
because you hit the inserted GDB breakpoint. You will execute
the program breakpoint at the same location when you remove
the GDB breakpoint to step over it. You cannot hit both at
the same time. I think this corner case (GDB breakpoint over
program breakpoint) will be handled as for other stop reasons
that occur while stepping over a GDB breakpoint, but I will
look more closely at this.

Test cases seem impossible to write completely generically
because the test needs a target-specific break instruction.
We have one (not in this patch) I could submit for Xtensa.
It places 2 sizes of program breakpoints for Xtensa as follows:
   __asm__("break 7,15");
   __asm__("break.n 7");
Is there a way to write a generic test with a program break?

I will go through your feedback thouroughly in the next couple
of days.

Thanks,
Ross


Pedro Alves wrote:


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?





Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]