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

From: Jiri Olsa
Date: Mon Jan 27 2020 - 06:59:22 EST


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