[PATCH v2 0/6] perf annotate: Misc fixes / improvements

[PATCH v2 0/6] perf annotate: Misc fixes / improvements

[Ravi Bangoria]

Few fixes / improvements related to perf annotate.

v1: http://lore.kernel.org/r/20200117092612.30874-1-ravi.bangoria@linux.ibm.com

v1->v2:
 - Split [PATCH v1 1/3] into two patches.
 - Patch 5 and patch 6 are new.

Ravi Bangoria (6):
  perf annotate: Remove privsize from symbol__annotate() args
  perf annotate: Simplify disasm_line allocation and freeing code
  perf annotate: Align struct annotate_args
  perf annotate: Fix segfault with source toggle
  perf annotate: Make few functions static
  perf annotate: Get rid of annotation->nr_jumps

 tools/perf/builtin-top.c     |   2 +-
 tools/perf/ui/gtk/annotate.c |   2 +-
 tools/perf/util/annotate.c   | 116 +++++++++++++----------------------
 tools/perf/util/annotate.h   |   8 +--
 4 files changed, 47 insertions(+), 81 deletions(-)

-- 
2.24.1

[PATCH v2 1/6] perf annotate: Remove privsize from symbol__annotate() args

[Ravi Bangoria]

privsize is passed as 0 from all the symbol__annotate() callers.
Remove it from argument list.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/builtin-top.c     | 2 +-
 tools/perf/ui/gtk/annotate.c | 2 +-
 tools/perf/util/annotate.c   | 7 ++++---
 tools/perf/util/annotate.h   | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 8affcab75604..3e37747364e0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -143,7 +143,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 		return err;
 	}
 
