[PATCH 0/3] perf annotate: Misc fixes / improvements

[PATCH 0/3] perf annotate: Misc fixes / improvements

[Ravi Bangoria]

Few fixes / improvements related to perf annotate.

Ravi Bangoria (3):
  perf annotate: Nuke privsize
  perf annotate: Align struct annotate_args
  perf annotate: Fix segfault with source toggle

 tools/perf/builtin-top.c     |   2 +-
 tools/perf/ui/gtk/annotate.c |   2 +-
 tools/perf/util/annotate.c   | 106 +++++++++++++----------------------
 tools/perf/util/annotate.h   |   3 +-
 4 files changed, 43 insertions(+), 70 deletions(-)

-- 
2.24.1

[PATCH 1/3] perf annotate: Nuke privsize

[Ravi Bangoria]

We don't use privsize now and it's always 0. Remove privsize
from code. Also simplify disasm_line allocation and freeing
code which was bit complex because of privsize.

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   | 92 +++++++++++++-----------------------
 tools/perf/util/annotate.h   |  3 +-
 4 files changed, 35 insertions(+), 64 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 795e353de095..26765e41c083 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 f5e77ed237e8..7f9851772462 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__new(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__new(&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)
@@ -2143,13 +2116,12 @@ 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)
 {
 	struct symbol *sym = ms->sym;
 	struct annotation *notes = symbol__annotation(sym);
 	struct annotate_args args = {
-		.privsize	= privsize,
 		.evsel		= evsel,
 		.options	= options,
 	};
@@ -2784,7 +2756,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);
@@ -3064,7 +3036,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 7075d98f69d9..4a86558646ff 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -139,7 +139,6 @@ struct annotation_line {
 	u64			 cycles;
 	u64			 cycles_max;
 	u64			 cycles_min;
-	size_t			 privsize;
 	char			*path;
 	u32			 idx;
 	int			 idx_asm;
@@ -350,7 +349,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 1/3] perf annotate: Nuke privsize

[Jiri Olsa]

On Fri, Jan 17, 2020 at 02:56:10PM +0530, Ravi Bangoria wrote:
> We don't use privsize now and it's always 0. Remove privsize
> from code. Also simplify disasm_line allocation and freeing
> code which was bit complex because of privsize.
> 
> 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   | 92 +++++++++++++-----------------------
>  tools/perf/util/annotate.h   |  3 +-
>  4 files changed, 35 insertions(+), 64 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 795e353de095..26765e41c083 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 f5e77ed237e8..7f9851772462 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__new(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)
>  {

I agree we can get rid of the 'users' privsize passed from symbol__annotate,
but could you please put it in separate patch, while keeping privsize in here?

and then put the rest of the code factoring into separate patch,
so we can see clearly the change and the benefits

your new annotation_line__new should be renamed to something like
annotation_line__init ... we keep __new suffix for functions that
return new objects

thanks,
jirka

Re: [PATCH 1/3] perf annotate: Nuke privsize

[Ravi Bangoria]

On 1/20/20 3:38 PM, Jiri Olsa wrote:
>> -/*
>> - * 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)
>>   {
> 
> I agree we can get rid of the 'users' privsize passed from symbol__annotate,
> but could you please put it in separate patch, while keeping privsize in here?
> 
> and then put the rest of the code factoring into separate patch,
> so we can see clearly the change and the benefits
> 
> your new annotation_line__new should be renamed to something like
> annotation_line__init ... we keep __new suffix for functions that
> return new objects

Sure Jiri. Will resend with these changes.

- Ravi

[PATCH 2/3] 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 7f9851772462..fe98d29dfbc4 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__new(struct annotation_line *al,
-- 
2.24.1

[PATCH 3/3] 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>
---
 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 fe98d29dfbc4..df09c2070337 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2610,6 +2610,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

Re: [PATCH 3/3] perf annotate: Fix segfault with source toggle

[Jiri Olsa]

On Fri, Jan 17, 2020 at 02:56:12PM +0530, Ravi Bangoria wrote:
> 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>
> ---
>  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 fe98d29dfbc4..df09c2070337 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2610,6 +2610,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;

seems fair ;-)

Acked-by: Jiri Olsa <jolsa@redhat.com>

also could you please make that function static (in separate change)
in your next repost?

thanks,
jirka

>  
>  	list_for_each_entry(al, &notes->src->source, node) {
>  		size_t line_len = strlen(al->line);
> -- 
> 2.24.1
> 

Re: [PATCH 3/3] perf annotate: Fix segfault with source toggle

[Ravi Bangoria]

On 1/20/20 3:42 PM, Jiri Olsa wrote:
> On Fri, Jan 17, 2020 at 02:56:12PM +0530, Ravi Bangoria wrote:
>> 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>
>> ---
>>   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 fe98d29dfbc4..df09c2070337 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -2610,6 +2610,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;
> 
> seems fair ;-)
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks!

> 
> also could you please make that function static (in separate change)
> in your next repost?

Sure will do.

- Ravi