[cfe-dev] Missing edges in analysis call graph

[Daniel DeFreez]

On Sat, Nov 22, 2014 at 2:11 AM, Anna Zaks <ganna at apple.com> wrote:

>
> On Nov 21, 2014, at 5:01 PM, Daniel DeFreez <dcdefreez at ucdavis.edu> wrote:
>
>
> On Sat, Nov 22, 2014 at 12:48 AM, Anna Zaks <ganna at apple.com> wrote:
>
>>
>> On Nov 21, 2014, at 3:35 PM, Daniel DeFreez <dcdefreez at ucdavis.edu>
>> wrote:
>>
>>  I believe with your patch, the node will get added, but the definition
>>>> used will not have the body attached to it, so the analyzer will not
>>>> process the body.
>>>>
>>>
>>> Hmm, doesn't the analyzer itself use getBody() to access the body,
>>> thereby walking the list of redeclarations? It seems like it is still
>>> processing the body of the function, but maybe I don't know what to look
>>> for.
>>>
>>> This is a good point. We might not be gaining anything by replacing the
>>> decl with the one that has a body since getBody is guaranteed to search the
>>> redeclarations regardless.
>>>
>>> How about just removing both calls to isThisDeclarationADefinition()
>>> (from includeInGraph) and replacing the call to getBody with hasBody (to
>>> avoid the unnecessary AST de-serialization of the body)? That should work
>>> and would simplify the function greatly. What do you think?
>>>
>>
>> I think replacing getBody() with hasBody() is a good idea regardless of
>> the approach taken to fix the missing edges. And avoiding node replacement
>> would make it cleaner.
>>
>>
>> I am not sure why some of the checks in your first patch were necessary
>>> (like this one).
>>> +    // Skip definitions with previous declarations
>>> +    if (FD->getPreviousDecl())
>>> +      return false;
>>> +
>>>
>>>
>> The reason I put that in was to avoid inserting redundant nodes. Just
>> removing isThisDeclarationADefinition() yields:
>>
>>
>> Oh, I see! getOrInsertNode() would not insert an existing declaration,
>> but it does not check for re-declarations.
>>
>> But would your approach work for this set?
>> void a();
>> void a() { }
>> void b() { a(); } // Here, a call to "a()" might not be added since "a()"
>> has a previous declaration.
>>
>
> Alas, yes, you're right. I can't believe I didn't try that. Not going to
> work.
>
> Sounds like we would want to store canonical declarations, which brings us
>> back to the suggestion of using "hasBody(Definition)" and only store the
>> definitions that have bodies. Changing the signature of "includeInGraph" so
>> that it mutates the Decl is fine; though, you might want to rename it as
>> well.
>>
>>
> Yep. The problem isn't with includeInGraph, though. It's with
> hasBody(Definition). Since it takes a reference to a pointer-to-const,
> using it get the canonical declaration means the canonical declaration
> itself must be const. So pretty much all of the Decl* in CallGraph would
> have to get changed to const.
>
> I only know of two ways around it:
> 1) Change the FunctionDecl class
> 2) Walk redecls in some fashion similar to my second patch, which
> essentially just re-implemented hasBody without forcing the declaration to
> be const.
>
> Any other way?
>
>
> Looks like we can use getCanonicalDecl() instead (in both FunctionDecl and
> ObjCMethodDecl case).
>
>
Ah! Excellent. How aptly named :)

Many thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20141122/4a442907/attachment.html>