[PATCH 0/4] perf annotate: Fixes for line numbers and Introduce source_code

[PATCH 0/4] perf annotate: Fixes for line numbers and Introduce source_code

[Taeung Song]

Hi, :)

Currently perf-annotate have several problems.

  - Wrong line numbers on perf-annotate (both stdio and TUI)
  - Wrong sum of overhead(percent) matching source lines 
  - It's so confusing that the output is mixed with
    both source code and assembly code. (related to depending on 'objdump -S') 

So fix them regarding wrong line numbers
and Introduce new source_code collecting actual code,
not depending on 'objdump -S'.

In near future, I have a plan for new reable annotate view
base on source code (per function(sym)) using the 'struct source_code'. :)

I'd appreciate some feedback.

(Current v1 don't consider improvement of performance of annotate
and don't make code of annotate more compact so I'll consider them in v2) 

Thanks,
Taeung

Taeung Song (4):
  perf annotate: Remove needless regular expression for filename:linenr
  perf annotate: Align filename:linenr and more correct summary
  perf annotate: Change the method counting line numbers
  perf annotate: Introduce source_code to collect actual code

 tools/perf/util/annotate.c | 174 +++++++++++++++++++++++++++++++++++----------
 tools/perf/util/annotate.h |   7 ++
 2 files changed, 144 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH 1/4] perf annotate: Remove needless regular expression for filename:linenr

[Taeung Song]

The commit e592488c01d5 ("perf annotate: Support source line
numbers in annotate") support source line numbers in annotate.

But we can get filename:line number by symbol__get_source_line()
Furthermore, the way can't exactly match source code lines
to actual 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 annotate with the way about regmatch
is as below.

  4   Disassembly of section .text:

  6   0000000000400966 <get_cond_maxprice>:
  7   get_cond_maxprice():
  26  };

  28  unsigned int limited_wgt;

  30  unsigned int get_cond_maxprice(int wgt)
  31  {

The root cause is from objdump -S.
Because the source code of objdump used to print more code lines
than actual one code line according to a particular line number.
In the near future, I'll fix the problem about line numbers.

Cc: Andi Kleen <ak@linux.intel.com>
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 | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 273f21f..bc54e41 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -19,14 +19,12 @@
 #include "evsel.h"
 #include "block-range.h"
 #include "arch/common.h"
-#include <regex.h>
 #include <pthread.h>
 #include <linux/bitops.h>
 #include <sys/utsname.h>
 
 const char 	*disassembler_style;
 const char	*objdump_path;
-static regex_t	 file_lineno;
 
 static struct ins_ops *ins__find(struct arch *arch, const char *name);
 static void ins__sort(struct arch *arch);
@@ -1151,7 +1149,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
 	size_t line_len;
 	s64 line_ip, offset = -1;
-	regmatch_t match[2];
 
 	if (getline(&line, &line_len, file) < 0)
 		return -1;
@@ -1169,12 +1166,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	line_ip = -1;
 	parsed_line = line;
 
-	/* /filename:linenr ? Save line number and ignore. */
-	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
-		*line_nr = atoi(line + match[1].rm_so);
-		return 0;
-	}
-
 	/*
 	 * Strip leading spaces:
 	 */
@@ -1235,11 +1226,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	return 0;
 }
 
