[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 3 Mar 2017 12:25:28 +0900
From: Taeung Song <treeze.taeung@...il.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Wang Nan <wangnan0@...wei.com>,
Masami Hiramatsu <mhiramat@...nel.org>,
Jiri Olsa <jolsa@...hat.com>, Andi Kleen <andi@...stfloor.org>,
kernel-team@....com
Subject: Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched
with addr
On 03/03/2017 11:40 AM, Namhyung Kim wrote:
> + Andi Kleen who wrote the code.
>
> On Thu, Mar 02, 2017 at 03:05:14PM +0900, Taeung Song wrote:
>>
>>
>> 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.
>
> Hmm.. ok. It looks like that it should reuse the old line_nr as is.
>
>>
>> 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)
>
> Right, but only if filename is same as binary name.
>
>>
>> 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 ?
>
> I think we need to fix 1) definitely, but not sure about 2) and 3).
> If objdump could do all necessary works, why not use it? :)
>
> Thanks,
> Namhyung
>
>
Okey! I'll concentrate on fixing 1)
,not removing objdump -l :)
Thanks,
Taeung
Powered by blists - more mailing lists