Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr

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

On 03/01/2017 10:17 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Wed, Mar 01, 2017 at 04:59:51AM +0900, Taeung Song wrote:
>> Currently perf-annotate show wrong line numbers.
>>
>> For example,
>> Actual source code is as below
>>
>> ...
>>   21 };
>>   22
>>   23 unsigned int limited_wgt;
>>   24
>>   25 unsigned int get_cond_maxprice(int wgt)
>>   26 {
>> ...
>>
>> However, the output of perf-annotate is as below.
>>
>>   4   Disassembly of section .text:
>>
>>   6   0000000000400966 <get_cond_maxprice>:
>>   7   get_cond_maxprice():
>>   26  };
>>
>>   28  unsigned int limited_wgt;
>>
>>   30  unsigned int get_cond_maxprice(int wgt)
>>   31  {
>>
>> The cause is the wrong way counting line numbers
>> in symbol__parse_objdump_line().
>> So remove wrong current code counting line number and
>> use other method for it using functions related to addr2line
>> instead of the output of '-l' of objdump.
>
> Hmm.. do you think it's a bug of objdump or it's perf failing to parse
> the line number correctly?  I'd like to see the output of `objdump -l`
>

Both are ok.
'objdump -l' hasn't a bug related to line number
and perf's method parsing the line number is ok.

But symbol__parse_objdump_line() wrongly count line numbers
after parsing it as below.

1172         /* /filename:linenr ? Save line number and ignore. */
1173         if (regexec(&file_lineno, line, 2, match, 0) == 0) {
1174                 *line_nr = atoi(line + match[1].rm_so);
1175                 return 0;
1176         }
...
1208         dl = disasm_line__new(offset, parsed_line, privsize, 
*line_nr, arch, map);
1209         free(line);
1210         (*line_nr)++;

Increasing line_nr each asm line is wrong method.
Because 'line_nr' means actual source code line number.

Sure, I can fix only the wrong counting way.
But the above parsing method(1172~1176) is never used because of 'grep -v'
in command as below.
(the grep already remove lines containing filename:linenr of output)

1435         snprintf(command, sizeof(command),
1436                  "%s %s%s --start-address=0x%016" PRIx64
1437                  " --stop-address=0x%016" PRIx64
1438                  " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
1439                  objdump_path ? objdump_path : "objdump",
1440                  disassembler_style ? "-M " : "",
1441                  disassembler_style ? disassembler_style : "",
1442                  map__rip_2objdump(map, sym->start),
1443                  map__rip_2objdump(map, sym->end),
1444                  symbol_conf.annotate_asm_raw ? "" : "--no-show-raw",
1445                  symbol_conf.annotate_src ? "-S" : "",
1446                  symfs_filename, symfs_filename);

Therefore, I think it is better to do three things

   1) fix the wrong counting line number problem
   2) remove unused the line number parsing method
   3) In addtion, a bit reduce objdump dependency
      using functions related to addr2line of perf.

What do you think about that ?
Is it bad idea ?

>>
>> However, despite the correct line numbers,
>> we can't show proper source code view
>> because of limitations from output of 'objdump -S'.
>>
>> So, next commit will resolve the limitations from 'objdump -S'
>> with the new source code view.
>
> It seems not related with this commit..
>

Okey, will remove the mention.

Thanks,
Taeung