-static __attribute__((constructor)) void symbol__init_regexpr(void)
-{
-	regcomp(&file_lineno, "^/[^:]+:([0-9]+)", REG_EXTENDED);
-}
-
 static void delete_last_nop(struct symbol *sym)
 {
 	struct annotation *notes = symbol__annotation(sym);
@@ -1435,7 +1421,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",
+		 " -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
 		 objdump_path ? objdump_path : "objdump",
 		 disassembler_style ? "-M " : "",
 		 disassembler_style ? disassembler_style : "",
-- 
2.7.4

Re: [PATCH 1/4] perf annotate: Remove needless regular expression for filename:linenr

[Namhyung Kim]

Hi Taeung,

On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
> The commit e592488c01d5 ("perf annotate: Support source line
> numbers in annotate") support source line numbers in annotate.
>
> But we can get filename:line number by symbol__get_source_line()
> Furthermore, the way can't exactly match source code lines
> to actual 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 annotate with the way about regmatch
> is as below.
>
>   4   Disassembly of section .text:
>
>   6   0000000000400966 <get_cond_maxprice>:
>   7   get_cond_maxprice():
>   26  };
>
>   28  unsigned int limited_wgt;
>
>   30  unsigned int get_cond_maxprice(int wgt)
>   31  {
>
> The root cause is from objdump -S.
> Because the source code of objdump used to print more code lines
> than actual one code line according to a particular line number.
> In the near future, I'll fix the problem about line numbers.

So what's the purpose of this patch?  You just removed the line
number from the output, it changed behavior for what?  If you want
to fix a problem of line number, please do it here..

Thanks,
Namhyung


>
> Cc: Andi Kleen <ak@linux.intel.com>
> 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 | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 273f21f..bc54e41 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -19,14 +19,12 @@
>  #include "evsel.h"
>  #include "block-range.h"
>  #include "arch/common.h"
> -#include <regex.h>
>  #include <pthread.h>
>  #include <linux/bitops.h>
>  #include <sys/utsname.h>
>
>  const char     *disassembler_style;
>  const char     *objdump_path;
> -static regex_t  file_lineno;
>
>  static struct ins_ops *ins__find(struct arch *arch, const char *name);
>  static void ins__sort(struct arch *arch);
> @@ -1151,7 +1149,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>         char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
>         size_t line_len;
>         s64 line_ip, offset = -1;
> -       regmatch_t match[2];
>
>         if (getline(&line, &line_len, file) < 0)
>                 return -1;
> @@ -1169,12 +1166,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>         line_ip = -1;
>         parsed_line = line;
>
> -       /* /filename:linenr ? Save line number and ignore. */
> -       if (regexec(&file_lineno, line, 2, match, 0) == 0) {
> -               *line_nr = atoi(line + match[1].rm_so);
> -               return 0;
> -       }
> -
>         /*
>          * Strip leading spaces:
>          */
> @@ -1235,11 +1226,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>         return 0;
>  }
>
> -static __attribute__((constructor)) void symbol__init_regexpr(void)
> -{
> -       regcomp(&file_lineno, "^/[^:]+:([0-9]+)", REG_EXTENDED);
> -}
> -
>  static void delete_last_nop(struct symbol *sym)
>  {
>         struct annotation *notes = symbol__annotation(sym);
> @@ -1435,7 +1421,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",
> +                " -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
>                  objdump_path ? objdump_path : "objdump",
>                  disassembler_style ? "-M " : "",
>                  disassembler_style ? disassembler_style : "",
> --
> 2.7.4
>



-- 
Thanks,
Namhyung

Re: [PATCH 1/4] perf annotate: Remove needless regular expression for filename:linenr

[Taeung Song]

Hi Namhyung,

On 02/22/2017 07:47 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
>> The commit e592488c01d5 ("perf annotate: Support source line
>> numbers in annotate") support source line numbers in annotate.
>>
>> But we can get filename:line number by symbol__get_source_line()
>> Furthermore, the way can't exactly match source code lines
>> to actual 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 annotate with the way about regmatch
>> is as below.
>>
>>   4   Disassembly of section .text:
>>
>>   6   0000000000400966 <get_cond_maxprice>:
>>   7   get_cond_maxprice():
>>   26  };
>>
>>   28  unsigned int limited_wgt;
>>
>>   30  unsigned int get_cond_maxprice(int wgt)
>>   31  {
>>
>> The root cause is from objdump -S.
>> Because the source code of objdump used to print more code lines
>> than actual one code line according to a particular line number.
>> In the near future, I'll fix the problem about line numbers.
>
> So what's the purpose of this patch?  You just removed the line
> number from the output, it changed behavior for what?  If you want
> to fix a problem of line number, please do it here..
>
> Thanks,
> Namhyung
>
>

The purpose of this patch was to remove needless code about line number.
To avoid big size of commit, I made several patches as parts..

But as you said I also think it is better to remake this patch
preferentially centering important points of the patchset!

Thanks
Taeung

[PATCH 2/4] perf annotate: Align filename:linenr and more correct summary

[Taeung Song]

In the stdio interface, currently 'filename:linenr' infos
are confusedly printed in the intervals of assembly code.
So fix it.

The cause was a 0.5% filter of if statement. After fixed,
additionally summary of overhead per srcline is more correct.

Before:

    # perf annotate --stdio -l

  Sorted summary for file /home/taeung/workspace/perf-test/test
  ----------------------------------------------

     36.57 test.c:38
     28.72 test.c:37

  ...

   Percent |      Source code & Disassembly of test ...

  ...

      0.21 :        400816:       push   %rbp
   test.c:26    1.86 :         400817:       mov    %rsp,%rbp
      0.21 :        40081a:       mov    %edi,-0x24(%rbp)
      0.21 :        40081d:       mov    %rsi,-0x30(%rbp)

After:

    # perf annotate --stdio -l

  Sorted summary for file /home/taeung/workspace/perf-test/test
  ----------------------------------------------

     37.40 test.c:38
     29.34 test.c:37

  ...

   Percent |      Source code & Disassembly of test ...

  ...

   test.c:26
      0.21 :        400816:       push   %rbp
      1.86 :        400817:       mov    %rsp,%rbp
      0.21 :        40081a:       mov    %edi,-0x24(%rbp)
      0.21 :        40081d:       mov    %rsi,-0x30(%rbp)

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 | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index bc54e41..9d0aa50 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1002,7 +1002,6 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
 		      int max_lines, struct disasm_line *queue)
 {
 	static const char *prev_line;
-	static const char *prev_color;
 
 	if (dl->offset != -1) {
 		const char *path = NULL;
@@ -1059,17 +1058,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
 
 		color = get_percent_color(max_percent);
 
-		/*
-		 * Also color the filename and line if needed, with
-		 * the same color than the percentage. Don't print it
-		 * twice for close colored addr with the same filename:line
-		 */
 		if (path) {
-			if (!prev_line || strcmp(prev_line, path)
-				       || color != prev_color) {
-				color_fprintf(stdout, color, " %s", path);
+			if (!prev_line || strcmp(prev_line, path)) {
+				fprintf(stdout, " %s\n", path);
 				prev_line = path;
-				prev_color = color;
 			}
 		}
 
@@ -1650,14 +1642,9 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 				percent_max = src_line->samples[k].percent;
 		}
 
-		if (percent_max <= 0.5)
-			goto next;
-
 		offset = start + i;
 		src_line->path = get_srcline(map->dso, offset, NULL, false);
 		insert_source_line(&tmp_root, src_line);
-
-	next:
 		src_line = (void *)src_line + sizeof_src_line;
 	}
 
-- 
2.7.4

Re: [PATCH 2/4] perf annotate: Align filename:linenr and more correct summary

[Namhyung Kim]

On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
> In the stdio interface, currently 'filename:linenr' infos
> are confusedly printed in the intervals of assembly code.
> So fix it.
>
> The cause was a 0.5% filter of if statement. After fixed,
> additionally summary of overhead per srcline is more correct.

This patch does two things and fails to explain why.

Thanks,
Namhyung


>
> Before:
>
>     # perf annotate --stdio -l
>
>   Sorted summary for file /home/taeung/workspace/perf-test/test
>   ----------------------------------------------
>
>      36.57 test.c:38
>      28.72 test.c:37
>
>   ...
>
>    Percent |      Source code & Disassembly of test ...
>
>   ...
>
>       0.21 :        400816:       push   %rbp
>    test.c:26    1.86 :         400817:       mov    %rsp,%rbp
>       0.21 :        40081a:       mov    %edi,-0x24(%rbp)
>       0.21 :        40081d:       mov    %rsi,-0x30(%rbp)
>
> After:
>
>     # perf annotate --stdio -l
>
>   Sorted summary for file /home/taeung/workspace/perf-test/test
>   ----------------------------------------------
>
>      37.40 test.c:38
>      29.34 test.c:37
>
>   ...
>
>    Percent |      Source code & Disassembly of test ...
>
>   ...
>
>    test.c:26
>       0.21 :        400816:       push   %rbp
>       1.86 :        400817:       mov    %rsp,%rbp
>       0.21 :        40081a:       mov    %edi,-0x24(%rbp)
>       0.21 :        40081d:       mov    %rsi,-0x30(%rbp)
>
> 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 | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index bc54e41..9d0aa50 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1002,7 +1002,6 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>                       int max_lines, struct disasm_line *queue)
>  {
>         static const char *prev_line;
> -       static const char *prev_color;
>
>         if (dl->offset != -1) {
>                 const char *path = NULL;
> @@ -1059,17 +1058,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>
>                 color = get_percent_color(max_percent);
>
> -               /*
> -                * Also color the filename and line if needed, with
> -                * the same color than the percentage. Don't print it
> -                * twice for close colored addr with the same filename:line
> -                */
>                 if (path) {
> -                       if (!prev_line || strcmp(prev_line, path)
> -                                      || color != prev_color) {
> -                               color_fprintf(stdout, color, " %s", path);
> +                       if (!prev_line || strcmp(prev_line, path)) {
> +                               fprintf(stdout, " %s\n", path);
>                                 prev_line = path;
> -                               prev_color = color;
>                         }
>                 }
>
> @@ -1650,14 +1642,9 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
>                                 percent_max = src_line->samples[k].percent;
>                 }
>
> -               if (percent_max <= 0.5)
> -                       goto next;
> -
>                 offset = start + i;
>                 src_line->path = get_srcline(map->dso, offset, NULL, false);
>                 insert_source_line(&tmp_root, src_line);
> -
> -       next:
>                 src_line = (void *)src_line + sizeof_src_line;
>         }
>
> --
> 2.7.4
>



-- 
Thanks,
Namhyung

Re: [PATCH 2/4] perf annotate: Align filename:linenr and more correct summary

[Namhyung Kim]

On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
> In the stdio interface, currently 'filename:linenr' infos
> are confusedly printed in the intervals of assembly code.
> So fix it.
>
> The cause was a 0.5% filter of if statement. After fixed,
> additionally summary of overhead per srcline is more correct.
>
> Before:
>
>     # perf annotate --stdio -l
>
>   Sorted summary for file /home/taeung/workspace/perf-test/test
>   ----------------------------------------------
>
>      36.57 test.c:38
>      28.72 test.c:37
>
>   ...
>
>    Percent |      Source code & Disassembly of test ...
>
>   ...
>
>       0.21 :        400816:       push   %rbp
>    test.c:26    1.86 :         400817:       mov    %rsp,%rbp
>       0.21 :        40081a:       mov    %edi,-0x24(%rbp)
>       0.21 :        40081d:       mov    %rsi,-0x30(%rbp)
>
> After:
>
>     # perf annotate --stdio -l
>
>   Sorted summary for file /home/taeung/workspace/perf-test/test
>   ----------------------------------------------
>
>      37.40 test.c:38
>      29.34 test.c:37
>
>   ...
>
>    Percent |      Source code & Disassembly of test ...
>
>   ...
>
>    test.c:26
>       0.21 :        400816:       push   %rbp
>       1.86 :        400817:       mov    %rsp,%rbp
>       0.21 :        40081a:       mov    %edi,-0x24(%rbp)
>       0.21 :        40081d:       mov    %rsi,-0x30(%rbp)

I guess it's just a problem of a missing newline..

Thanks,
Namhyung

Re: [PATCH 2/4] perf annotate: Align filename:linenr and more correct summary

[Taeung Song]

On 02/22/2017 08:22 PM, Namhyung Kim wrote:
> On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
>> In the stdio interface, currently 'filename:linenr' infos
>> are confusedly printed in the intervals of assembly code.
>> So fix it.
>>
>> The cause was a 0.5% filter of if statement. After fixed,
>> additionally summary of overhead per srcline is more correct.
>>
>> Before:
>>
>>     # perf annotate --stdio -l
>>
>>   Sorted summary for file /home/taeung/workspace/perf-test/test
>>   ----------------------------------------------
>>
>>      36.57 test.c:38
>>      28.72 test.c:37
>>
>>   ...
>>
>>    Percent |      Source code & Disassembly of test ...
>>
>>   ...
>>
>>       0.21 :        400816:       push   %rbp
>>    test.c:26    1.86 :         400817:       mov    %rsp,%rbp
>>       0.21 :        40081a:       mov    %edi,-0x24(%rbp)
>>       0.21 :        40081d:       mov    %rsi,-0x30(%rbp)
>>
>> After:
>>
>>     # perf annotate --stdio -l
>>
>>   Sorted summary for file /home/taeung/workspace/perf-test/test
>>   ----------------------------------------------
>>
>>      37.40 test.c:38
>>      29.34 test.c:37
>>
>>   ...
>>
>>    Percent |      Source code & Disassembly of test ...
>>
>>   ...
>>
>>    test.c:26
>>       0.21 :        400816:       push   %rbp
>>       1.86 :        400817:       mov    %rsp,%rbp
>>       0.21 :        40081a:       mov    %edi,-0x24(%rbp)
>>       0.21 :        40081d:       mov    %rsi,-0x30(%rbp)
>
> I guess it's just a problem of a missing newline..
>

I think the problem is not only from a missing newline but also
from 0.5 filtering if statement.

For example,
If just appending new line, the output is as below

        0.21 :        400816:       push   %rbp
     test.c:26
        1.86 :        400817:       mov    %rsp,%rbp
        0.21 :        40081a:       mov    %edi,-0x24(%rbp)
        0.21 :        40081d:       mov    %rsi,-0x30(%rbp)

The reason of the wrong sorting is that only 400817 is matched with 
test.c:26
And the root cause is a if statement filtering smaller values than 0.5.
The if statement prevent other addresses that are less than 0.5
from matching test.c:26

So I eliminated it.

-               if (percent_max <= 0.5)
-                       goto next;

But 400816, 400817, 40081a and 40081d addresses should be matched
with test.c:26. So I think it is better to show as below

     test.c:26
        0.21 :        400816:       push   %rbp
        1.86 :        400817:       mov    %rsp,%rbp
        0.21 :        40081a:       mov    %edi,-0x24(%rbp)
        0.21 :        40081d:       mov    %rsi,-0x30(%rbp)


And I think it is better to rewrite this commit title and message..
I'll change this patch as v2 to clearly understand problem and solution.


Thanks,
Teaung

[PATCH 3/4] perf annotate: Change the method counting line numbers

[Taeung Song]

Currently line numbers are wrong due to
just counting according to output of 'objdump -S'

So remove needless local variables (line_nr, lineno)
and get correct line numbers according to each address
in symbol__get_source_line().

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 | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 9d0aa50..acd500b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -812,7 +812,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 }
 
 static struct disasm_line *disasm_line__new(s64 offset, char *line,
-					    size_t privsize, int line_nr,
+					    size_t privsize,
 					    struct arch *arch,
 					    struct map *map)
 {
@@ -821,7 +821,7 @@ static struct disasm_line *disasm_line__new(s64 offset, char *line,
 	if (dl != NULL) {
 		dl->offset = offset;
 		dl->line = strdup(line);
-		dl->line_nr = line_nr;
+
 		if (dl->line == NULL)
 			goto out_delete;
 
@@ -1133,8 +1133,7 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
  */
 static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 				      struct arch *arch,
-				      FILE *file, size_t privsize,
-				      int *line_nr)
+				      FILE *file, size_t privsize)
 {
 	struct annotation *notes = symbol__annotation(sym);
 	struct disasm_line *dl;
@@ -1188,9 +1187,8 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 			parsed_line = tmp2 + 1;
 	}
 
-	dl = disasm_line__new(offset, parsed_line, privsize, *line_nr, arch, map);
+	dl = disasm_line__new(offset, parsed_line, privsize, arch, map);
 	free(line);
-	(*line_nr)++;
 
 	if (dl == NULL)
 		return -1;
@@ -1338,7 +1336,6 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 	struct kcore_extract kce;
 	bool delete_extract = false;
 	int stdout_fd[2];
-	int lineno = 0;
 	int nline;
 	pid_t pid;
 	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
@@ -1460,8 +1457,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 
 	nline = 0;
 	while (!feof(file)) {
-		if (symbol__parse_objdump_line(sym, map, arch, file, privsize,
-			    &lineno) < 0)
+		if (symbol__parse_objdump_line(sym, map, arch, file, privsize) < 0)
 			break;
 		nline++;
 	}
@@ -1631,6 +1627,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 	for (i = 0; i < len; i++) {
 		u64 offset;
 		double percent_max = 0.0;
+		struct disasm_line *dl;
 
 		src_line->nr_pcnt = nr_pcnt;
 
@@ -1645,6 +1642,19 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 		offset = start + i;
 		src_line->path = get_srcline(map->dso, offset, NULL, false);
 		insert_source_line(&tmp_root, src_line);
+
+		if (!strcmp(src_line->path, SRCLINE_UNKNOWN))
+			goto next;
+
+		list_for_each_entry(dl, &notes->src->source, node) {
+			if ((dl->offset + start) == offset) {
+				char *sep = strchr(src_line->path, ':');
+
+				if (sep)
+					dl->line_nr = strtoul(++sep, NULL, 0);
+			}
+		}
+	next:
 		src_line = (void *)src_line + sizeof_src_line;
 	}
 
-- 
2.7.4

[PATCH 4/4] perf annotate: Introduce source_code to collect actual code

[Taeung Song]

The output of perf-annotate has a problem.
It is so confusing that the output is mixed with
both source code and assembly code.
IMHO, we need readable annotate view based on source code,
not mixed view. (not depending on 'objdump -S')

And to do that, we can collect actual source code per function(sym)
using addr2line() and we can handle 'struct source_code'
that contains each line of code.

In near future, it would be used for new annotate view based on
actual source code per function(sym).

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 | 117 +++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/annotate.h |   7 +++
 2 files changed, 124 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index acd500b..93abc1e 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1590,6 +1590,116 @@ static void symbol__free_source_line(struct symbol *sym, int len)
 	zfree(&notes->src->lines);
 }
 
+static int symbol__free_source_code(struct symbol *sym)
+{
+	struct annotation *notes = symbol__annotation(sym);
+	struct source_code *pos, *tmp;
+
+	if (&notes->src->code == NULL)
+		return -1;
+
+	list_for_each_entry_safe(pos, tmp, &notes->src->code, node) {
+		list_del(&pos->node);
+		zfree(&pos->code_line);
+		free(pos);
+	}
+	return 0;
+}
+
+static int parse_srcline(char *srcline, char **path, int *line_nr)
+{
+	char *sep;
+
+	if (!strcmp(srcline, SRCLINE_UNKNOWN))
+		return -1;
+
+	sep = strchr(srcline, ':');
+	if (sep) {
+		*sep = '\0';
+		*path = srcline;
+		*line_nr = strtoul(++sep, NULL, 0);
+	} else
+		return -1;
+
+	return 0;
+}
+
+static bool symbol__get_source_code(struct symbol *sym, struct map *map)
+{
+	FILE *file;
+	int first_linenr, start_linenr, end_linenr;
+	size_t len;
+	char *line = NULL, *path, *start_srcline, *end_srcline;
+	u64 start = map__rip_2objdump(map, sym->start);
+	u64 end = map__rip_2objdump(map, sym->end - 1);
+	bool bef_fullpath = srcline_full_filename;
+	bool ret = false;
+	struct annotation *notes = symbol__annotation(sym);
+
+	srcline_full_filename = true;
+	start_srcline = get_srcline(map->dso, start, NULL, false);
+	end_srcline = get_srcline(map->dso, end, NULL, false);
+	srcline_full_filename = bef_fullpath;
+
+	if (parse_srcline(start_srcline, &path, &start_linenr) < 0)
+		return false;
+	if (parse_srcline(end_srcline, &path, &end_linenr) < 0)
+		return false;
+
+	/* To read a function header for the sym */
+	if (start_linenr > 4)
+		first_linenr = start_linenr - 4;
+	else
+		first_linenr = 1;
+
+	if (access(path, R_OK) != 0)
+		return false;
+
+	file = fopen(path, "r");
+	if (!file)
+		return false;
+
+	INIT_LIST_HEAD(&notes->src->code);
+
+	while (!feof(file)) {
+		int nr;
+		char *c, *parsed_line;
+		struct source_code *code;
+
+		if (getline(&line, &len, file) < 0) {
+			symbol__free_source_code(sym);
+			break;
+		}
+
+		if (++nr < first_linenr)
+			continue;
+
+		parsed_line = rtrim(line);
+		c = strchr(parsed_line, '\n');
+		if (c)
+			*c = '\0';
+		code = zalloc(sizeof(*code));
+		if (!code) {
+			symbol__free_source_code(sym);
+			break;
+		}
+		code->nr = nr;
+		code->code_line = strdup(parsed_line);
+		list_add_tail(&code->node, &notes->src->code);
+
+		if (nr == end_linenr) {
+			ret = true;
+			break;
+		}
+	}
+
+	free(line);
+	fclose(file);
+	free_srcline(start_srcline);
+	free_srcline(end_srcline);
+	return ret;
+}
+
 /* Get the filename:line for the colored entries */
 static int symbol__get_source_line(struct symbol *sym, struct map *map,
 				   struct perf_evsel *evsel,
@@ -1862,6 +1972,7 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 	struct dso *dso = map->dso;
 	struct rb_root source_line = RB_ROOT;
 	u64 len;
+	bool has_code = false;
 
 	if (symbol__disassemble(sym, map, perf_evsel__env_arch(evsel), 0) < 0)
 		return -1;
@@ -1874,11 +1985,17 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 		print_summary(&source_line, dso->long_name);
 	}
 
+	if (symbol_conf.annotate_src)
+		has_code = symbol__get_source_code(sym, map);
+
 	symbol__annotate_printf(sym, map, evsel, full_paths,
 				min_pcnt, max_lines, 0);
 	if (print_lines)
 		symbol__free_source_line(sym, len);
 
+	if (has_code)
+		symbol__free_source_code(sym);
+
 	disasm__purge(&symbol__annotation(sym)->src->source);
 
 	return 0;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 09776b5..7231f46 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -95,6 +95,12 @@ struct cyc_hist {
 	u16	reset;
 };
 
+struct source_code {
+	struct list_head node;
+	int nr;
+	char *code_line;
+};
+
 struct source_line_samples {
 	double		percent;
 	double		percent_sum;
@@ -123,6 +129,7 @@ struct source_line {
  */
 struct annotated_source {
 	struct list_head   source;
+	struct list_head   code;
 	struct source_line *lines;
 	int    		   nr_histograms;
 	size_t		   sizeof_sym_hist;
-- 
2.7.4

Re: [PATCH 4/4] perf annotate: Introduce source_code to collect actual code

[Namhyung Kim]

On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
> The output of perf-annotate has a problem.
> It is so confusing that the output is mixed with
> both source code and assembly code.
> IMHO, we need readable annotate view based on source code,
> not mixed view. (not depending on 'objdump -S')
>
> And to do that, we can collect actual source code per function(sym)
> using addr2line() and we can handle 'struct source_code'
> that contains each line of code.
>
> In near future, it would be used for new annotate view based on
> actual source code per function(sym).

I think this is just a preparation so you'd be better sending it with
your new annotate view patchset later.

Thanks,
Namhyung

>
> 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 | 117 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/annotate.h |   7 +++
>  2 files changed, 124 insertions(+)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index acd500b..93abc1e 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1590,6 +1590,116 @@ static void symbol__free_source_line(struct symbol *sym, int len)
>         zfree(&notes->src->lines);
>  }
>
> +static int symbol__free_source_code(struct symbol *sym)
> +{
> +       struct annotation *notes = symbol__annotation(sym);
> +       struct source_code *pos, *tmp;
> +
> +       if (&notes->src->code == NULL)
> +               return -1;
> +
> +       list_for_each_entry_safe(pos, tmp, &notes->src->code, node) {
> +               list_del(&pos->node);
> +               zfree(&pos->code_line);
> +               free(pos);
> +       }
> +       return 0;
> +}
> +
> +static int parse_srcline(char *srcline, char **path, int *line_nr)
> +{
> +       char *sep;
> +
> +       if (!strcmp(srcline, SRCLINE_UNKNOWN))
> +               return -1;
> +
> +       sep = strchr(srcline, ':');
> +       if (sep) {
> +               *sep = '\0';
> +               *path = srcline;
> +               *line_nr = strtoul(++sep, NULL, 0);
> +       } else
> +               return -1;
> +
> +       return 0;
> +}
> +
> +static bool symbol__get_source_code(struct symbol *sym, struct map *map)
> +{
> +       FILE *file;
> +       int first_linenr, start_linenr, end_linenr;
> +       size_t len;
> +       char *line = NULL, *path, *start_srcline, *end_srcline;
> +       u64 start = map__rip_2objdump(map, sym->start);
> +       u64 end = map__rip_2objdump(map, sym->end - 1);
> +       bool bef_fullpath = srcline_full_filename;
> +       bool ret = false;
> +       struct annotation *notes = symbol__annotation(sym);
> +
> +       srcline_full_filename = true;
> +       start_srcline = get_srcline(map->dso, start, NULL, false);
> +       end_srcline = get_srcline(map->dso, end, NULL, false);
> +       srcline_full_filename = bef_fullpath;
> +
> +       if (parse_srcline(start_srcline, &path, &start_linenr) < 0)
> +               return false;
> +       if (parse_srcline(end_srcline, &path, &end_linenr) < 0)
> +               return false;
> +
> +       /* To read a function header for the sym */
> +       if (start_linenr > 4)
> +               first_linenr = start_linenr - 4;
> +       else
> +               first_linenr = 1;
> +
> +       if (access(path, R_OK) != 0)
> +               return false;
> +
> +       file = fopen(path, "r");
> +       if (!file)
> +               return false;
> +
> +       INIT_LIST_HEAD(&notes->src->code);
> +
> +       while (!feof(file)) {
> +               int nr;
> +               char *c, *parsed_line;
> +               struct source_code *code;
> +
> +               if (getline(&line, &len, file) < 0) {
> +                       symbol__free_source_code(sym);
> +                       break;
> +               }
> +
> +               if (++nr < first_linenr)
> +                       continue;
> +
> +               parsed_line = rtrim(line);
> +               c = strchr(parsed_line, '\n');
> +               if (c)
> +                       *c = '\0';
> +               code = zalloc(sizeof(*code));
> +               if (!code) {
> +                       symbol__free_source_code(sym);
> +                       break;
> +               }
> +               code->nr = nr;
> +               code->code_line = strdup(parsed_line);
> +               list_add_tail(&code->node, &notes->src->code);
> +
> +               if (nr == end_linenr) {
> +                       ret = true;
> +                       break;
> +               }
> +       }
> +
> +       free(line);
> +       fclose(file);
> +       free_srcline(start_srcline);
> +       free_srcline(end_srcline);
> +       return ret;
> +}
> +
>  /* Get the filename:line for the colored entries */
>  static int symbol__get_source_line(struct symbol *sym, struct map *map,
>                                    struct perf_evsel *evsel,
> @@ -1862,6 +1972,7 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
>         struct dso *dso = map->dso;
>         struct rb_root source_line = RB_ROOT;
>         u64 len;
> +       bool has_code = false;
>
>         if (symbol__disassemble(sym, map, perf_evsel__env_arch(evsel), 0) < 0)
>                 return -1;
> @@ -1874,11 +1985,17 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
>                 print_summary(&source_line, dso->long_name);
>         }
>
> +       if (symbol_conf.annotate_src)
> +               has_code = symbol__get_source_code(sym, map);
> +
>         symbol__annotate_printf(sym, map, evsel, full_paths,
>                                 min_pcnt, max_lines, 0);
>         if (print_lines)
>                 symbol__free_source_line(sym, len);
>
> +       if (has_code)
> +               symbol__free_source_code(sym);
> +
>         disasm__purge(&symbol__annotation(sym)->src->source);
>
>         return 0;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 09776b5..7231f46 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -95,6 +95,12 @@ struct cyc_hist {
>         u16     reset;
>  };
>
> +struct source_code {
> +       struct list_head node;
> +       int nr;
> +       char *code_line;
> +};
> +
>  struct source_line_samples {
>         double          percent;
>         double          percent_sum;
> @@ -123,6 +129,7 @@ struct source_line {
>   */
>  struct annotated_source {
>         struct list_head   source;
> +       struct list_head   code;
>         struct source_line *lines;
>         int                nr_histograms;
>         size_t             sizeof_sym_hist;
> --
> 2.7.4
>



-- 
Thanks,
Namhyung

Re: [PATCH 4/4] perf annotate: Introduce source_code to collect actual code

[Taeung Song]

On 02/22/2017 08:27 PM, Namhyung Kim wrote:
> On Wed, Feb 22, 2017 at 7:08 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
>> The output of perf-annotate has a problem.
>> It is so confusing that the output is mixed with
>> both source code and assembly code.
>> IMHO, we need readable annotate view based on source code,
>> not mixed view. (not depending on 'objdump -S')
>>
>> And to do that, we can collect actual source code per function(sym)
>> using addr2line() and we can handle 'struct source_code'
>> that contains each line of code.
>>
>> In near future, it would be used for new annotate view based on
>> actual source code per function(sym).
>
> I think this is just a preparation so you'd be better sending it with
> your new annotate view patchset later.
>
> Thanks,
> Namhyung

I got it!
I thought it is needed to separate a big commit into several commits
so I sent this patch before including new annotate view.

But as you said, it seems like just a preparation without important 
point(new view).
So I'll send v2 with new annotate view !

Thanks,
Taeung

Re: [PATCH 4/4] perf annotate: Introduce source_code to collect actual code

[Ravi Bangoria]

Hi Taeung,

On Wednesday 22 February 2017 03:38 PM, Taeung Song wrote:
> +	INIT_LIST_HEAD(&notes->src->code);
> +
> +	while (!feof(file)) {
> +		int nr;
> +		char *c, *parsed_line;
> +		struct source_code *code;
> +
> +		if (getline(&line, &len, file) < 0) {
> +			symbol__free_source_code(sym);
> +			break;
> +		}
> +
> +		if (++nr < first_linenr)

Please initialize variable nr. I got a compilation error:

util/annotate.c: In function ‘symbol__tty_annotate’:
util/annotate.c:1674:6: error: ‘nr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   if (++nr < first_linenr)
      ^
util/annotate.c:1665:7: note: ‘nr’ was declared here
   int nr;
       ^

Ravi