RE: [PATCH 02/15] btrace: change branch trace data structure

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Monday, May 13, 2013 5:25 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; Himpel, Christian
> Subject: Re: [PATCH 02/15] btrace: change branch trace data structure
> 
> Hi Markus,
> 
> as mailed off-list you have an updated version so sending only a partial
> review as I have done so far.

Thanks for your review.

[...]

> > -/* Compute the function trace from the instruction trace.  */
> > +/* Allocate and initialize a new branch trace function segment.  */
> 
> Something like:
> 
> +/* If both FUN and MFUN are NULL it is marker of the end of trace.  */

I do not understand this comment.

Actually, it turns out that we can have functions without even minimal symbol
Information.  See testsuite/gdb.btrace/unknown_functions.exp in a later patch.

[...]

> > +      bfun->insn_offset = prev->insn_offset
> > +	+ VEC_length (btrace_insn_s, prev->insn);
> 
> GNU Coding Standards require multi-line expressions to use parentheses,
> therefore:
>       bfun->insn_offset = (prev->insn_offset
>                            + VEC_length (btrace_insn_s, prev->insn));

Thanks.  I believe there are more instances of this around.

[...]

> > +      if (VEC_empty (btrace_insn_s, bfun->insn))
> > +	continue;
> 
> Shouldn't here be rather "return NULL" instead of continue?  I do not
> understand in which cases BFUN->INSN can be empty.  Empty INSN case could be
> also described in btrace_function->insn.

In the sense of this function, continue is the right thing to do.  Nevertheless, we
currently do not generate such function segments and their handling is not
consistent.  They were intended to allow adding artificial frames later on to
represent for example inlined functions.

I changed this into gdb_assert and added a comment to the declaration of
struct btrace_fucntion.

[...]

> > +  /* If we didn't have a function before, we create one.  */
> > +  if (bfun == NULL)
> > +    return ftrace_new_function (bfun, mfun, fun);
> 
> mfun == NULL && fun == NULL is not allowed according to the comments in struct
> btrace_function.  Already suggested gdb_assert for ftrace_new_function but it
> seems it may happen here.

I updated the comment.  See above for an example.

[...]

> > +  /* Check the last instruction, if we have one.
> > +     We do this check first, since it allows us to fill in the call stack
> > +     links in addition to the normal flow links.  */
> > +  last = NULL;
> > +  if (!VEC_empty (btrace_insn_s, bfun->insn))
> > +    last = VEC_last (btrace_insn_s, bfun->insn);
> >
> > -      /* Let's see if we have source correlation, as well.  */
> > -      sal = find_pc_line (pc, 0);
> > -      if (sal.symtab == NULL || sal.line == 0)
> > +  if (last != NULL)
> 
> This conditional is excessive, it is true iff !VEC_empty is true above.

I still need to check either one of them again, here.  Last is used also in
the if statement below....

[...]

> > +    {
> > +    }
> > +
> > +  /* Check if we're switching functions for some other reason.  */
> > +  if (ftrace_function_switched (bfun, mfun, fun))
> > +    {

[...]

> > +      if (last != NULL)

...here.

> >  	{
> > -	  DEBUG_FTRACE ("ignoring file at %u, pc=%s, file=%s", idx,
> > -			core_addr_to_string_nz (pc), filename);
> > -	  continue;
> > +	  CORE_ADDR start, lpc;
> > +
> > +	  /* 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 anyway
> - get_pc_function_start returns 0 if it has failed.
> 
> Or was the 'fun != NULL || mfun != NULL' check there for performance reasons?

Without symbol information, we assume that we do jump to the beginning of
a function.  If we called get_pc_function_start unconditionally, we wouldn't be
able to tell from a zero return whether we knew that we did not jump to the start
of a function or whether we just don't know.

We already know that we switched functions (i.e. that the symbol information
changed).  I put in this PC check as another safeguard for detecting tail calls.

[...]

> > +	  /* Jumps indicate optimized tail calls.  */
> > +	  if (start == pc
> > +	      && gdbarch_insn_jump_p_p (gdbarch)
> > +	      && gdbarch_insn_jump_p (gdbarch, lpc))
> > +	    return ftrace_new_tailcall (bfun, mfun, fun);
> 
> Cannot a plain intra-function jump be confused into a tailcall due to
> a tripped binary?  FUN and MFUN are NULL for stripped binary,
> therefore "start = pc;" gets assigned above, which will call
> ftrace_new_tailcall.  I did not try to reproduce it, though.

Yes, but unlikely.  We already had a change in symbol information, so we're
jumping from a section with symbol information to one without (or vice versa
or within a section with symbol information but for those get_pc_function_start
should return the proper address).

[...]

> >    btrace = target_read_btrace (btinfo->target, btrace_read_new);
> > -  if (VEC_empty (btrace_block_s, btrace))
> > -    return;
> > -
> > -  btrace_clear (tp);
> > +  cleanup = make_cleanup (VEC_cleanup (btrace_block_s), &btrace);
> >
> > -  btinfo->btrace = btrace;
> > -  btinfo->itrace = compute_itrace (btinfo->btrace);
> > -  btinfo->ftrace = compute_ftrace (btinfo->itrace);
> > +  if (!VEC_empty (btrace_block_s, btrace))
> > +    {
> > +      btrace_clear (tp);
> > +      btrace_compute_ftrace (btinfo, btrace);
> > +    }
> 
> If BTRACE is empty shouldn't TP's btrace be still cleared?

I'm calling target_read_btrace with flag btrace_read_new.  It returns an
empty block trace vector if the trace has not changed.  In this case, the old
trace is still correct.

[...]

> > +enum btrace_function_flag
> > +{
> > +  /* The 'up' link interpretation.
> > +     If set, it points to the function segment we returned to.
> > +     If clear, it points to the function segment we called from.  */
> > +  bfun_up_links_to_ret = (1 << 0),
> > +
> > +  /* The 'up' link points to a tail call.  This obviously only makes sense
> > +     if bfun_up_links_to_ret is clear.  */
> > +  bfun_up_links_to_tailcall = (1 << 1)
> 
> enum values are uppercased in GDB.

It appears to me that most enumeration constants are lowercase.  If you
insist, I'll make mine UPPERCASE, but I find lowercase more readable.


> But I do not see a reason to have the flags field and this enum.  You can just
> use something like
>   unsigned bfun_up_links_to_ret : 1;
>   unsigned bfun_up_links_to_tailcall : 1;
> 
> instead of the flags field in struct btrace_function.  Sometimes one wants to
> pass FLAGS as a function parameter but that is not the case here.

We don't know what other flags we might need.  A flags bit-vector seems more
flexible.

[...]

> > +struct btrace_function
> >  {
> >    /* The full and minimal symbol for the function.  One of them may be NULL.  */
> 
> IIUC from the code rather:
> 
> +/* Iff both FUN and MFUN are NULL it is marker of the end of trace.  */

That changed, meanwhile.  I removed the empty function as end marker and I found
exmples where both are NULL.

[...]

> > +/* Return the instruction number for a branch trace iterator.  Returns zero
> 
> s/the instruction number/index of the instruction relative to start of the
> function/

I use the term "instruction number" in other places, as well.

[...]

> >    struct gdbarch *gdbarch;
> > -  struct btrace_inst *inst;
> > -  unsigned int idx;
> > +  struct btrace_insn *inst;
> 
> Dead variable 'inst'.

Thanks.  Couldn't we make gcc warn about this?


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