Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

On 09/23/2015 06:51 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> > +static const unsigned char *
> > +aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
>
> Please add comment like this to all these $ARCH_breakpoint_from_pc functions.
>
> /* Implementation of linux_target_ops method "breakpoint_from_pc".  */

Done

> > diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
> > index e0bfa1a..da5876d 100644
> > --- a/gdb/gdbserver/linux-cris-low.c
> > +++ b/gdb/gdbserver/linux-cris-low.c
> > @@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno)
> >   extern int debug_threads;
> >
> >   static CORE_ADDR
> > -cris_get_pc (struct regcache *regcache, void)
> > +cris_get_pc (struct regcache *regcache)
> >   {
> >     unsigned long pc;
> >     collect_register_by_name (regcache, "pc", &pc);
>
> This is a unrelated change.  Please move it out this patch, and submit
> it separately.

Done

> > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> > index f5b64ab..bb08761 100644
> > --- a/gdb/gdbserver/linux-low.c
> > +++ b/gdb/gdbserver/linux-low.c
> > @@ -3012,7 +3012,11 @@ linux_wait_1 (ptid_t ptid,
> >     if (!ptid_equal (step_over_bkpt, null_ptid)
> >         && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
> >       {
> > -      unsigned int increment_pc = the_low_target.breakpoint_len;
> > +      int increment_pc = 0;
> > +      CORE_ADDR stop_pc = event_child->stop_pc;
> > +
> > +      (*the_low_target.breakpoint_from_pc)
> > +	(&stop_pc, &increment_pc);
>
> There was a problem here and your patch doesn't fix it.  I want to raise
> it here first.  It is incorrect to get increment_pc by
> the_low_target.breakpoint_len or (*the_low_target.breakpoint_from_pc)
> for arm/thumb, because given the stop_pc, we can't tell the breakpoint
> size (2-byte or 4-byte).We need a new target hook, say
> breakpoint_from_current_state, and its default implementation is
> breakpoint_from_pc.  However, its arm implementation checks whether
> the program is in thumb mode by CPSR and return the right breakpoint size.
>
> IIUC, the code here is used for step-over GDBserver breakpoint, so it is
> not used for arm target until we support conditional breakpoint or
> tracepoint, but we should fix it before supporting conditional
> breakpoint and tracepoint for arm target.

Indeed good point. It's too bad another target hook needs to be there 
for this but there's no choice I think.

I will fix it in my next patchset introducing conditional breakpoints.

> >         if (debug_threads)
> >   	{
> > @@ -6932,6 +6936,17 @@ current_lwp_ptid (void)
> >     return ptid_of (current_thread);
> >   }
> >
> > +const unsigned char *
> > +linux_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
> > +{
> > +  if (the_low_target.breakpoint_from_pc != NULL)
> > +    {
> > +      return (*the_low_target.breakpoint_from_pc) (pcptr, lenptr);
> > +    }
>
> "{" and "}" is not needed.

Done

> > diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
> > index f8f6e78..c623150 100644
> > --- a/gdb/gdbserver/linux-low.h
> > +++ b/gdb/gdbserver/linux-low.h
> > @@ -141,8 +141,13 @@ struct linux_target_ops
> >
> >     CORE_ADDR (*get_pc) (struct regcache *regcache);
> >     void (*set_pc) (struct regcache *regcache, CORE_ADDR newpc);
> > -  const unsigned char *breakpoint;
> > -  int breakpoint_len;
> > +
> > +  /* Return the raw breakpoint for this target based on PC.  Note that the PC
>
> s/PC/*PCPTR/
>
> > +     can be NULL, the default breakpoint for the target should be returned in
>
> PC can't be NULL after your patch #2.  You can remove the second
> sentence in this patch or patch #2.
I think you mean after patch #3 ?

But it can still be NULL see in #3 :

/* Default if no pc is set to arm breakpoint.  */
+  if (pcptr == NULL)
+    {
+      *lenptr = arm_breakpoint_len;
+      return (unsigned char *) &arm_breakpoint;
+    }

> > +     this case. The PC is ajusted to the real memory location in case a flag was
> > +     present in the PC.  */
> > +  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
> > +

Note also that I removed the win32 changes, they were not needed and 
broken, I can't test win32 properly anyway.