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

[Pedro Alves <pedro at codesourcery dot com>]

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?

-- 
Pedro Alves