From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15895 invoked by alias); 16 Sep 2013 09:01:06 -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 15881 invoked by uid 89); 16 Sep 2013 09:01:05 -0000 Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 16 Sep 2013 09:01:05 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_50,KHOP_THREADED,RDNS_NONE,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mga02.intel.com Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 16 Sep 2013 02:01:01 -0700 X-ExtLoop1: 1 Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by orsmga002.jf.intel.com with ESMTP; 16 Sep 2013 02:00:35 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.69]) by IRSMSX102.ger.corp.intel.com ([169.254.2.234]) with mapi id 14.03.0123.003; Mon, 16 Sep 2013 09:59:39 +0100 From: "Metzger, Markus T" To: Jan Kratochvil CC: "gdb-patches@sourceware.org" , "Himpel, Christian" Subject: RE: [patch v4 03/24] btrace: change branch trace data structure Date: Mon, 16 Sep 2013 09:01:00 -0000 Message-ID: References: <1372842874-28951-1-git-send-email-markus.t.metzger@intel.com> <1372842874-28951-4-git-send-email-markus.t.metzger@intel.com> <20130818190426.GC24153@host2.jankratochvil.net> <20130912200927.GA29475@host2.jankratochvil.net> In-Reply-To: <20130912200927.GA29475@host2.jankratochvil.net> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00429.txt.bz2 > -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Jan Kratochvil Thanks for your feedback. > > > > + /* If we have symbol information for our current location, use > > > > + it to check that we jump to the start of a function. */ > > > > + if (fun !=3D NULL || mfun !=3D NULL) > > > > + start =3D get_pc_function_start (pc); > > > > + else > > > > + start =3D pc; > > > > > > This goes into implementation detail of get_pc_function_start. > > > Rather always call get_pc_function_start but one should check if it > > > failed in all cases (you do not check if get_pc_function_start > > > failed). get_pc_function_start returns 0 if it has failed. > > > > The check is implicit since pc can't be zero. >=20 > PC can be zero on embedded platforms, _start commonly starts there. It i= s a > bug of get_pc_function_start it is not compatible with it. >=20 > Newer implementation of get_pc_function_start may fail in some case even > if FUN or MFUN is not NULL. >=20 > The code is making needless assumptions about get_pc_function_start > inners. >=20 >=20 > > > Or was the 'fun !=3D NULL || mfun !=3D NULL' check there for performa= nce > > > reasons? > > > > That's for performance reasons. No need to call the function if we > > know it won't help us. >=20 > The idea was that for example GDB may introduce 3rd kind of symbols, > besides minimal symbols and full symbols. At that moment > get_pc_function_start could work with the 3rd kind of symbol which the > code as is would not call get_pc_function_start at all. >=20 > The code is making needless assumptions about get_pc_function_start > inners. I removed the symbol NULL check and instead check for a zero return of get_pc_function_start (PC). This still rules out zero as a valid PC value, but that's the current error return value of get_pc_function_start. > OK, please do not misuse patch series for chronological development. > Patch series splitting is there for separation of topic. Do you want me to squash the series into a single patch? > > > Unrelated to this patch but the function > > > record_btrace_insn_history_from does not need to be virtualized. It > > > does not access any internals of record-btrace.c, it could be fully > > > implemented in the superclass record.c and to_insn_history_from > > > could be deleted. > > > > > > The same applies for record_btrace_call_history_from and > > > to_call_history_from. > > > > Both depend on the numbering scheme, which is an implementation detail. > > They both assume that counting starts at 0 (at 1 in a later patch). > > > > This does not hold for record-full, where the lowest instruction may > > be bigger than zero. >=20 > OK, one reason is that currently there is no implementation of these > methods for record-full: > (gdb) record instruction-history > You can't do that when your target is `record-full' >=20 > The second reason is that while record-full can drop old record, seeing o= nly > the last window: > (gdb) set record full insn-number-max 10 > (gdb) record > (gdb) info record > Active record target: record-full > Record mode: > Lowest recorded instruction number is 1587. > Highest recorded instruction number is 1596. > Log contains 10 instructions. > Max logged instructions is 10. >=20 > btrace backend does not seem to support such sliding window (the kernel > buffer sliding is unrelated). GDB still stores in its memory all the btr= ace > records and one cannot do anything like > (gdb) set record btrace insn-number-max 10 It's inherent in btrace. We only ever see the tail of the trace. We exten= d the recorded trace when the kernel buffer does not overflow between updates. Otherwise, we discard the trace in GDB and start anew with the current tail. > Still I believe the code for the methods like to_insn_history_from should= be > common for all the backends as the user visible behavior should be the > same. > And this common code should support arbitrary "Lowest recorded instruction > number" (which the btrace backend currently does not support). The lowest recorded instruction is always zero for record-btrace. If we added target methods to query for the lowest and highest instruction number, we could implement the logic in record.c. I didn't see any benefit in that, so I didn't do it. We will end up with about the same number of target methods either way. Regards, Markus. Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052