From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1147 invoked by alias); 21 Sep 2013 19:44:22 -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 1131 invoked by uid 89); 21 Sep 2013 19:44:21 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 21 Sep 2013 19:44:21 +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_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8LJiHhK027435 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 21 Sep 2013 15:44:17 -0400 Received: from host2.jankratochvil.net (ovpn-116-46.ams2.redhat.com [10.36.116.46]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r8LJiD93017407 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Sat, 21 Sep 2013 15:44:16 -0400 Date: Sat, 21 Sep 2013 19:44:00 -0000 From: Jan Kratochvil To: "Metzger, Markus T" Cc: "gdb-patches@sourceware.org" , "Himpel, Christian" Subject: Re: [patch v4 03/24] btrace: change branch trace data structure Message-ID: <20130921194413.GA20539@host2.jankratochvil.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00789.txt.bz2 On Mon, 16 Sep 2013 10:59:38 +0200, Metzger, Markus T wrote: > > 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. > > 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? Definitely not. Not that it is too important but: I meant that [patch v4 08/24] record-btrace: make ranges include begin and end could be merged into [patch v4 03/24] btrace: change branch trace data structure as the patch #08 modifies only new code from patch #03, without any incremental additions; just changing exclusive range implemented by #03 to an inclusive range. I understand one can easily overlook it on such a big series or I missed some other reason for the separate #08 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. > > > > 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' > > > > The second reason is that while record-full can drop old record, seeing only > > 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. > > > > btrace backend does not seem to support such sliding window (the kernel > > buffer sliding is unrelated). GDB still stores in its memory all the btrace > > 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 extend 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. (gdb) set record full insn-number-max 3 (gdb) record (gdb) stepi (gdb) stepi (gdb) stepi (gdb) info record Active record target: record-full Record mode: Lowest recorded instruction number is 1. Highest recorded instruction number is 3. Log contains 3 instructions. Max logged instructions is 3. (gdb) stepi Do you want to auto delete previous execution log entries when record/replay buffer becomes full (record full stop-at-limit)?([y] or n) y (gdb) info record Active record target: record-full Record mode: Lowest recorded instruction number is 2. Highest recorded instruction number is 4. Log contains 3 instructions. Max logged instructions is 3. While 'record full' stores only the tail of selected size 'record btrace' stores everything and one has to occasionally 'record stop' and 'record btrace' again otherwise GDB runs out of memory. At least this is what I expect for long-term running inferiors, I do not have Haswell available to verify it. With btrace one cannot select the tail size (there is nothing like 'set record btrace insn-number-max 3'), perf_event_buffer_size() is auto-detected, 4MB max. I try to explain the numbering ranges X-Y (and not just 1-Y) should apply also to record btrace, not just to record full. btrace also needs to drop very old records and it is inconvenient for users to renumber the events all the time. This also implies that the functions/instructions numbering style of both btrace and full should be the same, and therefore those function should be common in record.c and the vectorization to_call_history_from is not needed then. > > 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. Which may cause GDB memory many-MB overflow if one traces long-running inferior, I guess. > 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. Maybe the number of methods will be the same but it seems more logical to me that the numbering/windowing should be common for all the backends. Thanks, Jan