[cfe-dev] Missing edges in analysis call graph

Daniel DeFreez dcdefreez at ucdavis.edu
Fri Nov 21 17:01:06 PST 2014


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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20141122/84500fae/attachment.html>


More information about the cfe-dev mailing list