Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751261AbdCBGHG (ORCPT ); Thu, 2 Mar 2017 01:07:06 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:35766 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbdCBGHF (ORCPT ); Thu, 2 Mar 2017 01:07:05 -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> Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra , Wang Nan , Masami Hiramatsu , Jiri Olsa From: Taeung Song Message-ID: Date: Thu, 2 Mar 2017 15:05:14 +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: <20170301131702.GA2690@danjae.aot.lge.com> 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: 3343 Lines: 107 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. 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) 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 ? >> >> 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