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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----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 != 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.

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


> 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