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

[Yao Qi <qiyaoltc at gmail dot com>]

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".  */

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

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

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

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

> +     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);
> +

-- 
Yao (éå)