Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Pedro Alves <palves at redhat dot com>]

On 01/23/2016 07:31 PM, Marcin KoÅcielnicki wrote:
> The struct tracepoint_action has an ops field, pointing to
> a tracepoint_action_ops structure, containing send and download ops.
> However, this field is only present when compiled in gdbserver, and not
> when compiled in IPA.  When gdbserver is downloading tracepoint actions
> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
> its struct tracepoint_action, to get to the part that corresponds to
> IPA's struct tracepoint_action.
> 
> Unfortunately, this fails badly on ILP32 platforms where alignof(long long)
> == 8.  Consider struct collect_memory_action layout in gdbserver:
> 
> 0-3: base.ops
> 4: base.type
> 8-15: addr
> 16-23: len
> 24-27: basereg
> sizeof == 32
> 
> and its layout in IPA:
> 
> 0: base.type
> 8-15: addr
> 16-23: len
> 24-27: basereg
> sizeof == 32
> 
> When gdbserver tries to download it to IPA, it skips the first 4 bytes
> (base.ops), figuring the rest will match what IPA expects - which is
> not true, since addr is aligned to 8 bytes and will be at a different
> relative position to base.type.
> 
> The problem went unnoticed on the currently supported platforms, since
> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
> alignment for long long.
> 
> There are a few possible ways around this problem.  I decided on removing
> ops altogether, since they can be easily inlined in their (only) places
> of use - in fact allowing us share the code between 'L' and 'R'.  Any
> approach where struct tracepoint_action is different between IPA and
> gdbserver is just asking for trouble.
> 
> Found on s390.  Tested on x86_64, s390, s390x.

Hmm, this is essentially the same as:

 https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html

Right?

Seems that other patch inlines things a bit less though, which offhand
looks preferable.  WDYT?

Not sure what happened to that series.  I thought most of it (if not all)
had been approved already.

Thanks,
Pedro Alves