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

Marcin Kościelnicki
2016-03-16T17:55:18Z

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
>