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

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

On Tue, 10 Sep 2013 11:10:33 +0200, Metzger, Markus T wrote:
> > > +static void
> > > +ftrace_update_caller (struct btrace_function *bfun,
> > > +		      struct btrace_function *caller,
> > > +		      unsigned int flags)
> > 
> > FLAGS should be enum btrace_function_flag (it is ORed bitmask but GDB
> > displays
> > enum ORed bitmasks appropriately).
> 
> Changed it.  This will burn us when we want to switch to C++ someday.

I would prefer a wrapper class, so that Enum | Enum remains Enum.
OTOH wrapper classes may not be too easily understandable for contributors.
That would need an agreement first.

But that is off-topic here, so far enums types have been kept.


> > > +	  /* We maintain levels for a series of returns for which we have
> > > +	     not seen the calls, but we restart at level 0, otherwise.  */
> > > +	  bfun->level = min (0, prev->level) - 1;
> > 
> > Why is there the 'min (0, ' part?
> 
> When we return from some tail call chain, for example, and we have
> not traced the actual function call that started this chain.
> 
> I added a reference to tail calls in the comment.

I have found now the problem is:

struct btrace_function
  /* The function level in a back trace across the entire branch trace.
     A caller's level is one higher than the level of its callee.

     Levels can be negative if we see returns for which we have not seen
     the corresponding calls.  The branch trace thread information provides
     a fixup to normalize function levels so the smallest level is zero.  */
  int level;

should be:
-    A caller's level is one higher than the level of its callee.
+    A callee's level is one higher than the level of its caller.

as one can see for gdb.btrace/tailcall.exp:

record function-call-history /c 1^M
1       0main^M
2       1  foo^M
3       2    bar^M
4       0main^M
        ^

In such case please rename btrace_function->level to something else, such as
btrace_function->calls_level or btrace_function->reverse_level etc.
as it is the opposite of the related GDB frame_info->level field.


The 'min (0, ' then makes sense to me:
1       1  foo
2       2    bar
3       0main


> > > -	  bfun = VEC_safe_push (btrace_func_s, ftrace, NULL);
> > > +	  /* There is a call in PREV's back trace to which we should have
> > > +	     returned.  Let's remain at this level.  */
> > > +	  bfun->level = prev->level;
> > 
> > Shouldn't here be rather:
> > 	  bfun->level = caller->level;
> 
> We did not return to this caller - otherwise, we would have found it before.
> 
> This is handling a case that should not normally occur.  No matter what we
> do, the indentation will likely be off in one or the other case.

We know the most recent tail calls are done so it should be safe to subtract
them from the current level.

But I agree it should not happen so the current code also makes sense.


> > > +	  /* If we have symbol information for our current location, use
> > > +	     it to check that we jump to the start of a function.  */
> > > +	  if (fun != NULL || mfun != NULL)
> > > +	    start = get_pc_function_start (pc);
> > > +	  else
> > > +	    start = 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.

PC can be zero on embedded platforms, _start commonly starts there.  It is
a bug of get_pc_function_start it is not compatible with it.

Newer implementation of get_pc_function_start may fail in some case even if
FUN or MFUN is not NULL.

The code is making needless assumptions about get_pc_function_start inners.


> > Or was the 'fun != NULL || mfun != NULL' check there for performance
> > reasons?
> 
> That's for performance reasons.  No need to call the function if we know
> it won't help us.

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.

The code is making needless assumptions about get_pc_function_start inners.


> > > +static void
> > > +btrace_compute_ftrace (struct btrace_thread_info *btinfo,
> > > +		       VEC (btrace_block_s) *btrace)
> > 
> > When doing any non-trivial trace on buggy Nehalem (enabling btrace by a
> > GDB
> > patch) GDB locks up on "info record".  I found it is looping in this function
> > with too big btrace range:
> > (gdb) p *block
> > $5 = {begin = 4777824, end = 9153192}
> > 
> > But one can break it easily with CTRL-C and hopefully on btrace-correct CPUs
> > such things do not happen.
> 
> We should not normally get such trace.  I could add a simple heuristic that
> blocks can't be bigger than x bytes but I don't think that's necessary.

OK; I agree such trace should not normally happen.


> > > +  bfun = it->function;
> > > +  if (bfun != NULL)
> > > +    return bfun->number;
> > 
> > Similiarly btrace_call_iterator::function does not state if it can be NULL and
> > consequently here to code should rather gdb_assert it (or ignore it all).
> 
> For the call iterator, function can be NULL (see e.g. btrace_call_end).
> 
> It is documented in btrace.h  (maybe I added this afterwards).

I see now, my mistake checking it all.


> > > +  /* The branch trace function segment.
> > > +     This will be NULL for the iterator pointing to the end of the trace.  */
> > 
> > btrace_call_next can return NULL in function while btrace_insn_next returns
> > rather the very last of all instructions.  Is there a reason for this
> > difference?
> 
> The end iterator points one past the last element.  For instructions, this is
> one past the last instruction index in the last (non-empty) function segment.
> For calls, this is a NULL function segment.

Thanks for the explanation, I see now there is no better solution.

Functions are given by pointer while instructions are given by their index.
Therefore "after the end" for function is NULL while "after the end" for
instructions can be last_index+1.


> > > +  low = (unsigned int) from;
> > > +  high = (unsigned int) to;
> > 
> > I do not see a reason for this cast, it is even not signed vs. unsigned.
> 
> >From and to are ULONGEST which may be 64bit whereas low and high
> are 32bit.

The '(unsigned int)' cast just does not have to be there.  If you want to
highlight the assignment does trim the type width you could write that rather
as a comment.


> > > -  if (end <= begin)
> > > +  if (high <= low)
> > >      error (_("Bad range."));
> > 
> > Function description says:
> >     /* Disassemble a section of the recorded execution trace from instruction
> >        BEGIN (inclusive) to instruction END (exclusive).  */
> > 
> > But it beahves as if END was inclusive.  Or I do not understand something?
> > (gdb) record instruction-history 1925,1926
> > 1925	   0x00007ffff62f6afc <memset+28>:	ja     0x7ffff62f6b30
> > <memset+80>
> > 1926	   0x00007ffff62f6afe <memset+30>:	cmp    $0x10,%rdx
> > 
> > If it should be inclusive then LOW == HIGH should be allowed:
> > (gdb) record instruction-history 1925,1925
> > Bad range.
> > 
> > Not in this patch (in some later one) but there is also:
> >       /* We want both begin and end to be inclusive.  */
> >       btrace_insn_next (&end, 1);
> > 
> > which contradicts the description of to_insn_history_range.
> 
> Initially, I had it exclusive, and this should be the behaviour if you just apply
> this patch.  Later on, I changed it to be inclusive, instead, to better match
> existing commands like list.
> 
> I fixed this behaviour in the respective patch, added a test, and updated
> the comment in target.h.

OK, please do not misuse patch series for chronological development.
Patch series splitting is there for separation of topic.


> > 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

May it be a problem for btrace practical use cases?  As I do not have CPU
capable of btrace I cannot say how long it will take before the btrace storage
may become too big (such as >100MB) for long-term running processes under GDB.

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).

But as it is only a future extension and to_insn_history_from & co. methods
are already checked in FSF GDB HEAD this discussion is off-topic for this
patchset and the method implementations can be removed and unified into common
functions after anyone implements "record instruction-history" for
"record-full" and after anyone implements
arbitrary "Lowest recorded instruction number" - window sliding - for the
btrace backend.


Thanks,
Jan