From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89477 invoked by alias); 25 Feb 2016 13:08:25 -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 89467 invoked by uid 89); 25 Feb 2016 13:08:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=BAYES_00,SPF_HELO_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=reindent, Hx-languages-length:2809, super, HContent-Transfer-Encoding:8bit X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 25 Feb 2016 13:08:24 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 4745C35A808; Thu, 25 Feb 2016 13:08:23 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1PD8L8D003872; Thu, 25 Feb 2016 08:08:21 -0500 Message-ID: <56CEFCC5.8070109@redhat.com> Date: Thu, 25 Feb 2016 13:08:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: =?UTF-8?B?TWFyY2luIEtvxZtjaWVsbmlja2k=?= , gdb-patches@sourceware.org CC: antoine.tremblay@ericsson.com Subject: Re: [PATCH] [PR gdb/13808] gdb.trace: Pass tdesc selected in gdbserver to IPA. References: <1456089436-1445-1-git-send-email-koriakin@0x04.net> In-Reply-To: <1456089436-1445-1-git-send-email-koriakin@0x04.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-02/txt/msg00781.txt.bz2 On 02/21/2016 09:17 PM, Marcin Kościelnicki wrote: > If gdbserver and IPA are using different tdesc, they will disagree > about 'R' trace packet size. This results in mangled traces. > > To make sure they pick the same tdesc, gdbserver pokes the tdesc > (specified as an index in a target-specific list) into a global > variable in IPA. In theory, IPA could find out the tdesc on its > own, but that may be complex (in particular, I don't know how to > tell whether we have LAST_BREAK on s390 without messing with ptrace), > and we'd have to duplicate the logic. > > Tested on i386 and x86_64. On i386, it fixes two FAILs in ftrace.exp. > On x86_64, these failures have been KFAILed - one of them works now, > but the other now fails due to an unrelated reason (ugh). Thanks. I realized this grows each traceframe's size, probably several times fold on x86, given the size/number of vector registers. It'd be nice to avoid this. - One way would be to actually respect the ax's register masks. - Another way would be to trim recorded regblock sizes up until the last recordable register in the tdesc. So if when reading back the traceframe, the regblock is shorter than expected, just treat the missing registers as unavailable. BTW, do we print the not-actually-collected-in-fast-tracepoint registers as , or as zero's? It'd also be nice to be able to actually collect the now tdesc-accessible registers, but that runs into the register size problems discussed earlier. Plus, we probably wouldn't want to _always_ collect everything. I haven't analyzed the actual traceframe size impact, but I guess we can consider those orthogonal optimization problems, as without this fix, things just don't work. One thing that wasn't super obvious in the patch is why do several different target descriptions in gdbserver map to a single description in the IPA? I noticed several new functions with missing entry comment. > +const struct target_desc * > +get_ipa_tdesc (int idx) > +{ > + switch (idx) { > + case X86_TDESC_MMX: /* Should not happen. */ ... > +const struct target_desc * > +get_ipa_tdesc (int idx) > +{ > + switch (idx) { '{' goes on next line (and then reindent). > @@ -328,6 +332,14 @@ tracepoint_look_up_symbols (void) > } > } > > + /* Tell IPA about the correct tdesc. */ > + if (write_inferior_integer (ipa_sym_addrs.addr_ipa_tdesc_idx, > + target_get_ipa_tdesc_idx ())) > + { > + internal_error (__FILE__, __LINE__, > + "Error setting ipa_tdesc_idx variable in lib"); > + } Failure to write to the inferior should never be an internal error. The inferior might vanish, e.g., because it was SIGKILL'ed from outside gdbserver. > + > agent_look_up_symbols (NULL); > } > Thanks, Pedro Alves