Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757740AbdCUQU6 (ORCPT ); Tue, 21 Mar 2017 12:20:58 -0400 Received: from mail-pg0-f68.google.com ([74.125.83.68]:33844 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756924AbdCUQUX (ORCPT ); Tue, 21 Mar 2017 12:20:23 -0400 Subject: Re: [PATCH 4/4] perf annotate: More exactly grep -v of the objdump command To: Arnaldo Carvalho de Melo , Taeung Song References: <1489978617-31396-1-git-send-email-treeze.taeung@gmail.com> <1489978617-31396-5-git-send-email-treeze.taeung@gmail.com> <20170321143728.GD3641@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Wang Nan , Masami Hiramatsu , Jiri Olsa From: Taeung Song Message-ID: Date: Wed, 22 Mar 2017 01:19:34 +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: <20170321143728.GD3641@kernel.org> Content-Type: text/plain; charset=windows-1252; 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: 7831 Lines: 245 Thank you for your review :) On 03/21/2017 11:37 PM, Arnaldo Carvalho de Melo wrote: > Em Mon, Mar 20, 2017 at 11:56:57AM +0900, Taeung Song escreveu: >> grep -v "file name" in the objdump command >> cause a side effect eliminating filename:linenr of >> output of 'objdump -l' if the object file name and >> source file name are about the same so fix it. >> >> The objdump command in symbol__disassemble() can be as below >> >> $ objdump -l -d -S -C /home/taeung/hello --start-address=... >> >> /home/taeung/hello: file format elf64-x86-64 >> >> Disassembly of section .text: >> >> 0000000000400526
: >> main(): >> /home/taeung/hello.c:4 >> >> void main() >> { >> 400526: 55 push %rbp >> 400527: 48 89 e5 mov %rsp,%rbp >> /home/taeung/hello.c:5 >> ... >> >> But currently it use grep -v "file name" e.g. "/home/taeung/hello" >> in the objdump command to remove the first line containing file name >> and file format such as, >> >> Before: >> $ objdump -l -d -S -C /home/taeung/hello | grep /home/taeung/hello >> >> But it cause a side effect removing filename:linenr >> because the object file and source file has same name >> e.g. "/home/taueng/hello", "/home/taeung/hello.c" >> >> So more exactly grep -v as below >> to correctly remove the one line >> e.g. "/home/taeung/hello: file format elf64-x86-64" >> >> After: >> $ objdump -l -d -S -C /home/taeung/hello | grep /home/taeung/hello: > > What environment is this? I tried to reproduce this here but got > different results, and I had to figure out the 'perf record' you used, > you forgot to mention it, i.e. 'perf record ./hello' doesn't work, one > has to use 'perf record /full/path/to/hello', anyway, with my Sorry for my insufficient commit message. $ objdump -v GNU objdump (GNU Binutils) 2.28.51.20170309 ... $ pwd /home/taeung $ perf record ./hello hello, world ^C[ perf record: Woken up 3 times to write data ] [ perf record: Captured and wrote 0.676 MB perf.data (17380 samples) ] And, when running 'perf annotate', I internally checked the full objdump command line with gdb and printf. Here it is, objdump --start-address=0x0000000000400526 --stop-address=0x0000000000400567 -l -d --no-show-raw -S -C /home/taeung/hello 2>/dev/null|grep -v /home/taeung/hello|expand If just running the command, the output is like below. $ objdump --start-address=0x0000000000400526 --stop-address=0x0000000000400567 -l -d --no-show-raw -S -C /home/taeung/hello 2>/dev/null|grep -v /home/taeung/hello|expand|head -16 Disassembly of section .text: 0000000000400526
: main(): #include void main() 400526: push %rbp 400527: mov %rsp,%rbp 40052a: sub $0x10,%rsp { unsigned long long i; As you can see, grep -v eliminate "filename:linenr" such as /home/taeung/hello.c:3 and etc But if running the command line without grep -v as below, we can see "filename:linenr". $ objdump --start-address=0x0000000000400526 --stop-address=0x0000000000400567 -l -d --no-show-raw -S -C /home/taeung/hello 2>/dev/null|expand|head -16 /home/taeung/hello: file format elf64-x86-64 Disassembly of section .text: 0000000000400526
: main(): /home/taeung/hello.c:3 #include void main() 400526: push %rbp 400527: mov %rsp,%rbp 40052a: sub $0x10,%rsp /home/taeung/hello.c:6 So append ":" into grep -v as below. (it is contents of my patch) $ objdump --start-address=0x0000000000400526 --stop-address=0x0000000000400567 -l -d --no-show-raw -S -C /home/taeung/hello 2>/dev/null|grep -v /home/taeung/hello:|expand|head -16 Disassembly of section .text: 0000000000400526
: main(): /home/taeung/hello.c:3 #include void main() 400526: push %rbp 400527: mov %rsp,%rbp 40052a: sub $0x10,%rsp /home/taeung/hello.c:6 { I'll change the commit message to be more clearly.. > environemnt, which is: > > [root@jouet ~]# objdump -v > GNU objdump version 2.26.1-1.fc25 > > I end up getting: > > Sorted summary for file /home/acme/c/hello > ---------------------------------------------- > > 52.67 hello.c:7 > 47.32 hello.c:8 > Percent | Source code & Disassembly of hello for cycles:u (83555 samples) > ------------------------------------------------------------------------------- > : 00000000004004f6
: > : main(): > : #include > : > : int main(void) > : { > 0.00 : 4004f6: push %rbp > 0.00 : 4004f7: mov %rsp,%rbp > 0.00 : 4004fa: sub $0x10,%rsp > : unsigned long long i; > : printf("hello, world\n"); > 0.00 : 4004fe: mov $0x4005d0,%edi > 0.00 : 400503: callq 4003f0 > : while (i < (unsigned long long)-1) { > 0.00 : 400508: jmp 40052d > : i *= 171; > hello.c:8 8.66 : 40050a: mov -0x8(%rbp),%rdx > 0.01 : 40050e: mov %rdx,%rax > 0.00 : 400511: shl $0x3,%rax > hello.c:8 0.84 : 400515: add %rdx,%rax > hello.c:8 8.60 : 400518: add %rax,%rax > hello.c:8 3.46 : 40051b: add %rdx,%rax > hello.c:8 8.57 : 40051e: lea 0x0(,%rax,8),%rdx > 8.65 : 400526: add %rdx,%rax > 8.56 : 400529: mov %rax,-0x8(%rbp) > > > Which looks ok, i.e. since this version of objdump puts just the basename (hello.c), > that grep doesn't nuke it. > > But yeah, your patch is more robust, just fix what I described below, i.e. use > quotes in the grep -v bit, probably. > > - Arnaldo > >> Cc: Namhyung Kim >> Cc: Jiri Olsa >> Signed-off-by: Taeung Song >> --- >> tools/perf/util/annotate.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c >> index 63130ec..e49eb7e 100644 >> --- a/tools/perf/util/annotate.c >> +++ b/tools/perf/util/annotate.c >> @@ -1443,7 +1443,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na >> snprintf(command, sizeof(command), >> "%s %s%s --start-address=0x%016" PRIx64 >> " --stop-address=0x%016" PRIx64 >> - " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand", >> + " -l -d %s %s -C %s 2>/dev/null|grep -v %s: |expand", > > > Are you sure this works as you expect? I.e. both lines will have '%s:' Yes, if we do, we can exactly the only below line without the side effect IMHO. "/home/taeung/hello: file format elf64-x86-64" > that space just before the '|' will not be considered by grep > IIUC, Just append ":" into grep -v leaving out " " space before the '|' ? as below diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 273f21f..4d325cd 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1435,7 +1435,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na snprintf(command, sizeof(command), "%s %s%s --start-address=0x%016" PRIx64 " --stop-address=0x%016" PRIx64 - " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand", + " -l -d %s %s -C %s 2>/dev/null|grep -v %s:|expand", objdump_path ? objdump_path : "objdump", disassembler_style ? "-M " : "", disassembler_style ? disassembler_style : "", Thanks, Taeung