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

[Marcin KoÅcielnicki <koriakin at 0x04 dot 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.