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

[Jiri Olsa <jolsa@redhat.com>]

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