Re: [PATCH 4/4] perf annotate: More exactly grep -v of the objdump command

[Taeung Song <taeung.dev@gmail.com>]

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>:
>>     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>:
main():
#include <stdio.h>

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>:
main():
/home/taeung/hello.c:3
#include <stdio.h>

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>:
main():
/home/taeung/hello.c:3
#include <stdio.h>

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>:
>          :      main():
>          :      #include <stdio.h>
>          :
>          :      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 <puts@plt>
>          :              while (i < (unsigned long long)-1) {
>     0.00 :        400508:       jmp    40052d <main+0x37>
>          :                      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)
> <SNIP>
>
> 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 <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  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