Re: [PATCH 3/4 v2] gdbserver: Add powerpc fast tracepoint support.

["Ulrich Weigand" <uweigand@de.ibm.com>]

Marcin Kościelnicki wrote:

> I'm not sure how to properly determine whether ELFv1 or ELFv2 is in use
> by the target, so I assume it's the same ABI as in gdbserver itself.

That's a generally sensible assumption.  In principle, you can check
which ABI the target is using by checking the e_flags field in the
ELF header.  The problem for gdbserver is that it usually doesn't
access the ELF file.  However, the ELF header is mapped into target
memory, so could be checked there.  Yet again, the *address* of the
ELF header is not very easily detectable without making assumptions
on how memory is layed out.

The best bet would probably be to determine the AT_PHDR auxillary vector,
which gives you the address of the *program headers* for the main
executable in the target, and then round that address down to the page
size boundary.  This should give you the address of the ELF header,
when making the assumption that the linker places program headers
in the fist page, following the ELF header.  This is not necessarily
guaranteed by the ELF format, but is what all linkers happen to do.

> Allocating the jump pad buffer needs a mention.  On powerpc64, IPA simply
> scans from 0x10000000 (the executable load address) downwards until it
> find a free address.  However, on 32-bit, ld.so loads dynamic libraries
> there, and they can easily cause us to exceed the 32MiB branch range -
> so, instead I aim right after the executable, at sbrk(0).  This will
> of course cause further memory allocation via brk to fail, but glibc
> transparently falls back to mmap in this case, so that's not a problem.
> Of course, given a program with large data/bss section, this might
> exceed the branch range as well, so perhaps some better heuristic is
> in order here.

These heuristics seem reasonable to me.

> This fixes get_raw_reg for 32-bit (ULONGEST was used to read the
> registers dumped by jump pad instead of unsigned long).  Also slightly
> optimizes gen_limm and fixes a few comments.

> +alloc_jump_pad_buffer (size_t size)
> +{
> +#ifdef __powerpc64__
> +  uintptr_t addr;
> +  int pagesize;
> +  void *res;
> +
> +  pagesize = sysconf (_SC_PAGE_SIZE);
> +  if (pagesize == -1)
> +    perror_with_name ("sysconf");
> +
> +  addr = 0x10000000 - size;

It also would be preferable to dynamically determine the actual
load address, since 0x10000000 is just the default, which can
be changed e.g. via linker script or due to PIE address randomization.

You might again use the AT_PHDR auxillary vector (which is what
Wei-cheng's patches did).

> +/* Get the thread area address.  This is used to recognize which
> +   thread is which when tracing with the in-process agent library.  We
> +   don't read anything from the address, and treat it as opaque; it's
> +   the address itself that we assume is unique per-thread.  */
> +
> +static int
> +ppc_get_thread_area (int lwpid, CORE_ADDR *addr)
> +{
> +#ifdef __powerpc64__
> +  struct lwp_info *lwp = find_lwp_pid (pid_to_ptid (lwpid));
> +  struct thread_info *thr = get_lwp_thread (lwp);
> +  struct regcache *regcache = get_thread_regcache (thr, 1);
> +  int is_64 = register_size (regcache->tdesc, 0) == 8;
> +#endif
> +  long res;
> +
> +#ifdef __powerpc64__
> +  if (is_64)
> +    res = ptrace (PTRACE_PEEKUSER, lwpid, (long) PT_R13 * sizeof (long),
> +		  (long) 0);
> +  else
> +#endif
> +    res = ptrace (PTRACE_PEEKUSER, lwpid, (long) PT_R2 * sizeof (long),
> +		  (long) 0);

Was there a particular reason why you changed to using direct ptrace
calls instead of collecting the value from the regcache as in the
original patch?

> +/* GEN_LOAD and GEN_STORE generate 64- or 32-bit load/store for ppc64 or ppc32
> +   respectively.  They are primary used for save/restore GPRs in jump-pad,
> +   not used for bytecode compiling.  */
> +
> +#if defined __powerpc64__
> +#define GEN_LOAD(buf, rt, ra, si)	(is_64 ? GEN_LD (buf, rt, ra, si) : \
> +						 GEN_LWZ (buf, rt, ra, si))
> +#define GEN_STORE(buf, rt, ra, si)	(is_64 ? GEN_STD (buf, rt, ra, si) : \
> +						 GEN_STW (buf, rt, ra, si))

Huh, using the is_64 variable from the surrounding routine without
passing it in as macro argument is probably not the best idea :-)

> +#if _CALL_ELF == 2
> +  /* XXX is there a way to get this dynamically from the inferior?  */
> +  int is_opd = 0;

See above.


Otherwise, this looks good to me.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com