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

From: Milian Wolff <milian.wolff@kdab.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: acme@kernel.org, Jin Yao <yao.jin@linux.intel.com>,
	Linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	David Ahern <dsahern@gmail.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	kernel-team@lge.com
Subject: Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
Date: Thu, 07 Sep 2017 17:05:34 +0200	[thread overview]
Message-ID: <3394979.u4vPUxEDmB@milian-kdab2> (raw)
In-Reply-To: <20170907145853.GA5764@danjae.aot.lge.com>

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

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@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3826 bytes --]