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

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

>
>
>
> On 01/25/2016 07:17 AM, Marcin Kościelnicki wrote: 
> > On 25/01/16 12:53, Pedro Alves wrote: 
> >> 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 
> >> 
> > 
> > Huh, I didn't know about that patch series.  Good to know, since I was 
> > going to try doing ppc tracepoints next, and had no idea that has 
> > already been attempted.  What happened to that patchset/author?  Kind of 
> > strange to abandon mostly-finished code when there's a $3k bounty on it. 
> > 
> > The other patch looks fine too, I have no preference here. 
> > 
> > Marcin Kościelnicki 
>
> I had the same problem on ARM. 
>
> We could just keep the struct and pack it too, this is common for ABIs 
> IMO... 
>
> It would avoid this kind of mistake in the future if we were going to 
> reintroduce something similar... 
>
> I had this patch in the pipeline: 
>
> Author: Antoine Tremblay <antoine.tremblay@ericsson.com> 
> Date:   Thu Jan 28 13:03:10 2016 -0500 
>
>      Fix structure alignment problems with IPA protocol objects. 
>
>      While testing fast tracepoints on ARM I came across this problem : 
>
>      Program received signal SIGSEGV, Segmentation fault. 
>      0x4010b56c in ?? () from target:/lib/arm-linux-gnueabihf/libc.so.6 
>      (gdb) FAIL: gdb.trace/ftrace.exp: ond globvar > 7: continue 
>
>      The problem is that on GDBServer side struct tracepoint_action is 
> aligned 
>      on 4 bytes, and collect_memory_action is aligned on 8. However in 
> the IPA 
>      they are both aligned on 8 bytes. 
>
>      Thus when we copy the data from GDBServer's struct 
> tracepoint_action->type 
>      offset to the ipa the alignement is wrong and the addr,len,basereg 
> values 
>      are wrong also, causing a crash in the inferiror as it tries to read 
>      memory at a bogus address. 
>
>      This patch fixes this issue by setting the tracepoint_action and 
>      collect_memory_action as packed. 
>
>      This patch also fixes this issue in general by setting all IPA protocol 
>      object structures as packed. 
>
>      gdb/gdbserver/ChangeLog: 
>
>      * tracepoint.c (ATTR_PACKED): Moved earlier in the file. 
>      (struct tracepoint_action): Use ATTR_PACKED. 
>      (struct collect_memory_action): Likewise. 
>      (struct collect_registers_action): Likewise. 
>      (struct eval_expr_action): Likewise. 
>      (struct collect_static_trace_data_action): Likewise. 
>
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c 
> index 33f6120..35ce2ad 100644 
> --- a/gdb/gdbserver/tracepoint.c 
> +++ b/gdb/gdbserver/tracepoint.c 
> @@ -467,6 +467,14 @@ static int write_inferior_data_ptr (CORE_ADDR 
> where, CORE_ADDR ptr); 
>
>   #endif 
>
> +#ifndef ATTR_PACKED 
> +#  if defined(__GNUC__) 
> +#    define ATTR_PACKED __attribute__ ((packed)) 
> +#  else 
> +#    define ATTR_PACKED /* nothing */ 
> +#  endif 
> +#endif 
> + 
>   /* Operations on various types of tracepoint actions.  */ 
>
>   struct tracepoint_action; 
> @@ -490,7 +498,7 @@ struct tracepoint_action 
>     const struct tracepoint_action_ops *ops; 
>   #endif 
>     char type; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'M' (collect memory) action.  */ 
>   struct collect_memory_action 
> @@ -500,14 +508,14 @@ struct collect_memory_action 
>     ULONGEST addr; 
>     ULONGEST len; 
>     int32_t basereg; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'R' (collect registers) action.  */ 
>
>   struct collect_registers_action 
>   { 
>     struct tracepoint_action base; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'X' (evaluate expression) action.  */ 
>
> @@ -516,13 +524,13 @@ struct eval_expr_action 
>     struct tracepoint_action base; 
>
>     struct agent_expr *expr; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'L' (collect static trace data) action.  */ 
>   struct collect_static_trace_data_action 
>   { 
>     struct tracepoint_action base; 
> -}; 
> +} ATTR_PACKED; 
>
>   #ifndef IN_PROCESS_AGENT 
>   static CORE_ADDR 
> @@ -828,14 +836,6 @@ IP_AGENT_EXPORT_VAR struct trace_state_variable 
> *trace_state_variables; 
>      when the wrapped-around trace frame is the one being discarded; the 
>      free space ends up in two parts at opposite ends of the buffer.  */ 
>
> -#ifndef ATTR_PACKED 
> -#  if defined(__GNUC__) 
> -#    define ATTR_PACKED __attribute__ ((packed)) 
> -#  else 
> -#    define ATTR_PACKED /* nothing */ 
> -#  endif 
> -#endif 
> - 
>   /* The data collected at a tracepoint hit.  This object should be as 
>      small as possible, since there may be a great many of them.  We do 
>      not need to keep a frame number, because they are all sequential 
>
>
> WDYT ? 
>
> Thanks, 
> Antoine 
>
>

Well, that'd work too, but I still think having a struct with different layout between ipa and gdbserver is unnecessary and asking for trouble.  We should be consistent - if the encoding paths go through the ops structure, why does the actual execution of actions use a switch?  Removing the ops, by Wei-cheng's patch or mine, simplifies the code and makes it consistent with the switch-using paths.