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