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

["Marcin Kościelnicki" <koriakin@0x04.net>]

On 16/03/16 17:58, Ulrich Weigand wrote:
> 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.

I'll try to do that.  Seems like a bit of work, but it's only called 
once per installed pad / emitted expression, so should be OK.
>
>> 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).

Alright, I'll do that (and throw it in for aarch64 as well, code should 
be near-identical).
>
>> +/* 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?

Not really, I just happened to had this bit written before I learned of 
Wei-cheng's patches.  What are your preferences here?

>
>> +/* 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 :-)

I'll change that.

>
>> +#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
>