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

[Ross Morley <ross at tensilica dot 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