[cfe-dev] Missing edges in analysis call graph

[Anna Zaks]

> On Nov 21, 2014, at 5:21 PM, Daniel DeFreez <dcdefreez at ucdavis.edu> wrote:
> 
> 
> On Sat, Nov 22, 2014 at 2:11 AM, Anna Zaks <ganna at apple.com <mailto:ganna at apple.com>> wrote:
> 
>> On Nov 21, 2014, at 5:01 PM, Daniel DeFreez <dcdefreez at ucdavis.edu <mailto:dcdefreez at ucdavis.edu>> wrote:
>> 
>> 
>> On Sat, Nov 22, 2014 at 12:48 AM, Anna Zaks <ganna at apple.com <mailto:ganna at apple.com>> wrote:
>> 
>>> On Nov 21, 2014, at 3:35 PM, Daniel DeFreez <dcdefreez at ucdavis.edu <mailto: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 :)
> 

:)

I'll be on vacation next week. 

Seems like we have a solution here. I might be able to approve the patch, but won't be able to apply it. You should send the patch to cfe-dev and CC me.

Thank you!
Anna.

> Many thanks.
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20141121/613ae1f8/attachment.html>