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