Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames

[Milian Wolff]

On Thursday, September 7, 2017 4:58:53 PM CEST Namhyung Kim wrote:
> Hi Milian,

Hey Namhyung!

> > > > > @@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node
> > > > > *node)
> > > > > 
> > > > >  	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
> > > > >  	
> > > > >  		list_del_init(&ilist->list);
> > > > > 
> > > > > -		zfree(&ilist->filename);
> > > > > -		zfree(&ilist->funcname);
> > > > > +		zfree(&ilist->srcline);
> > > > > +		// only the inlined symbols are owned by the list
> > > > > +		if (ilist->symbol && ilist->symbol->inlined)
> > > > > +			symbol__delete(ilist->symbol);
> > > > 
> > > > Existing symbols are released at this moment.
> > > 
> > > Thanks for the review, I'll try to look into these issues once I have
> > > more
> > > time again.
> > 
> > OK, so I just dug into this part of the patch again. I don't think it's
> > actually a problem after all:
> > 
> > When an inline node reuses the real symbol, that symbol won't have its
> > `inlined` member set to true. Thus these symbols will never get deleted by
> > inline_node__delete.
> 
> But ilist->symbol is a dangling pointer so accessing ->inlined would
> be a problem, no?

Sorry, but I can't follow. Why would it be a dangling pointer? Note, again, 
that I've tested this with both valgrind and ASAN and neither reports any 
issues about this code.

> > If you have suggestions on how to make this clearer, I'm
> > all ears. For now, I'll add a comment to where we alias/reuse the symbol.
> > 
> > I'll try to split up the patch now to make it somehow easier to review.
> 
> Thanks for doing this, but I'm afraid I don't have time to review
> before going to OSS-NA.

No problem, enjoy!
-- 
Milian Wolff | milian.wolff@xxxxxxxx | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Attachment: smime.p7s
Description: S/MIME cryptographic signature