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

[Taeung Song]

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