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

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

[Ravi Bangoria]

Few fixes / improvements related to perf annotate.

v2: https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com

v2->v3:
 - [PATCH v3 2/6] New function annotation_line__exit() to clear
   annotation_line objects.

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   | 115 ++++++++++++++---------------------
 tools/perf/util/annotate.h   |   8 +--
 4 files changed, 49 insertions(+), 78 deletions(-)

-- 
2.24.1

[PATCH v3 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

[PATCH v3 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 | 86 ++++++++++++++------------------------
 tools/perf/util/annotate.h |  1 -
 2 files changed, 31 insertions(+), 56 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ea70bc050bce..b7fb7451445f 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,61 @@ 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;
+	al->offset = args->offset;
+	al->line = strdup(args->line);
+	al->line_nr = args->line_nr;
+	al->data_nr = nr;
+}
 
+static void annotation_line__exit(struct annotation_line *al)
+{
 	free_srcline(al->path);
 	zfree(&al->line);
-	free(ptr);
 }
 
-/*
- * 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 +1225,8 @@ void disasm_line__free(struct disasm_line *dl)
 	else
 		ins__delete(&dl->ops);
 	zfree(&dl->ins.name);
-	annotation_line__delete(&dl->al);
+	annotation_line__exit(&dl->al);
+	free(dl);
 }
 
 int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw, int max_ins_name)
@@ -2152,11 +2130,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

[PATCH v3 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 b7fb7451445f..c0154f650959 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 v3 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 c0154f650959..29029a5d59c7 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2621,6 +2621,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 v3 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 29029a5d59c7..bf7b5eefb990 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;
 
@@ -2583,7 +2583,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);
 
@@ -2616,7 +2616,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;
 
@@ -2672,7 +2672,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 v3 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 bf7b5eefb990..b2a26adeb4cd 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2611,8 +2611,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

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

[Jiri Olsa]

On Tue, Feb 04, 2020 at 10:22:27AM +0530, Ravi Bangoria wrote:
> Few fixes / improvements related to perf annotate.
> 
> v2: https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com
> 
> v2->v3:
>  - [PATCH v3 2/6] New function annotation_line__exit() to clear
>    annotation_line objects.

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

thanks,
jirka

> 
> 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   | 115 ++++++++++++++---------------------
>  tools/perf/util/annotate.h   |   8 +--
>  4 files changed, 49 insertions(+), 78 deletions(-)
> 
> -- 
> 2.24.1
> 

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

[Jiri Olsa]

On Tue, Feb 04, 2020 at 10:22:27AM +0530, Ravi Bangoria wrote:
> Few fixes / improvements related to perf annotate.
> 
> v2: https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com
> 
> v2->v3:
>  - [PATCH v3 2/6] New function annotation_line__exit() to clear
>    annotation_line objects.

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

thanks,
jirka

> 
> 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   | 115 ++++++++++++++---------------------
>  tools/perf/util/annotate.h   |   8 +--
>  4 files changed, 49 insertions(+), 78 deletions(-)
> 
> -- 
> 2.24.1
> 

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

[Arnaldo Carvalho de Melo]

Em Thu, Feb 06, 2020 at 08:04:12PM +0100, Jiri Olsa escreveu:
> On Tue, Feb 04, 2020 at 10:22:27AM +0530, Ravi Bangoria wrote:
> > Few fixes / improvements related to perf annotate.
> > 
> > v2: https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com
> > 
> > v2->v3:
> >  - [PATCH v3 2/6] New function annotation_line__exit() to clear
> >    annotation_line objects.
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks applied the series to perf/urgent as it contains a fix.

- Arnaldo
 
> thanks,
> jirka
> 
> > 
> > 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   | 115 ++++++++++++++---------------------
> >  tools/perf/util/annotate.h   |   8 +--
> >  4 files changed, 49 insertions(+), 78 deletions(-)
> > 
> > -- 
> > 2.24.1
> > 
> 

-- 

- Arnaldo

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

[Ravi Bangoria]

On 2/27/20 7:41 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 06, 2020 at 08:04:12PM +0100, Jiri Olsa escreveu:
>> On Tue, Feb 04, 2020 at 10:22:27AM +0530, Ravi Bangoria wrote:
>>> Few fixes / improvements related to perf annotate.
>>>
>>> v2: https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com
>>>
>>> v2->v3:
>>>   - [PATCH v3 2/6] New function annotation_line__exit() to clear
>>>     annotation_line objects.
>>
>> Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> Thanks applied the series to perf/urgent as it contains a fix.

Thanks Arnaldo. I don't see patch #5 and #6 in perf/urgent. You missed them?
Or didn't consider for perf/urgent :) ?

Ravi

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

[Arnaldo Melo]

On February 28, 2020 11:42:27 AM GMT-03:00, Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote:
>
>
>On 2/27/20 7:41 PM, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Feb 06, 2020 at 08:04:12PM +0100, Jiri Olsa escreveu:
>>> On Tue, Feb 04, 2020 at 10:22:27AM +0530, Ravi Bangoria wrote:
>>>> Few fixes / improvements related to perf annotate.
>>>>
>>>> v2:
>https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com
>>>>
>>>> v2->v3:
>>>>   - [PATCH v3 2/6] New function annotation_line__exit() to clear
>>>>     annotation_line objects.
>>>
>>> Acked-by: Jiri Olsa <jolsa@redhat.com>
>> 
>> Thanks applied the series to perf/urgent as it contains a fix.
>
>Thanks Arnaldo. I don't see patch #5 and #6 in perf/urgent. You missed
>them?
>Or didn't consider for perf/urgent :) ?
 
Yep

>
>Ravi

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Arnaldo Melo]

On February 28, 2020 11:42:27 AM GMT-03:00, Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote:
>
>
>On 2/27/20 7:41 PM, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Feb 06, 2020 at 08:04:12PM +0100, Jiri Olsa escreveu:
>>> On Tue, Feb 04, 2020 at 10:22:27AM +0530, Ravi Bangoria wrote:
>>>> Few fixes / improvements related to perf annotate.
>>>>
>>>> v2:
>https://lore.kernel.org/r/20200124080432.8065-1-ravi.bangoria@linux.ibm.com
>>>>
>>>> v2->v3:
>>>>   - [PATCH v3 2/6] New function annotation_line__exit() to clear
>>>>     annotation_line objects.
>>>
>>> Acked-by: Jiri Olsa <jolsa@redhat.com>
>> 
>> Thanks applied the series to perf/urgent as it contains a fix.
>
>Thanks Arnaldo. I don't see patch #5 and #6 in perf/urgent. You missed
>them?
>Or didn't consider for perf/urgent :) ?

Yeah, I have those in my perf/core branch.

- Arnaldo
>
>Ravi

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[tip: perf/urgent] perf annotate: Simplify disasm_line allocation and freeing code

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     2316f861ae9ca640708f9529ae40a6f0bd7ae048
Gitweb:        https://git.kernel.org/tip/2316f861ae9ca640708f9529ae40a6f0bd7ae048
Author:        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate:    Tue, 04 Feb 2020 10:22:29 +05:30
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Thu, 27 Feb 2020 11:07:13 -03:00

perf annotate: Simplify disasm_line allocation and freeing code

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>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Link: http://lore.kernel.org/lkml/20200204045233.474937-3-ravi.bangoria@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/annotate.c | 86 +++++++++++++------------------------
 tools/perf/util/annotate.h |  1 +-
 2 files changed, 31 insertions(+), 56 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index a76309f..f11031a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1143,7 +1143,6 @@ out:
 }
 
 struct annotate_args {
-	size_t			 privsize;
 	struct arch		*arch;
 	struct map_symbol	 ms;
 	struct evsel	*evsel;
@@ -1153,83 +1152,61 @@ 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;
+	al->offset = args->offset;
+	al->line = strdup(args->line);
+	al->line_nr = args->line_nr;
+	al->data_nr = nr;
+}
 
+static void annotation_line__exit(struct annotation_line *al)
+{
 	free_srcline(al->path);
 	zfree(&al->line);
-	free(ptr);
 }
 
-/*
- * 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 +1225,8 @@ void disasm_line__free(struct disasm_line *dl)
 	else
 		ins__delete(&dl->ops);
 	zfree(&dl->ins.name);
-	annotation_line__delete(&dl->al);
+	annotation_line__exit(&dl->al);
+	free(dl);
 }
 
 int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw, int max_ins_name)
@@ -2152,11 +2130,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 7bc6098..0012586 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;

[tip: perf/urgent] perf annotate: Align struct annotate_args

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     d3c03147bf8019bda821334428e0ba31ce4fb425
Gitweb:        https://git.kernel.org/tip/d3c03147bf8019bda821334428e0ba31ce4fb425
Author:        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate:    Tue, 04 Feb 2020 10:22:30 +05:30
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Thu, 27 Feb 2020 11:47:23 -03:00

perf annotate: Align struct annotate_args

Align fields of struct annotate_args.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Link: http://lore.kernel.org/lkml/20200204045233.474937-4-ravi.bangoria@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.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 f11031a..c816e58 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1143,13 +1143,13 @@ out:
 }
 
 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,

[tip: perf/urgent] perf annotate: Fix segfault with source toggle

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     e0560ba6d92f06dbe13e9d11c921a60c07ea6fcc
Gitweb:        https://git.kernel.org/tip/e0560ba6d92f06dbe13e9d11c921a60c07ea6fcc
Author:        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate:    Tue, 04 Feb 2020 10:22:31 +05:30
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Thu, 27 Feb 2020 11:47:23 -03:00

perf annotate: Fix segfault with source toggle

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>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Link: http://lore.kernel.org/lkml/20200204045233.474937-5-ravi.bangoria@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@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 c816e58..0ea95be 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2621,6 +2621,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);

[tip: perf/urgent] perf annotate: Remove privsize from symbol__annotate() args

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     e0ad4d68548005adb54cc7c35fd9abf760a2a050
Gitweb:        https://git.kernel.org/tip/e0ad4d68548005adb54cc7c35fd9abf760a2a050
Author:        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate:    Tue, 04 Feb 2020 10:22:28 +05:30
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Thu, 27 Feb 2020 11:06:14 -03:00

perf annotate: Remove privsize from symbol__annotate() args

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>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Link: http://lore.kernel.org/lkml/20200204045233.474937-2-ravi.bangoria@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.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 cc26aea..f6dd1a6 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 22cc240..35f9641 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 3b79da5..a76309f 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 8e54184..7bc6098 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -350,7 +350,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,

[tip: perf/core] perf annotate: Get rid of annotation->nr_jumps

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     dabce16bd2926b82ef1bef70acd8b24828e9448b
Gitweb:        https://git.kernel.org/tip/dabce16bd2926b82ef1bef70acd8b24828e9448b
Author:        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate:    Tue, 04 Feb 2020 10:22:33 +05:30
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 04 Mar 2020 10:34:10 -03:00

perf annotate: Get rid of annotation->nr_jumps

The 'nr_jumps' field in '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>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Link: http://lore.kernel.org/lkml/20200204045233.474937-7-ravi.bangoria@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.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 0ea95be..f1ea0d6 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2611,8 +2611,6 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
 
 		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 0012586..07c7759 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -279,7 +279,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;