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

[Ross Morley <ross@tensilica.com>]

Pedro Alves wrote:

>On Tuesday 19 May 2009 20:52:01, Ross Morley wrote:
>  
>
>>First, the intention is that a program breakpoint is not
>>reported in the case of hitting a GDB breakpoint. 
>>    
>>
>
>You mean, reported to the user, right? 
>
No, I mean reported to the workings of infrun (high level
of GDB). The target interface reports a trap which could
be a GDB breakpoint. But handle_inferior_event distinguishes
a program breakpoint from a GDB one then sets a flag in the
thread structure if it's a program breakpoint. This chunk in
infrun.c:

 /* The PTID we'll do a target_wait on.*/
@@ -2937,6 +2944,13 @@
       || stop_soon == STOP_QUIETLY || stop_soon == STOP_QUIETLY_NO_SIGSTOP
       || stop_soon == STOP_QUIETLY_REMOTE)
     {
+      /* If we hit a trap not inserted by GDB, it must be in the 
program. */
+      ecs->event_thread->program_breakpoint_hit
+    = (STOPPED_BY_TRAP_INSTRUCTION 
(&ecs->event_thread->program_breakpoint_size)
+        && !software_breakpoint_inserted_here_p (stop_pc));
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+      if (debug_infrun && ecs->event_thread->program_breakpoint_hit)
+    fprintf_unfiltered (gdb_stdlog, "infrun: program breakpoint hit\n");
+

So a program breakpoint is deemed hit if a trap was hit and
there was not a software breakpoint inserted there. The idea
is to avoid interfering with normal breakpoint handling and
only handle cases where it's a program breakpoint.

> I agree.  However,
>the target will report to gdb a program breakpoint for
>gdb breakpoints as well:
>
>+  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;
>
>Only way to tell the difference is if there's a breakpoint
>at PC in gdb's tables or not.
>  
>
Exactly.

>Hmm, isn't there an off-by-one error in the XCHAL_HAVE_BE
>case here? :
>
>+#if XCHAL_HAVE_BE
>+    if (insn[1] == 0xd2 && (insn[2] & 0x0f) == 0x0f)
>+#else
>+    if (insn[0] == 0x2d && (insn[1] & 0xf0) == 0xf0)
>+#endif
>+      /* break.n s (density) */
>
>  
>
You are right. There's also a byte reversal in the big endian
case of the 3 byte break. I did gdbserver in a hurry just to
test with the FSF source tree since our toolchain doesn't have
the top of trunk changes yet so I couldn't use our simulator.
I used a little endian configuration and didn't test big endian.
I will test this on big endian before submitting a final patch.

>  
>
>>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 think you can do something reasonably generic enough by
>passing "additional_flags=-DNOP=mysmallnopinsn -DBREAK_INSN1=foo -DBREAK_INSN2=bar"
>to gdb_compile, and write the test in c with embedded asm, as:
>
>int foo ()
>{
>    NOP
>    NOP
>    BREAK_INSN1
>jmphere:
>    NOP /* breakpoint here*/
>    NOP
>    BREAK_INSN1
>    NOP
>goto jmphere;
>}
>
>etc, etc.
>
>Then, the .exp file passes the appropriate defines/flags
>based on target triplet.
>
>if { ([istarget "xtensa*-*-*"] } {
>  additional_flags={-DNOP=mysmallnopinsn -DBREAK_INSN1="break 7,15" -DBREAK_INSN2="break.n 7"}
>} else if { ([istarget "fooarch*-*-*"] } {
> ...
>}
>
>  
>
OK, I'll try that.

It will be a day or two before I can look at the other issues you raised.

Thanks,
Ross