Sorry for the long delay. Just revisiting that.
On Wed, Oct 22, 2014 at 10:03:51AM +0900, Namhyung Kim wrote:
> > | | f2 tcall.c:5
> > | | f1 tcall.c:12
> > | | f1 tcall.c:12
> > | | f2 tcall.c:7
> > | | f2 tcall.c:5
> > | | f1 tcall.c:11
>
> I think it'd be better if it just prints function names as normal
> callchain does (and optionally srcline with a switch) and duplicates
> removed like below:
>
> 54.91% tcall.c:6 [.] f2 tcall
> |
> |--65.53%-- f2 tcall.c:5
> | |
> | |--70.83%-- f1
> | | main
> | | f1
> | | f2
> | | f1
> | | f2
I considered this. For this example it doesn't make much difference
because the functions are so small.
But for anything larger I really need the line numbers to make
sense of it.
So I prefer to keep them. I'll look into some easy switch
to turn them off though.
> > + if (sort__has_parent && !*parent &&
> > + symbol__match_regex(al.sym, &parent_regex))
> > + *parent = al.sym;
> > + else if (have_ignore_callees && root_al &&
> > + symbol__match_regex(al.sym, &ignore_callees_regex)) {
> > + /* Treat this symbol as the root,
> > + forgetting its callees. */
> > + *root_al = al;
> > + callchain_cursor_reset(&callchain_cursor);
> > + }
> > + if (!symbol_conf.use_callchain)
> > + return -EINVAL;
>
> This check already went away.
>
> And, to remove duplicates, I think we need to check last callchain
> cursor node wrt the callchain_param.key here.
I don't understand the comment. I'm not modifying anything
that has been already added to the callchain. Just things
to be added in the future. So why would I need to check
or change the cursor?
>
> Also, by comparing 'from' address, I'd expect you add the from address
> alone but you add both of 'from' and 'to'. Do we really need to do
> that?
Adding from and to makes it much clearer to the user what happens,
especially with conditional branches, so they can follow the
control flow.
> And the first address saved in normal callchain is address of the
> function itself so it might be 'to' you need to check if sampled before
> any branch in a function.
I'm checking against the CALL, not the target.
>
> > + } else
> > + be[i] = branch->entries[branch->nr - i - 1];
> > + }
> > +
> > + nr = remove_loops(be, nr);
> > +
> > + for (i = 0; i < nr; i++) {
> > + err = add_callchain_ip(machine, thread, parent,
> > + root_al,
> > + -1, be[i].to);
> > + if (!err)
> > + err = add_callchain_ip(machine, thread,
> > + parent, root_al,
> > + -1, be[i].from);
> > + if (err == -EINVAL)
> > + break;
> > + if (err)
> > + return err;
> > + }
> > + chain_nr -= nr;
>
> I'm not sure this line is needed.
Without that i could exceed the limit.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
