Re: [patch v4 03/24] btrace: change branch trace data structure

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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