Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751560AbdCCDeN (ORCPT ); Thu, 2 Mar 2017 22:34:13 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:35784 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751148AbdCCDeM (ORCPT ); Thu, 2 Mar 2017 22:34:12 -0500 Subject: Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr To: Namhyung Kim References: <1488311993-15124-1-git-send-email-treeze.taeung@gmail.com> <1488311993-15124-2-git-send-email-treeze.taeung@gmail.com> <20170301131702.GA2690@danjae.aot.lge.com> <20170303024018.GJ30710@sejong> Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra , Wang Nan , Masami Hiramatsu , Jiri Olsa , Andi Kleen , kernel-team@lge.com From: Taeung Song Message-ID: <974fba56-36a1-45de-3f54-7e58acc18a22@gmail.com> Date: Fri, 3 Mar 2017 12:25:28 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170303024018.GJ30710@sejong> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3739 Lines: 117 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 : >>>> 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