From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72143 invoked by alias); 25 Jan 2016 12:17:31 -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 72126 invoked by uid 89); 25 Jan 2016 12:17:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=abandon, 3k, H*r:sk:84-10-2, bounty X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 25 Jan 2016 12:17:28 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id A460B3FE85; Mon, 25 Jan 2016 13:18:10 +0100 (CET) Received: from [192.168.1.62] (84-10-2-59.static.chello.pl [84.10.2.59]) by hogfather.0x04.net (Postfix) with ESMTPSA id 9399358008E; Mon, 25 Jan 2016 13:17:25 +0100 (CET) Subject: Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops. To: Pedro Alves , gdb-patches@sourceware.org References: <1453577516-19252-1-git-send-email-koriakin@0x04.net> <56A60CD3.6060704@redhat.com> From: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= Message-ID: <56A61255.8020503@0x04.net> Date: Mon, 25 Jan 2016 12:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56A60CD3.6060704@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00616.txt.bz2 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