Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751774AbdCCCkX (ORCPT ); Thu, 2 Mar 2017 21:40:23 -0500 Received: from LGEAMRELO12.lge.com ([156.147.23.52]:45562 "EHLO lgeamrelo12.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750709AbdCCCkW (ORCPT ); Thu, 2 Mar 2017 21:40:22 -0500 X-Original-SENDERIP: 156.147.1.151 X-Original-MAILFROM: namhyung@kernel.org X-Original-SENDERIP: 10.177.227.17 X-Original-MAILFROM: namhyung@kernel.org Date: Fri, 3 Mar 2017 11:40:18 +0900 From: Namhyung Kim To: Taeung Song 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 Subject: Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr Message-ID: <20170303024018.GJ30710@sejong> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3975 Lines: 124 + 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 > > > > > > > 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