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

[Ross Morley <ross@tensilica.com>]

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?
>
>  
>