Re: [Question] about symbol__get_source_line() in util/annotate.c

[Taeung Song <treeze.taeung@gmail.com>]

On 02/23/2017 02:14 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Thu, Feb 23, 2017 at 12:31:08PM +0900, Taeung Song wrote:
>> Hi Namhyung,
>>
>> I have two question about the code util/annotate.c:1653~1676
>>
>> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/tree/tools/perf/util/annotate.c?h=perf/core#n1653
>>
>> 1653         for (i = 0; i < len; i++) {
>> 1654                 u64 offset;
>> 1655                 double percent_max = 0.0;
>> 1656
>> 1657                 src_line->nr_pcnt = nr_pcnt;
>> 1658
>> 1659                 for (k = 0; k < nr_pcnt; k++) {
>> 1660                         h = annotation__histogram(notes, evidx + k);
>> 1661                         src_line->samples[k].percent = 100.0 * h->addr[i] / h->sum;
>> 1662
>> 1663                         if (src_line->samples[k].percent > percent_max)
>> 1664                                 percent_max = src_line->samples[k].percent;
>> 1665                 }
>> 1666
>> 1667                 if (percent_max <= 0.5)
>> 1668                         goto next;
>> 1669
>> 1670                 offset = start + i;
>> 1671                 src_line->path = get_srcline(map->dso, offset, NULL, false);
>> 1672                 insert_source_line(&tmp_root, src_line);
>> 1673
>> 1674         next:
>> 1675                 src_line = (void *)src_line + sizeof_src_line;
>> 1676         }
>>
>>
>> 1) Why use 'offset = start + i;' ?
>> For example,
>> There are addresses matched with test.c:26 as below,
>>
>>     400816:       push   %rbp
>>     400817:       mov    %rsp,%rbp
>>     40081a:       mov    %edi,-0x24(%rbp)
>>     40081d:       mov    %rsi,-0x30(%rbp)
>>
>> If using 'offset = start + i;' in the above for loop,
>> needless addresses can be checked.
>
> Right, that's not nice.
>
>>
>>
>>     i=0, 400816
>>     i=1, 400817
>>     i=2, 400818 (nonvalidated)
>>     i=3, 400819 (nonvalidated)
>>     i=4, 40081a
>>     i=5, 40081b (nonvalidated)
>>     i=6, 40081c (nonvalidated)
>>     i=7, 40081d
>>
>> So I think it is better to use dissemble_line array such as
>> (in order to check only validated addresses.)
>>
>>     list_for_each_entry(dl, &notes->src->source, node)
>>
>> What about this ?
>
> I agree with you.  Maybe we can get rid of the source_line struct
> entirely.

Sounds good. the source_line struct is a bit obstacle..

>>
>> 2) Why use the if statement as below ?
>>
>>     if (percent_max <= 0.5)
>>             goto next;
>>
>> I think it is more correct to use 0.0 instead of 0.5
>>
>> What do you think about that ?
>
> Well, I think that the summary line doesn't want to show too many
> (small) lines.  Using 0.0 instead seems meaningless though.
>
> Thanks,
> Namhyung
>

I got it!

Thanks,
Taeung