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

[a.sidorin]

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.

[zaks.anna]

test/Analysis/inlining/analysis-order.c

13 ↗

(On Diff #42396)

Can you use CHECK-NEXT instead?

[a.sidorin]

Replaced CHECK with CHECK-NEXT

[zaks.anna]

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?

[a.sidorin]

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]

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

[a.sidorin]

Added a comment.

[a.sidorin]

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

cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

for (llvm::ReversePostOrderTraversal<clang::CallGraph*>::rpo_iterator
 
     // Analyze the function.
     SetOfConstDecls VisitedCallees;
 
     HandleCode(D, AM_Path, getInliningModeForFunction(D, Visited),
                (Mgr->options.InliningMode == All ? nullptr : &VisitedCallees));
 
     // Add the visited callees to the global visited set.
-    for (SetOfConstDecls::iterator I = VisitedCallees.begin(),
-                                   E = VisitedCallees.end(); I != E; ++I) {
-        Visited.insert(*I);
-    }
+    for (const Decl *Callee : VisitedCallees)
+      // Decls from CallGraph are already canonical. But Decls coming from
+      // CallExprs may be not. We should canonicalize them manually.
+      Visited.insert(isa<ObjCMethodDecl>(Callee) ? Callee
+                                                 : Callee->getCanonicalDecl());
     VisitedAsTopLevel.insert(D);
   }
 }
 
 void AnalysisConsumer::HandleTranslationUnit(ASTContext &C) {
   // Don't run the actions if an error has occurred with parsing the file.
   DiagnosticsEngine &Diags = PP.getDiagnostics();
   if (Diags.hasErrorOccurred() || Diags.hasFatalErrorOccurred())

cfe/trunk/test/Analysis/inlining/analysis-order.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core.builtin.NoReturnFunctions -analyzer-display-progress %s 2>&1 | FileCheck %s
+
+// Do not analyze test1() again because it was inlined
+void test1();
+
+void test2() {
+  test1();
+}
+
+void test1() {
+}
+
+// CHECK: analysis-order.c test2
+// CHECK-NEXT: analysis-order.c test1
+// CHECK-NEXT: analysis-order.c test2