This is an archive of the discontinued LLVM Phabricator instance.

AnalysisConsumer: use canonical decl for both lookup and store of visited decls
ClosedPublic

Authored by a.sidorin on Dec 10 2015, 1:09 AM.

Details

Summary

Due to redeclarations, the function may have different declarations used in CallExpr and in the definition. However, we need to use a unique declaration for both store and lookup in VisitedCallees. This patch fixes issues with analysis in topological order. A simple test is included.

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin updated this revision to Diff 42396.Dec 10 2015, 1:09 AM
a.sidorin retitled this revision from to AnalysisConsumer: use canonical decl for both lookup and store of visited decls.
a.sidorin updated this object.
a.sidorin set the repository for this revision to rL LLVM.
a.sidorin added a subscriber: cfe-commits.
NoQ added a subscriber: NoQ.Dec 10 2015, 2:08 AM
zaks.anna added inline comments.Dec 11 2015, 8:58 PM
test/Analysis/inlining/analysis-order.c
13 ↗(On Diff #42396)

Can you use CHECK-NEXT instead?

a.sidorin updated this revision to Diff 42691.Dec 14 2015, 1:11 AM

Replaced CHECK with CHECK-NEXT

zaks.anna edited edge metadata.Dec 14 2015, 12:49 PM

I wonder if we can refactor the code so that it is less error prone..

shouldSkipFunction(D, Visited, VisitedAsTopLevel) works with two sets. I assume that you have not updated Decls coming from VisitedAsTopLevel because they come from the CFG and should already be canonical. In this patch, you make sure the values stored in Visited are canonical inside ExprEngineCallAndReturn.cpp. Unfortunately, now the conversion to canonical form is spread all over, which makes it error prone.

Note also that the conversion into canonical from inside the CallGraph is different than what you have:

if (F && !isa<ObjCMethodDecl>(F))
  F = F->getCanonicalDecl();

Here is a copy an paste from the review of the patch that introduced that code that attempts to explain the special handling of ObjC:
"This differs from what we discussed by not using the canonical declaration for ObjCMethodDecls. This is because hasBody and getBody work differently for ObjCMethodDecls than FunctionDecls. Using the canonical declaration breaks the call graph. There isn't a problem with the call graph created for ObjCMethodDecls, so I have left them alone."

I suggest:

  • Try to figure out what's special about ObjCMethodDecl and can that be fixed in some other way.
  • Move the getCanonicalDecl conversion into HandleDeclsCallGraph() + maybe add a comment saying why the decls inside VisitedAsTopLevel are canonical already. This way most of the code would be in one place. Would that work?

Thank you. I'm not familiar with Objective-C/C++ issues so I missed that. I'll take an another look. BTW, maybe it is better to use definition decl instead of canonical?

a.sidorin updated this revision to Diff 43674.Dec 28 2015, 2:23 AM
a.sidorin edited edge metadata.
a.sidorin marked an inline comment as done.

Canonicalize the inserted declarations in a single place; honor ObjCMethodDecls.

a.sidorin updated this revision to Diff 43723.Dec 29 2015, 2:03 AM
a.sidorin removed rL LLVM as the repository for this revision.

Added a comment.

a.sidorin updated this revision to Diff 43726.Dec 29 2015, 2:36 AM

C++11-fy adding loop. (Sorry for the noise.)

zaks.anna accepted this revision.Jan 5 2016, 11:21 AM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.Jan 5 2016, 11:21 AM
This revision was automatically updated to reflect the committed changes.