Re: [PATCH 1/2] Fast tracepoint for powerpc64le

[Pedro Alves <palves@redhat.com>]

On 02/27/2015 07:52 PM, Ulrich Weigand wrote:

>> The main reason why PowerPC64 big-endian doesn't work is
>> calling convention (function descriptors) issue.
>>    When installing a tracepoint in inferior memory, gdbserver
>> asks the address of "gdb_collect" (and etc.) using qSymbol packet,
>> and it generate a sequence of instructions to calling that address.
>>    However, gdb-client "return the start of code instead of
>> any data function descriptor."
>>    See commenting in remote_check_symbols/remote.c,
>> https://sourceware.org/ml/gdb-patches/2007-06/msg00389.html
>> and gen_call() in this patch.
>>    In order for powerpc64be to work, qSymbol packet should be
>> extend for function descriptors.

> So I guess there's two ways to fix this.   One would be to change
> gdbserver to work more like GDB here.  This would involve removing
> the descriptor->code address conversion in remote.c, and instead
> performing the conversion in gdbserver's thread_db_enable_reporting.
> Now, there is no gdbarch_convert_from_func_ptr_addr in gdbserver,
> so a similar mechanism would have to be invented there.  (I guess
> this would mean a new target hook.)  Fortunately, the only platform
> that uses function descriptors *and* supports libthread_db debugging
> in gdbserver is ppc64-linux, so we'd only have to add that new
> mechanim on this platform.

Note sure about this one, ppc64_convert_from_func_ptr_addr wants to
get at the bfd/binary's unrelocated sections.  We'd have to teach
gdbserver to read the binary.

> 
> This has the advantage that qSymbol could now be used to lookup
> function symbols and get the descriptor address as expected.
> On the other hand, this would mean an incompatible change in the
> remote protocol: if you used a new GDB together with an old
> gdbserver (or vice versa), thread debugging would stop working.
> However, I guess that could be fixed by having gdbserver request
> the new behavior from GDB by specifying a feature code.  With old
> GDBs gdbserver would have to skip the descriptor->code conversion.
> 
> 
> The second alternative would be to extend qSymbol to support
> returning two different types of addresses for function symbols:
> the symbol value (i.e. function pointer value, i.e. descriptor
> on PPC64), and a code address suitable to set a breakpoint on
> function entry.  This could be either by having gdbserver
> request one or the other via an additional flag on the qSymbol
> request, or else by GDB simply always returning both values
> in two fields.  Again, this would be an incompatible protocol
> change that would need to be guarded by a qFeature check.
> 
> In this case, gdbserver would use the "normal" symbol values
> for most purposes (e.g. tracepoint routine lookup), but would
> use the "code address" values to return from ps_pglobal_lookup.
> With old GDBs, gdbserver would disable fast tracepoint support
> on powerpc64.
> 
> If we do that, then for consistency it might also be useful
> to pass code addresses to ps_pglobal_lookup in GDB itself.
> In addition, the "code address" could be changed to skip
> the local entry point prolog on powerpc64le to ensure that
> the breakpoint is set correctly.  (This does not matter in
> practice since __nptl_create_event has no local entry point,
> but it would seem more fully correct in general.)
> 
> 
> Overall, the second alternative seems a bit better to me,
> but I'd certainly appreciate feedback on this ...

I inclined for the second alternative as well.

(Note for testing: __nptl_create_event will only be used
on old kernels without PTRACE_EVENT_CLONE, unless you hack the
code to force usage.)

> 
> 
>> For powerpc32 to work, some data structure/function in tracepoint.c
>> need to be fixed.  For example,
>>
>> * write_inferior_data_ptr should be fixed for big-endian.
>>    If sizeof (CORE_ADDR) is larger than sizeof (void*), zeros are written.
>>    BTW, I thnink write_inferior_data_pointer provides the same functionality
>>    without this issue.  I'm not sure why write_inferior_data_ptr is needed?
> 
> This is odd, I don't see the point of this either.

Yeah, probably I just missed merging them fully while cleaning up the
initial contribution...  I agree we should just use
write_inferior_data_pointer.

>> * Data structure layout between gdbserver and IPA is not consistent.
>>
>>    There are two versions of tracepoint_action one for gdbserver,
>>    and antoher for inferior (IPA side).
>>

...

> Ugh.  That's a strange construct, and extremely dependent on alignment
> rules (as you noticed).  I'm not really sure what the best way to fix
> this would be.  My preference right now would be get rid of "ops" on
> the gdbserver side too, and just switch on "type" in the two places
> where the ops->send and ops->download routines are called right now.
> 
> This makes the data structures the same on gdbserver and IPA, which
> simplifies downloading quite a bit.  (Also, it keeps the data structure
> identical in IPA, which should avoid compatibility issues between
> versions.)

That sounds fine.

Thanks,
Pedro Alves