From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83661 invoked by alias); 29 Jan 2016 13:11:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 82836 invoked by uid 89); 29 Jan 2016 13:11:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 spammy=4907, Antoine, 490,7, 498,7 X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 29 Jan 2016 13:11:43 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id 2D.5E.06940.3226BA65; Fri, 29 Jan 2016 13:59:15 +0100 (CET) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.92) with Microsoft SMTP Server id 14.3.248.2; Fri, 29 Jan 2016 08:11:40 -0500 Subject: Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops. To: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= , Pedro Alves , References: <1453577516-19252-1-git-send-email-koriakin@0x04.net> <56A60CD3.6060704@redhat.com> <56A61255.8020503@0x04.net> From: Antoine Tremblay Message-ID: <56AB650B.1000204@ericsson.com> Date: Fri, 29 Jan 2016 13:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <56A61255.8020503@0x04.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00719.txt.bz2 On 01/25/2016 07:17 AM, Marcin Ko=C5=9Bcielnicki wrote: > On 25/01/16 12:53, Pedro Alves wrote: >> On 01/23/2016 07:31 PM, Marcin Ko=C5=9Bcielnicki 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) >>> =3D=3D 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 =3D=3D 32 >>> >>> and its layout in IPA: >>> >>> 0: base.type >>> 8-15: addr >>> 16-23: len >>> 24-27: basereg >>> sizeof =3D=3D 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=C5=9Bcielnicki I had the same problem on ARM. We could just keep the struct and pack it too, this is common for ABIs=20 IMO... It would avoid this kind of mistake in the future if we were going to=20 reintroduce something similar... I had this patch in the pipeline: Author: Antoine Tremblay 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=20 aligned on 4 bytes, and collect_memory_action is aligned on 8. However in=20 the IPA they are both aligned on 8 bytes. Thus when we copy the data from GDBServer's struct=20 tracepoint_action->type offset to the ipa the alignement is wrong and the addr,len,basereg=20 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=20 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=20 *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