Re: [PATCH 4/5 v4] Tracepoint for ppc64.

[Pierre Langlois <pierre dot langlois at arm dot com>]

On 27/06/15 17:21, Wei-cheng Wang wrote:
> Ulrich Weigand wrote:
>> Wei-cheng Wang wrote:
>>> Ulrich Weigand wrote:
>>>> Wei-cheng Wang wrote:
>>>>> +static int
>>>>> +ppc_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
>>>>> +                        CORE_ADDR addr, int *isize, char **msg)
>>>>> +{
>>>>> +  if (isize)
>>>>> +    *isize = gdbarch_max_insn_length (gdbarch);
>>>>> +  if (msg)
>>>>> +    *msg = NULL;
>>>>> +  return 1;
>>>>> +}
>>>>
>>>> Should/can we check here where the jump to the jump pad will be in
>>>> range?  Might be better to detect this early ...
>>>
>>> Client has no idea about where the jump pad will be installed.
>>> If it's out of range, gdbserver will report it right after user
>>> entered 'tstart' command
>> Well, but we know the logic the stub uses.  For example, we know that
>> we certainly cannot install a fast tracepoint in any shared library code,
>> since the jump pad will definitely be too far away.  We can check for
>> this condition here.  (We could also check for tracepoints in executables
>> that have a text section larger than 32 MB ...)
> 
> Now in this path, ppc_fast_tracepoint_valid_at will check the distance
> and return 0 (invalid) if ADDR is too far from jumppad.
> 
> However, if a tracepoint was pending and later found it's not valid,
> it will cause an internal-error.  See remote.c
> 
>   if (gdbarch_fast_tracepoint_valid_at (target_gdbarch (),
>                                         tpaddr, &isize, NULL))
>     xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x",
>                isize);
>   else
>     /* If it passed validation at definition but fails now,
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        something is very wrong.  */
>     internal_error (__FILE__, __LINE__, _("Fast tracepoint not "
>                                           "valid during download"));
> 
> If the tracepoint is pending at definition, it won't be checked at all.
> 
>   /* Fast tracepoints may have additional restrictions on location.  */
>   if (!pending && type_wanted == bp_fast_tracepoint)
>       ^^^^^^^^
>     {
>       for (ix = 0; VEC_iterate (linespec_sals, canonical.sals, ix, iter); ++ix)
>         check_fast_tracepoint_sals (gdbarch, &iter->sals);
>     }
> 
> Maybe we use "error" instead of "internal_error"?

Hi Wei-cheng,

Is it OK for GDB to guess where the jump pad is based assumptions on how/where
GDBServer installs it?  I'm thinking one could implement another stub supporting
fast tracepoints differently (?).

Maybe we could extend/add a tracepoint packet in order for GDB to ask the stub
what the jump pad address is.  Or change the fast_tracepoint_valid_at gdbarch
method into a target method, leaving GDBServer to decide if a fast tracepoint is
valid early on.

Any thoughts?

Thanks,
Pierre

> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index d947879..7924227 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -83,6 +83,9 @@
>  #include "features/rs6000/powerpc-e500.c"
>  #include "features/rs6000/rs6000.c"
> 
> +#include "ax.h"
> +#include "ax-gdb.h"
> +
>  /* Determine if regnum is an SPE pseudo-register.  */
>  #define IS_SPE_PSEUDOREG(tdep, regnum) ((tdep)->ppc_ev0_regnum >= 0 \
>      && (regnum) >= (tdep)->ppc_ev0_regnum \
> @@ -966,6 +969,53 @@ rs6000_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *bp_addr,
>      return little_breakpoint;
>  }
> 
> +/* Return true if ADDR is a valid address for tracepoint.  Set *ISZIE
> +   to the number of bytes the target should copy elsewhere for the
> +   tracepoint.  */
> +
> +static int
> +ppc_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
> +                             CORE_ADDR addr, int *isize, char **msg)
> +{
> +  CORE_ADDR base, pagesz;
> +  const int SCRATCH_BUFFER_NPAGES = 20;
> +  int isValid = 1;
> +
> +  if (isize)
> +    *isize = gdbarch_max_insn_length (gdbarch);
> +
> +  /* If we can figure out where is the jump-pad, check whether
> +     the address for tracepoint is too far away.  Otherwise,
> +     assume it is valid.  */
> +  if (target_auxv_search (&current_target, AT_PHDR, &base) > 0
> +      && target_auxv_search (&current_target, AT_PAGESZ, &pagesz) > 0)
> +    {
> +      /* The jump-pad is supposed to be mapped here.
> +        See gdbserver/linux-ppc-ipa.c and gdbserver/tracepoint.c.  */
> +      long dist;
> +      CORE_ADDR jpad_base
> +       = (base & ~(pagesz - 1)) - SCRATCH_BUFFER_NPAGES * pagesz;
> +
> +      dist = jpad_base - addr;
> +      if (dist >= (1 << 25) || dist < -(1 << 25))
> +       isValid = 0;
> +    }
> +
> +  if (isValid)
> +    {
> +      if (msg)
> +       *msg = NULL;
> +    }
> +  else
> +    {
> +      if (msg)
> +       *msg = xstrdup (_("The address is too far for "
> +                         "inserting fast tracepoint."));
> +    }
> +
> +  return isValid;
> +}
> +
>  /* Instruction masks for displaced stepping.  */
>  #define BRANCH_MASK 0xfc000000
>  #define BP_MASK 0xFC0007FE