[PATCH] gdb.trace: Remove struct tracepoint_action_ops.
Marcin Kościelnicki
koriakin@0x04.net
Mon Feb 8 11:55:00 GMT 2016
>
>
>
> 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.
More information about the Gdb-patches
mailing list