lkml.org 
[lkml]   [2020]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/6] perf annotate: Simplify disasm_line allocation and freeing code
From
Date
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

\
 
 \ /
  Last update: 2020-01-28 14:57    [W:0.040 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site