lkml.org 
[lkml]   [2017]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr
+ 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


>
> > >
> > > 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

\
 
 \ /
  Last update: 2017-03-03 03:41    [W:0.078 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site