This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [patch v4 03/24] btrace: change branch trace data structure
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "Himpel, Christian" <christian dot himpel at intel dot com>
- Date: Tue, 10 Sep 2013 09:10:33 +0000
- Subject: RE: [patch v4 03/24] btrace: change branch trace data structure
- Authentication-results: sourceware.org; auth=none
- References: <1372842874-28951-1-git-send-email-markus dot t dot metzger at intel dot com> <1372842874-28951-4-git-send-email-markus dot t dot metzger at intel dot com> <20130818190426 dot GC24153 at host2 dot jankratochvil dot net>
> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Sunday, August 18, 2013 9:04 PM
Thanks for your review.
> > @@ -248,89 +185,477 @@ ftrace_skip_file (struct btrace_func *bfun, const
> char *filename)
> > else
> > bfile = "";
> >
> > - if (filename == NULL)
> > - filename = "";
> > + if (fullname == NULL)
> > + fullname = "";
>
> The code should not assume FULLNAME cannot be "", "" is theoretically a
> valid
> source file filename.
>
> Second reason is that currently no caller of ftrace_skip_file will pass NULL
> as the second parameter.
>
> So the function can be just:
>
> if (sym == NULL)
> return 1;
>
> bfile = symtab_to_fullname (sym->symtab);
>
> return filename_cmp (bfile, fullname) != 0;
Sounds good. Changed it.
> And the function has only one caller so it would be IMO easier to read to get
> it inlined.
I'd rather keep it separate for documentation purposes.
> And not sure if it matters much but doing two symtab_to_fullname for
> comparison of two symtabs equality is needlessly expensive
> - symtab_to_fullname is very expensive. There are several places in GDB
> doing first:
> /* Before we invoke realpath, which can get expensive when many
> files are involved, do a quick comparison of the basenames. */
> if (!basenames_may_differ
> && filename_cmp (lbasename (symtab1->filename),
> lbasename (symtab2->filename)) != 0)
> continue;
I would expect it does matter. I noticed a slowdown when the trace gets
in the order of 1M instructions. I have not done any profiling, yet, but I will
get back to this once I look into performance.
> > +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.
> > +/* Fix up the caller for a function segment. */
>
> IIUC it should be:
>
> /* Fix up the caller for all segments of a function call. */
Thanks. Yes, that's how it should be.
> > + /* 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.
> > - 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.
> > + /* 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.
> 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.
> > +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.
> > +const struct btrace_insn *
> > +btrace_insn_get (const struct btrace_insn_iterator *it)
> > +{
> > + const struct btrace_function *bfun;
> > + unsigned int index, end;
> > +
> > + if (it == NULL)
> > + return NULL;
>
> I do not see this style in GDB and IMO it can delay bug report from where it
> occured. Either gdb_assert (it != NULL); or just to leave it crashing below.
OK. It's just a habit.
> > + index = it->index;
> > + bfun = it->function;
> > + if (bfun == NULL)
> > + return NULL;
>
> btrace_insn_iterator::function does not state if NULL is allowed and its
> meaning in such case. btrace_call_get description states "NULL if the
> interator points past the end of the branch trace." but I do not see it could
> be set to NULL in any current code (expecting it was so in older code).
> btrace_insn_next returns the last instruction not the last+1 pointer.
>
> IMO it should be stated btrace_insn_iterator::function can never be NULL
> and
> here should be either gdb_assert (bfun != NULL); or just nothing, like above.
Done. That's indeed a leftover. Thanks for pointing it out.
> > +
> > + btinfo = it->btinfo;
> > + if (btinfo == NULL)
> > + return 0;
>
> Similiarly btrace_call_iterator::btinfo does not state if it can be NULL and
> consequently here to code should rather gdb_assert it (or ignore it all).
OK. I ignored it.
> > + 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).
> > +
> > + /* 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.
> > uiout = current_uiout;
> > uiout_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout,
> > "insn history");
> > - btinfo = require_btrace ();
> > - last = VEC_length (btrace_inst_s, btinfo->itrace);
> > + 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.
> > - 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.
> 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.
> > - if (end <= begin)
> > + if (high <= low)
> > error (_("Bad range."));
>
> Similar inclusive/exclusive question as in record_btrace_insn_history_range
> and
> to_insn_history_range.
>
> /* We want both begin and end to be inclusive. */
> btrace_call_next (&end, 1);
>
> (gdb) record function-call-history 700,701
> 700 _dl_lookup_symbol_x
> 701 _dl_fixup
> (gdb) record function-call-history 700,700
> Bad range.
I fixed this behaviour in the respective patch, added a test, and updated
the comment in target.h.
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