-	err = symbol__annotate(&he->ms, evsel, 0, &top->annotation_opts, NULL);
+	err = symbol__annotate(&he->ms, evsel, &top->annotation_opts, NULL);
 	if (err == 0) {
 		top->sym_filter_entry = he;
 	} else {
diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
index 22cc240f7371..35f9641bf670 100644
--- a/tools/perf/ui/gtk/annotate.c
+++ b/tools/perf/ui/gtk/annotate.c
@@ -174,7 +174,7 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 	if (ms->map->dso->annotate_warned)
 		return -1;
 
-	err = symbol__annotate(ms, evsel, 0, &annotation__default_options, NULL);
+	err = symbol__annotate(ms, evsel, &annotation__default_options, NULL);
 	if (err) {
 		char msg[BUFSIZ];
 		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ca73fb74ad03..ea70bc050bce 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2149,9 +2149,10 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
 	annotation__calc_percent(notes, evsel, symbol__size(sym));
 }
 
-int symbol__annotate(struct map_symbol *ms, struct evsel *evsel, size_t privsize,
+int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 		     struct annotation_options *options, struct arch **parch)
 {
+	size_t privsize = 0;
 	struct symbol *sym = ms->sym;
 	struct annotation *notes = symbol__annotation(sym);
 	struct annotate_args args = {
@@ -2790,7 +2791,7 @@ int symbol__tty_annotate(struct map_symbol *ms, struct evsel *evsel,
 	struct symbol *sym = ms->sym;
 	struct rb_root source_line = RB_ROOT;
 
-	if (symbol__annotate(ms, evsel, 0, opts, NULL) < 0)
+	if (symbol__annotate(ms, evsel, opts, NULL) < 0)
 		return -1;
 
 	symbol__calc_percent(sym, evsel);
@@ -3070,7 +3071,7 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
 	if (perf_evsel__is_group_event(evsel))
 		nr_pcnt = evsel->core.nr_members;
 
-	err = symbol__annotate(ms, evsel, 0, options, parch);
+	err = symbol__annotate(ms, evsel, options, parch);
 	if (err)
 		goto out_free_offsets;
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 455403e8fede..dade64781670 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -352,7 +352,7 @@ struct annotated_source *symbol__hists(struct symbol *sym, int nr_hists);
 void symbol__annotate_zero_histograms(struct symbol *sym);
 
 int symbol__annotate(struct map_symbol *ms,
-		     struct evsel *evsel, size_t privsize,
+		     struct evsel *evsel,
 		     struct annotation_options *options,
 		     struct arch **parch);
 int symbol__annotate2(struct map_symbol *ms,
-- 
2.24.1

Re: [PATCH v2 1/6] perf annotate: Remove privsize from symbol__annotate() args

[Arnaldo Carvalho de Melo]

Em Fri, Jan 24, 2020 at 01:34:27PM +0530, Ravi Bangoria escreveu:
> privsize is passed as 0 from all the symbol__annotate() callers.
> Remove it from argument list.

Right, trying to figure out when was it that this became unnecessary to
see if this in fact is hiding some other problem...

It all starts in the following change, re-reading those patches...

- Arnaldo


commit c835e1914c4bcfdd41f43d270cafc6d8119d7782
Author: Jiri Olsa <jolsa@kernel.org>
Date:   Wed Oct 11 17:01:37 2017 +0200

    perf annotate: Add annotation_line__(new|delete) functions
    
    Changing the way the annotation lines are allocated and adding
    annotation_line__(new|delete) functions to deal with this.
    
    Before the allocation schema was as follows:
    
      -----------------------------------------------------------
      struct disasm_line | struct annotation_line | private space
      -----------------------------------------------------------
    
    Where the private space is used in TUI code to store computed
    annotation data for events. The stdio code computes the data
    on the fly.
    
    The goal is to compute and store annotation line's data directly
    in the struct annotation_line itself, so this patch changes the
    line allocation schema as follows:
    
      ------------------------------------------------------------
      privsize space | struct disasm_line | struct annotation_line
      ------------------------------------------------------------
    
    Moving struct annotation_line to the end, because in following
    changes we will move here the non-fixed length event's data.
 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  tools/perf/builtin-top.c     | 2 +-
>  tools/perf/ui/gtk/annotate.c | 2 +-
>  tools/perf/util/annotate.c   | 7 ++++---
>  tools/perf/util/annotate.h   | 2 +-
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 8affcab75604..3e37747364e0 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -143,7 +143,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
>  		return err;
>  	}
>  
> -	err = symbol__annotate(&he->ms, evsel, 0, &top->annotation_opts, NULL);
> +	err = symbol__annotate(&he->ms, evsel, &top->annotation_opts, NULL);
>  	if (err == 0) {
>  		top->sym_filter_entry = he;
>  	} else {
> diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
> index 22cc240f7371..35f9641bf670 100644
> --- a/tools/perf/ui/gtk/annotate.c
> +++ b/tools/perf/ui/gtk/annotate.c
> @@ -174,7 +174,7 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
>  	if (ms->map->dso->annotate_warned)
>  		return -1;
>  
> -	err = symbol__annotate(ms, evsel, 0, &annotation__default_options, NULL);
> +	err = symbol__annotate(ms, evsel, &annotation__default_options, NULL);
>  	if (err) {
>  		char msg[BUFSIZ];
>  		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index ca73fb74ad03..ea70bc050bce 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2149,9 +2149,10 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
>  	annotation__calc_percent(notes, evsel, symbol__size(sym));
>  }
>  
> -int symbol__annotate(struct map_symbol *ms, struct evsel *evsel, size_t privsize,
> +int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
>  		     struct annotation_options *options, struct arch **parch)
>  {
> +	size_t privsize = 0;
>  	struct symbol *sym = ms->sym;
>  	struct annotation *notes = symbol__annotation(sym);
>  	struct annotate_args args = {
> @@ -2790,7 +2791,7 @@ int symbol__tty_annotate(struct map_symbol *ms, struct evsel *evsel,
>  	struct symbol *sym = ms->sym;
>  	struct rb_root source_line = RB_ROOT;
>  
> -	if (symbol__annotate(ms, evsel, 0, opts, NULL) < 0)
> +	if (symbol__annotate(ms, evsel, opts, NULL) < 0)
>  		return -1;
>  
>  	symbol__calc_percent(sym, evsel);
> @@ -3070,7 +3071,7 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
>  	if (perf_evsel__is_group_event(evsel))
>  		nr_pcnt = evsel->core.nr_members;
>  
> -	err = symbol__annotate(ms, evsel, 0, options, parch);
> +	err = symbol__annotate(ms, evsel, options, parch);
>  	if (err)
>  		goto out_free_offsets;
>  
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 455403e8fede..dade64781670 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -352,7 +352,7 @@ struct annotated_source *symbol__hists(struct symbol *sym, int nr_hists);
>  void symbol__annotate_zero_histograms(struct symbol *sym);
>  
>  int symbol__annotate(struct map_symbol *ms,
> -		     struct evsel *evsel, size_t privsize,
> +		     struct evsel *evsel,
>  		     struct annotation_options *options,
>  		     struct arch **parch);
>  int symbol__annotate2(struct map_symbol *ms,
> -- 
> 2.24.1
> 

-- 

- Arnaldo

Re: [PATCH v2 1/6] perf annotate: Remove privsize from symbol__annotate() args

[Ravi Bangoria]

On 1/30/20 4:46 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 24, 2020 at 01:34:27PM +0530, Ravi Bangoria escreveu:
>> privsize is passed as 0 from all the symbol__annotate() callers.
>> Remove it from argument list.
> 
> Right, trying to figure out when was it that this became unnecessary to
> see if this in fact is hiding some other problem...
> 
> It all starts in the following change, re-reading those patches...
> 
> - Arnaldo
> 

Ok, I just had a quick look at:
https://lore.kernel.org/lkml/20171011194323.GI3503@kernel.org/

This change was for python annotation support which, I guess, Jiri didn't posted
the patches? Jiri, are you planning to post them?

-Ravi

Re: [PATCH v2 1/6] perf annotate: Remove privsize from symbol__annotate() args

[Jiri Olsa]

On Mon, Feb 03, 2020 at 10:24:29AM +0530, Ravi Bangoria wrote:
> 
> 
> On 1/30/20 4:46 PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jan 24, 2020 at 01:34:27PM +0530, Ravi Bangoria escreveu:
> > > privsize is passed as 0 from all the symbol__annotate() callers.
> > > Remove it from argument list.
> > 
> > Right, trying to figure out when was it that this became unnecessary to
> > see if this in fact is hiding some other problem...
> > 
> > It all starts in the following change, re-reading those patches...
> > 
> > - Arnaldo
> > 
> 
> Ok, I just had a quick look at:
> https://lore.kernel.org/lkml/20171011194323.GI3503@kernel.org/
> 
> This change was for python annotation support which, I guess, Jiri didn't posted
> the patches? Jiri, are you planning to post them?

yea, as I wrote in another reply, this came in as preparation
to support python code lines, which still did not get in ;-)

also I replied that this way is probably even better for that,
so that's why I'm ok with the change

jirka

Re: [PATCH v2 1/6] perf annotate: Remove privsize from symbol__annotate() args

[Ravi Bangoria]

On 2/3/20 7:00 PM, Jiri Olsa wrote:
> On Mon, Feb 03, 2020 at 10:24:29AM +0530, Ravi Bangoria wrote:
>>
>>
>> On 1/30/20 4:46 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Fri, Jan 24, 2020 at 01:34:27PM +0530, Ravi Bangoria escreveu:
>>>> privsize is passed as 0 from all the symbol__annotate() callers.
>>>> Remove it from argument list.
>>>
>>> Right, trying to figure out when was it that this became unnecessary to
>>> see if this in fact is hiding some other problem...
>>>
>>> It all starts in the following change, re-reading those patches...
>>>
>>> - Arnaldo
>>>
>>
>> Ok, I just had a quick look at:
>> https://lore.kernel.org/lkml/20171011194323.GI3503@kernel.org/
>>
>> This change was for python annotation support which, I guess, Jiri didn't posted
>> the patches? Jiri, are you planning to post them?
> 
> yea, as I wrote in another reply, this came in as preparation
> to support python code lines, which still did not get in ;-)
> 
> also I replied that this way is probably even better for that,
> so that's why I'm ok with the change

Thanks Jiri.

-Ravi

[PATCH v2 2/6] perf annotate: Simplify disasm_line allocation and freeing code

[Ravi Bangoria]

We are allocating disasm_line object in annotation_line__new() instead
of disasm_line__new(). Similarly annotation_line__delete() is actually
freeing disasm_line object as well. This complexity is because of privsize.
But we don't need privsize anymore so get rid of privsize and simplify
disasm_line allocation and freeing code.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/util/annotate.c | 87 +++++++++++++-------------------------
 tools/perf/util/annotate.h |  1 -
 2 files changed, 29 insertions(+), 59 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ea70bc050bce..a922bc7043d4 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1143,7 +1143,6 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 }
 
 struct annotate_args {
-	size_t			 privsize;
 	struct arch		*arch;
 	struct map_symbol	 ms;
 	struct evsel	*evsel;
@@ -1153,83 +1152,55 @@ struct annotate_args {
 	int			 line_nr;
 };
 
-static void annotation_line__delete(struct annotation_line *al)
+static void annotation_line__init(struct annotation_line *al,
+				  struct annotate_args *args,
+				  int nr)
 {
-	void *ptr = (void *) al - al->privsize;
-
-	free_srcline(al->path);
-	zfree(&al->line);
-	free(ptr);
+	al->offset = args->offset;
+	al->line = strdup(args->line);
+	al->line_nr = args->line_nr;
+	al->data_nr = nr;
 }
 
-/*
- * Allocating the annotation line data with following
- * structure:
- *
- *    --------------------------------------
- *    private space | struct annotation_line
- *    --------------------------------------
- *
- * Size of the private space is stored in 'struct annotation_line'.
- *
- */
-static struct annotation_line *
-annotation_line__new(struct annotate_args *args, size_t privsize)
+static size_t disasm_line_size(int nr)
 {
 	struct annotation_line *al;
-	struct evsel *evsel = args->evsel;
-	size_t size = privsize + sizeof(*al);
-	int nr = 1;
-
-	if (perf_evsel__is_group_event(evsel))
-		nr = evsel->core.nr_members;
 
-	size += sizeof(al->data[0]) * nr;
-
-	al = zalloc(size);
-	if (al) {
-		al = (void *) al + privsize;
-		al->privsize   = privsize;
-		al->offset     = args->offset;
-		al->line       = strdup(args->line);
-		al->line_nr    = args->line_nr;
-		al->data_nr    = nr;
-	}
-
-	return al;
+	return (sizeof(struct disasm_line) + (sizeof(al->data[0]) * nr));
 }
 
 /*
  * Allocating the disasm annotation line data with
  * following structure:
  *
- *    ------------------------------------------------------------
- *    privsize space | struct disasm_line | struct annotation_line
- *    ------------------------------------------------------------
+ *    -------------------------------------------
+ *    struct disasm_line | struct annotation_line
+ *    -------------------------------------------
  *
  * We have 'struct annotation_line' member as last member
  * of 'struct disasm_line' to have an easy access.
- *
  */
 static struct disasm_line *disasm_line__new(struct annotate_args *args)
 {
 	struct disasm_line *dl = NULL;
-	struct annotation_line *al;
-	size_t privsize = args->privsize + offsetof(struct disasm_line, al);
+	int nr = 1;
 
-	al = annotation_line__new(args, privsize);
-	if (al != NULL) {
-		dl = disasm_line(al);
+	if (perf_evsel__is_group_event(args->evsel))
+		nr = args->evsel->core.nr_members;
 
-		if (dl->al.line == NULL)
-			goto out_delete;
+	dl = zalloc(disasm_line_size(nr));
+	if (!dl)
+		return NULL;
 
-		if (args->offset != -1) {
-			if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
-				goto out_free_line;
+	annotation_line__init(&dl->al, args, nr);
+	if (dl->al.line == NULL)
+		goto out_delete;
 
-			disasm_line__init_ins(dl, args->arch, &args->ms);
-		}
+	if (args->offset != -1) {
+		if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
+			goto out_free_line;
+
+		disasm_line__init_ins(dl, args->arch, &args->ms);
 	}
 
 	return dl;
@@ -1248,7 +1219,9 @@ void disasm_line__free(struct disasm_line *dl)
 	else
 		ins__delete(&dl->ops);
 	zfree(&dl->ins.name);
-	annotation_line__delete(&dl->al);
+	free_srcline(dl->al.path);
+	zfree(&dl->al.line);
+	free(dl);
 }
 
 int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw, int max_ins_name)
@@ -2152,11 +2125,9 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
 int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 		     struct annotation_options *options, struct arch **parch)
 {
-	size_t privsize = 0;
 	struct symbol *sym = ms->sym;
 	struct annotation *notes = symbol__annotation(sym);
 	struct annotate_args args = {
-		.privsize	= privsize,
 		.evsel		= evsel,
 		.options	= options,
 	};
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index dade64781670..75c58a759b96 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -141,7 +141,6 @@ struct annotation_line {
 	u64			 cycles;
 	u64			 cycles_max;
 	u64			 cycles_min;
-	size_t			 privsize;
 	char			*path;
 	u32			 idx;
 	int			 idx_asm;
-- 
2.24.1

Re: [PATCH v2 2/6] perf annotate: Simplify disasm_line allocation and freeing code

[Jiri Olsa]

On Fri, Jan 24, 2020 at 01:34:28PM +0530, Ravi Bangoria wrote:

SNIP

>  
>  /*
>   * Allocating the disasm annotation line data with
>   * following structure:
>   *
> - *    ------------------------------------------------------------
> - *    privsize space | struct disasm_line | struct annotation_line
> - *    ------------------------------------------------------------
> + *    -------------------------------------------
> + *    struct disasm_line | struct annotation_line
> + *    -------------------------------------------
>   *
>   * We have 'struct annotation_line' member as last member
>   * of 'struct disasm_line' to have an easy access.
> - *
>   */
>  static struct disasm_line *disasm_line__new(struct annotate_args *args)
>  {
>  	struct disasm_line *dl = NULL;
> -	struct annotation_line *al;
> -	size_t privsize = args->privsize + offsetof(struct disasm_line, al);
> +	int nr = 1;
>  
> -	al = annotation_line__new(args, privsize);

ok, I finally recalled why we did it like this.. for the python
annotation support, which never made it in ;-) however the allocation
in 'specialized' line and later call to annotation_line__init might
actualy be a better way

> -	if (al != NULL) {
> -		dl = disasm_line(al);
> +	if (perf_evsel__is_group_event(args->evsel))
> +		nr = args->evsel->core.nr_members;
>  
> -		if (dl->al.line == NULL)
> -			goto out_delete;
> +	dl = zalloc(disasm_line_size(nr));
> +	if (!dl)
> +		return NULL;
>  
> -		if (args->offset != -1) {
> -			if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
> -				goto out_free_line;
> +	annotation_line__init(&dl->al, args, nr);
> +	if (dl->al.line == NULL)
> +		goto out_delete;
>  
> -			disasm_line__init_ins(dl, args->arch, &args->ms);
> -		}
> +	if (args->offset != -1) {
> +		if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
> +			goto out_free_line;
> +
> +		disasm_line__init_ins(dl, args->arch, &args->ms);
>  	}
>  
>  	return dl;
> @@ -1248,7 +1219,9 @@ void disasm_line__free(struct disasm_line *dl)
>  	else
>  		ins__delete(&dl->ops);
>  	zfree(&dl->ins.name);
> -	annotation_line__delete(&dl->al);
> +	free_srcline(dl->al.path);
> +	zfree(&dl->al.line);

no need to zfree if you're freeing the memory on the next line
also could you please put it to annotation_line__exit, since
you already added the __init function

> +	free(dl);
>  }
>  

the rest of the patches look good to me

thanks,
jirka

Re: [PATCH v2 2/6] perf annotate: Simplify disasm_line allocation and freeing code

[Ravi Bangoria]

Hi Jiri,

On 1/27/20 5:29 PM, Jiri Olsa wrote:
> On Fri, Jan 24, 2020 at 01:34:28PM +0530, Ravi Bangoria wrote:
> 
> SNIP
> 
>>   
>>   /*
>>    * Allocating the disasm annotation line data with
>>    * following structure:
>>    *
>> - *    ------------------------------------------------------------
>> - *    privsize space | struct disasm_line | struct annotation_line
>> - *    ------------------------------------------------------------
>> + *    -------------------------------------------
>> + *    struct disasm_line | struct annotation_line
>> + *    -------------------------------------------
>>    *
>>    * We have 'struct annotation_line' member as last member
>>    * of 'struct disasm_line' to have an easy access.
>> - *
>>    */
>>   static struct disasm_line *disasm_line__new(struct annotate_args *args)
>>   {
>>   	struct disasm_line *dl = NULL;
>> -	struct annotation_line *al;
>> -	size_t privsize = args->privsize + offsetof(struct disasm_line, al);
>> +	int nr = 1;
>>   
>> -	al = annotation_line__new(args, privsize);
> 
> ok, I finally recalled why we did it like this.. for the python
> annotation support, which never made it in ;-) however the allocation
> in 'specialized' line and later call to annotation_line__init might
> actualy be a better way

Sorry, I didn't get your point about 'specialized' line. You mean the way
I'm doing is better wrt existing code?

> 
>> -	if (al != NULL) {
>> -		dl = disasm_line(al);
>> +	if (perf_evsel__is_group_event(args->evsel))
>> +		nr = args->evsel->core.nr_members;
>>   
>> -		if (dl->al.line == NULL)
>> -			goto out_delete;
>> +	dl = zalloc(disasm_line_size(nr));
>> +	if (!dl)
>> +		return NULL;
>>   
>> -		if (args->offset != -1) {
>> -			if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
>> -				goto out_free_line;
>> +	annotation_line__init(&dl->al, args, nr);
>> +	if (dl->al.line == NULL)
>> +		goto out_delete;
>>   
>> -			disasm_line__init_ins(dl, args->arch, &args->ms);
>> -		}
>> +	if (args->offset != -1) {
>> +		if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
>> +			goto out_free_line;
>> +
>> +		disasm_line__init_ins(dl, args->arch, &args->ms);
>>   	}
>>   
>>   	return dl;
>> @@ -1248,7 +1219,9 @@ void disasm_line__free(struct disasm_line *dl)
>>   	else
>>   		ins__delete(&dl->ops);
>>   	zfree(&dl->ins.name);
>> -	annotation_line__delete(&dl->al);
>> +	free_srcline(dl->al.path);
>> +	zfree(&dl->al.line);
> 
> no need to zfree if you're freeing the memory on the next line

dl->al.line is allocated using strdup() in annotation_line__init(). So I
think we still need that zfree(&dl->al.line).

> also could you please put it to annotation_line__exit, since
> you already added the __init function

Sure will do it.

Thanks for review :)
Ravi

[PATCH v2 3/6] perf annotate: Align struct annotate_args

[Ravi Bangoria]

Align fields of struct annotate_args.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/util/annotate.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index a922bc7043d4..d4a130d5913a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1143,13 +1143,13 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 }
 
 struct annotate_args {
-	struct arch		*arch;
-	struct map_symbol	 ms;
-	struct evsel	*evsel;
+	struct arch		  *arch;
+	struct map_symbol	  ms;
+	struct evsel		  *evsel;
 	struct annotation_options *options;
-	s64			 offset;
-	char			*line;
-	int			 line_nr;
+	s64			  offset;
+	char			  *line;
+	int			  line_nr;
 };
 
 static void annotation_line__init(struct annotation_line *al,
-- 
2.24.1

[PATCH v2 4/6] perf annotate: Fix segfault with source toggle

[Ravi Bangoria]

While rendering annotate browser from perf report tui, we keep track
of total number of lines(asm + source) in annotation->nr_entries and
total number of asm lines in annotation->nr_asm_entries. But we don't
reset them before starting. Thus if user annotates same function
multiple times, we restart incrementing these fields with old values.

This causes a segfault when user tries to toggle source code after
annotating same function multiple times. Fix it.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/annotate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index d4a130d5913a..58e2c51e9d62 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2616,6 +2616,8 @@ void annotation__set_offsets(struct annotation *notes, s64 size)
 	struct annotation_line *al;
 
 	notes->max_line_len = 0;
+	notes->nr_entries = 0;
+	notes->nr_asm_entries = 0;
 
 	list_for_each_entry(al, &notes->src->source, node) {
 		size_t line_len = strlen(al->line);
-- 
2.24.1

[PATCH v2 5/6] perf annotate: Make few functions static

[Ravi Bangoria]

These functions are not used outside of tools/perf/util/annotate.c.
Make them static.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/util/annotate.c | 8 ++++----
 tools/perf/util/annotate.h | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 58e2c51e9d62..50b2c99b2551 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1057,7 +1057,7 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
 	}
 }
 
-void annotation__compute_ipc(struct annotation *notes, size_t size)
+static void annotation__compute_ipc(struct annotation *notes, size_t size)
 {
 	s64 offset;
 
@@ -2578,7 +2578,7 @@ bool disasm_line__is_valid_local_jump(struct disasm_line *dl, struct symbol *sym
 	return true;
 }
 
-void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
+static void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
 {
 	u64 offset, size = symbol__size(sym);
 
@@ -2611,7 +2611,7 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
 	}
 }
 
-void annotation__set_offsets(struct annotation *notes, s64 size)
+static void annotation__set_offsets(struct annotation *notes, s64 size)
 {
 	struct annotation_line *al;
 
@@ -2667,7 +2667,7 @@ static int annotation__max_ins_name(struct annotation *notes)
 	return max_name;
 }
 
-void annotation__init_column_widths(struct annotation *notes, struct symbol *sym)
+static void annotation__init_column_widths(struct annotation *notes, struct symbol *sym)
 {
 	notes->widths.addr = notes->widths.target =
 		notes->widths.min_addr = hex_width(symbol__size(sym));
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 75c58a759b96..3b6848c1d593 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -316,11 +316,7 @@ static inline bool annotation_line__filter(struct annotation_line *al, struct an
 	return notes->options->hide_src_code && al->offset == -1;
 }
 
-void annotation__set_offsets(struct annotation *notes, s64 size);
-void annotation__compute_ipc(struct annotation *notes, size_t size);
-void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym);
 void annotation__update_column_widths(struct annotation *notes);
-void annotation__init_column_widths(struct annotation *notes, struct symbol *sym);
 
 static inline struct sym_hist *annotated_source__histogram(struct annotated_source *src, int idx)
 {
-- 
2.24.1

[PATCH v2 6/6] perf annotate: Get rid of annotation->nr_jumps

[Ravi Bangoria]

nr_jumps from struct annotation is not used since it's inception in
commit 2402e4a936a0 ("perf annotate browser: Show 'jumpy' functions").
Get rid of it.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/util/annotate.c | 2 --
 tools/perf/util/annotate.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 50b2c99b2551..1c0fdc164f7a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2606,8 +2606,6 @@ static void annotation__mark_jump_targets(struct annotation *notes, struct symbo
 
 		if (++al->jump_sources > notes->max_jump_sources)
 			notes->max_jump_sources = al->jump_sources;
-
-		++notes->nr_jumps;
 	}
 }
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 3b6848c1d593..2f333dfb586d 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -281,7 +281,6 @@ struct annotation {
 	struct annotation_options *options;
 	struct annotation_line	**offsets;
 	int			nr_events;
-	int			nr_jumps;
 	int			max_jump_sources;
 	int			nr_entries;
 	int			nr_asm_entries;
-- 
2.24.1