[analyzer] Enable constructor support in evalCall event

[vrnithinkumar]

Summary

Adding CXX constructor calls support with evalCall event

• Made changes to pass EvalCallOptions via runCheckersForEvalCall into defaultEvalCall.
• Added tests to verify the order.
• Updated the AnalysisOrderChecker to support evalCall for testing

[vrnithinkumar]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

100

Moved the struct EvalCallOptions outside ExprEngine class.
Since CheckerManager.h using forward declaration of ExprEngine and inner type EvalCallOptions is needed for the runCheckersForEvalCall function declaration.

[NoQ]

Fantastic, thank you!

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

586

If you want you can make it an optional argument of runCheckersForEvalCall(), like it's done in defaultEvalCall().

clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp

34

No NEwLiNE aT EnD Of FiLE

clang/test/Analysis/new-ctor-conservative.cpp

30

Thx!

[vrnithinkumar]

Addressing review comment adding miised new line

[vrnithinkumar]

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

586

I tried to make it as default argument for runCheckersForEvalCall() but struct EvalCallOptions is forward declared in CheckerManager.h.

clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp

34

added new line

[vrnithinkumar]

Fixing wrongly deleted the old commit via arc

[NoQ]

Hmm, given that you're using ARC, you're probably intending to turn the review title/summary into a commit message. Let's make it prettier then:

• Commit messages are typically written in imperative mood, eg. "Enabling" -> "Enable", "Adding"/"Added" -> "Add".
• Abbreviating "constructor" to "ctr" was really unnecessary :)

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

586

Oh well! You probably still don't need a separate local variable, can you try EvalCallOptions() or even {}?

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

130

This seems a bit cryptic, how about {argno:?

[vrnithinkumar]

Fixing test failures

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

130

For more clarity, added "{argno:".

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

668–669

This is for fixing the unit test failures.
Previously assert to check the type inside cast<CallExpr> was causing the unit test failures.
The reason was CXXConstructExpr was not inherited from CallExpr and the cast was causing the assert failure with isa check.

I am not sure removing the cast is the best solution.
And the TODO comment, I did not understood properly.

Alternative approach was to cast it to CXXConstructExpr if it is not CallExpr but not sure whether runCheckersForEvalCall should aware of CXXConstructExpr.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

586

Updated to use EvalCallOptions()

[vrnithinkumar]

clang-format fix

[NoQ]

Thanks! I'll commit.

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

668–669

Wait, was it that easy?

Yeah, looks like the cast is indeed unnecessary. Somebody probably meant something when they put it here but they didn't bless us with a comment and i don't see any immediate reasons why would this code require a CallExpr specifically.

The TODO above is about supporting the situation when there is no expression *at all*. Like, CXXConstructExpr is still *something*, but in some cases the call isn't a result of evaluating any expression at all. One of the most common examples of such calls is destructor calls. Say, automatic destructors of local variables get triggered at the end of scope rather than at an expression. We'll have to get back to it when we evalCall() a destructor :)

[NoQ]

@vrnithinkumar what's your preferred Full Name <email> for llvm's git?

[vrnithinkumar]

In D82256#2113233, @NoQ wrote:

@vrnithinkumar what's your preferred Full Name <email> for llvm's git?

Nithin Vadukkumchery Rajendrakumar <[e-mail]>

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

CmdLineOption<Boolean,
                   Hide>,
     CmdLineOption<Boolean,
                   "PostStmtOffsetOfExpr",
                   "",
                   "false",
                   Released,
                   Hide>,
     CmdLineOption<Boolean,
+                  "EvalCall",
+                  "",
+                  "false",
+                  Released,
+                  Hide>,
+    CmdLineOption<Boolean,
                   "PreCall",
                   "",
                   "false",
                   Released,
                   Hide>,
     CmdLineOption<Boolean,
                   "PostCall",
                   "",

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h

 class CallEvent;
 class CheckerBase;
 class CheckerContext;
 class CheckerRegistry;
 class ExplodedGraph;
 class ExplodedNode;
 class ExplodedNodeSet;
 class ExprEngine;
+struct EvalCallOptions;
 class MemRegion;
 struct NodeBuilderContext;
 class ObjCMethodCall;
 class RegionAndSymbolInvalidationTraits;
 class SVal;
 class SymbolReaper;
 
 template <typename T> class CheckerFn;
//===----------------------------------------------------------------------===//
 
   /// Run checkers for handling assumptions on symbolic values.
   ProgramStateRef runCheckersForEvalAssume(ProgramStateRef state,
                                            SVal Cond, bool Assumption);
 
   /// Run checkers for evaluating a call.
   ///
   /// Warning: Currently, the CallEvent MUST come from a CallExpr!
-  void runCheckersForEvalCall(ExplodedNodeSet &Dst,
-                              const ExplodedNodeSet &Src,
-                              const CallEvent &CE, ExprEngine &Eng);
+  void runCheckersForEvalCall(ExplodedNodeSet &Dst, const ExplodedNodeSet &Src,
+                              const CallEvent &CE, ExprEngine &Eng,
+                              const EvalCallOptions &CallOpts);
 
   /// Run checkers for the entire Translation Unit.
   void runCheckersOnEndOfTranslationUnit(const TranslationUnitDecl *TU,
                                          AnalysisManager &mgr,
                                          BugReporter &BR);
 
   /// Run checkers for debug-printing a ProgramState.
   ///

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

 struct NodeBuilderContext;
 class NodeBuilderWithSinks;
 class ProgramState;
 class ProgramStateManager;
 class RegionAndSymbolInvalidationTraits;
 class SymbolManager;
 class SwitchNodeBuilder;
 
-class ExprEngine {
-  void anchor();
-public:
-  /// The modes of inlining, which override the default analysis-wide settings.
-  enum InliningModes {
-    /// Follow the default settings for inlining callees.
-    Inline_Regular = 0,
-
-    /// Do minimal inlining of callees.
-    Inline_Minimal = 0x1
-  };
-
-  /// Hints for figuring out of a call should be inlined during evalCall().
-  struct EvalCallOptions {
-    /// This call is a constructor or a destructor for which we do not currently
-    /// compute the this-region correctly.
-    bool IsCtorOrDtorWithImproperlyModeledTargetRegion = false;
 
-    /// This call is a constructor or a destructor for a single element within
-    /// an array, a part of array construction or destruction.
-    bool IsArrayCtorOrDtor = false;
 
-    /// This call is a constructor or a destructor of a temporary value.
-    bool IsTemporaryCtorOrDtor = false;
 
-    /// This call is a constructor for a temporary that is lifetime-extended
-    /// by binding it to a reference-type field within an aggregate,
-    /// for example 'A { const C &c; }; A a = { C() };'
-    bool IsTemporaryLifetimeExtendedViaAggregate = false;
 
-    /// This call is a pre-C++17 elidable constructor that we failed to elide
-    /// because we failed to compute the target region into which
-    /// this constructor would have been ultimately elided. Analysis that
-    /// we perform in this case is still correct but it behaves differently,
-    /// as if copy elision is disabled.
-    bool IsElidableCtorThatHasNotBeenElided = false;
 
-    EvalCallOptions() {}
-  };
 
+class ExprEngine {
+  void anchor();
+
+public:
+  /// The modes of inlining, which override the default analysis-wide settings.
+  enum InliningModes {
+    /// Follow the default settings for inlining callees.
+    Inline_Regular = 0,
+
+    /// Do minimal inlining of callees.
+    Inline_Minimal = 0x1
+  };
+
 private:
   cross_tu::CrossTranslationUnitContext &CTU;
 
   AnalysisManager &AMgr;
 
   AnalysisDeclContextManager &AnalysisDeclContexts;
 
   CoreEngine Engine;

clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

: public Checker<
           check::PreStmt<ArraySubscriptExpr>,
           check::PostStmt<ArraySubscriptExpr>, check::PreStmt<CXXNewExpr>,
           check::PostStmt<CXXNewExpr>, check::PreStmt<CXXDeleteExpr>,
           check::PostStmt<CXXDeleteExpr>, check::PreStmt<CXXConstructExpr>,
           check::PostStmt<CXXConstructExpr>, check::PreStmt<OffsetOfExpr>,
           check::PostStmt<OffsetOfExpr>, check::PreCall, check::PostCall,
           check::EndFunction, check::EndAnalysis, check::NewAllocator,
           check::Bind, check::PointerEscape, check::RegionChanges,
-          check::LiveSymbols> {
+          check::LiveSymbols, eval::Call> {
 
   bool isCallbackEnabled(const AnalyzerOptions &Opts,
                          StringRef CallbackName) const {
     return Opts.getCheckerBooleanOption(this, "*") ||
            Opts.getCheckerBooleanOption(this, CallbackName);
   }
 
   bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
if (isCallbackEnabled(C, "PreStmtOffsetOfExpr"))
       llvm::errs() << "PreStmt<OffsetOfExpr>\n";
   }
 
   void checkPostStmt(const OffsetOfExpr *OOE, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostStmtOffsetOfExpr"))
       llvm::errs() << "PostStmt<OffsetOfExpr>\n";
   }
 
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "EvalCall")) {
+      llvm::errs() << "EvalCall";
+      if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl()))
+        llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
+      llvm::errs() << " {argno: " << Call.getNumArgs() << '}';
+      llvm::errs() << " [" << Call.getKindAsString() << ']';
+      llvm::errs() << '\n';
+      return true;
+    }
+    return false;
+  }
+
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PreCall")) {
       llvm::errs() << "PreCall";
       if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl()))
         llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
       llvm::errs() << " [" << Call.getKindAsString() << ']';
       llvm::errs() << '\n';
     }

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

 }
 
 Optional<SVal>
 CallEvent::getReturnValueUnderConstruction() const {
   const auto *CC = getConstructionContext();
   if (!CC)
     return None;
 
-  ExprEngine::EvalCallOptions CallOpts;
+  EvalCallOptions CallOpts;
   ExprEngine &Engine = getState()->getStateManager().getOwningEngine();
   SVal RetVal =
     Engine.computeObjectUnderConstruction(getOriginExpr(), getState(),
                                           getLocationContext(), CC, CallOpts);
   return RetVal;
 }
 
 ArrayRef<ParmVarDecl*> AnyFunctionCall::parameters() const {

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

CheckerManager::runCheckersForEvalAssume(ProgramStateRef state,
   return state;
 }
 
 /// Run checkers for evaluating a call.
 /// Only one checker will evaluate the call.
 void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
                                             const ExplodedNodeSet &Src,
                                             const CallEvent &Call,
-                                            ExprEngine &Eng) {
+                                            ExprEngine &Eng,
+                                            const EvalCallOptions &CallOpts) {
   for (auto *const Pred : Src) {
     bool anyEvaluated = false;
 
     ExplodedNodeSet checkDst;
     NodeBuilder B(Pred, checkDst, Eng.getBuilderContext());
 
     // Check if any of the EvalCall callbacks can evaluate the call.
     for (const auto &EvalCallChecker : EvalCallCheckers) {
       // TODO: Support the situation when the call doesn't correspond
       // to any Expr.
       ProgramPoint L = ProgramPoint::getProgramPoint(
-          cast<CallExpr>(Call.getOriginExpr()),
-          ProgramPoint::PostStmtKind,
-          Pred->getLocationContext(),
-          EvalCallChecker.Checker);
+          Call.getOriginExpr(), ProgramPoint::PostStmtKind,
+          Pred->getLocationContext(), EvalCallChecker.Checker);
       bool evaluated = false;
       { // CheckerContext generates transitions(populates checkDest) on
         // destruction, so introduce the scope to make sure it gets properly
         // populated.
         CheckerContext C(B, Eng, Pred, L);
         evaluated = EvalCallChecker(Call, C);
       }
       assert(!(evaluated && anyEvaluated)
              && "There are more than one checkers evaluating the call");
       if (evaluated) {
         anyEvaluated = true;
         Dst.insert(checkDst);
 #ifdef NDEBUG
         break; // on release don't check that no other checker also evals.
 #endif
       }
     }
 
     // If none of the checkers evaluated the call, ask ExprEngine to handle it.
     if (!anyEvaluated) {
       NodeBuilder B(Pred, Dst, Eng.getBuilderContext());
-      Eng.defaultEvalCall(B, Pred, Call);
+      Eng.defaultEvalCall(B, Pred, Call, CallOpts);
     }
   }
 }
 
 /// Run checkers for the entire Translation Unit.
 void CheckerManager::runCheckersOnEndOfTranslationUnit(
                                                   const TranslationUnitDecl *TU,
                                                   AnalysisManager &mgr,

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

if (CE && CE->getConstructor()->isTrivial() &&
     // FIXME: Handle other kinds of trivial constructors as well.
     for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
          I != E; ++I)
       performTrivialCopy(Bldr, *I, *Call);
 
   } else {
     for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
          I != E; ++I)
-      defaultEvalCall(Bldr, *I, *Call, CallOpts);
+      getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call, *this,
+                                                 CallOpts);
   }
 
   // If the CFG was constructed without elements for temporary destructors
   // and the just-called constructor created a temporary object then
   // stop exploration if the temporary object has a noreturn constructor.
   // This can lose coverage because the destructor, if it were present
   // in the CFG, would be called at the end of the full expression or
   // later (for life-time extended temporaries) -- but avoids infeasible

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
   ExplodedNodeSet dstPreVisit;
   getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred,
                                             Call, *this);
 
   // Actually evaluate the function call.  We try each of the checkers
   // to see if the can evaluate the function call, and get a callback at
   // defaultEvalCall if all of them fail.
   ExplodedNodeSet dstCallEvaluated;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,
-                                             Call, *this);
+                                             Call, *this, EvalCallOptions());
 
   // If there were other constructors called for object-type arguments
   // of this call, clean them up.
   ExplodedNodeSet dstArgumentCleanup;
   for (ExplodedNode *I : dstCallEvaluated)
     finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
   ExplodedNodeSet dstPostCall;
void ExprEngine::conservativeEvalCall(const CallEvent &Call, NodeBuilder &Bldr,
 
   // And make the result node.
   Bldr.generateNode(Call.getProgramPoint(), State, Pred);
 }
 
 ExprEngine::CallInlinePolicy
 ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
                               AnalyzerOptions &Opts,
-                              const ExprEngine::EvalCallOptions &CallOpts) {
+                              const EvalCallOptions &CallOpts) {
   const LocationContext *CurLC = Pred->getLocationContext();
   const StackFrameContext *CallerSFC = CurLC->getStackFrame();
   switch (Call.getKind()) {
   case CE_Function:
   case CE_Block:
     break;
   case CE_CXXMember:
   case CE_CXXMemberOperator:

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
 // CHECK-NEXT: ctu-invocation-list = invocations.yaml
 // CHECK-NEXT: deadcode.DeadStores:ShowFixIts = false
 // CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = true
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
 // CHECK-NEXT: debug.AnalysisOrder:EndAnalysis = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
+// CHECK-NEXT: debug.AnalysisOrder:EvalCall = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
 // CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXConstructExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXDeleteExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false

clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp

+// RUN: %clang_analyze_cc1 %s \
+// RUN:  -analyzer-checker=debug.AnalysisOrder \
+// RUN:  -analyzer-config debug.AnalysisOrder:EvalCall=true \
+// RUN:  -analyzer-config debug.AnalysisOrder:PreCall=true \
+// RUN:  -analyzer-config debug.AnalysisOrder:PostCall=true \
+// RUN:  2>&1 | FileCheck %s
+
+// This test ensures that eval::Call event will be triggered for constructors.
+
+class C {
+public:
+  C(){};
+  C(int x){};
+  C(int x, int y){};
+};
+
+void foo() {
+  C C0;
+  C C1(42);
+  C *C2 = new C{2, 3};
+}
+
+// CHECK:  PreCall (C::C) [CXXConstructorCall]
+// CHECK-NEXT:  EvalCall (C::C) {argno: 0} [CXXConstructorCall]
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
+// CHECK-NEXT:  PreCall (C::C) [CXXConstructorCall]
+// CHECK-NEXT:  EvalCall (C::C) {argno: 1} [CXXConstructorCall]
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
+// CHECK-NEXT:  PreCall (operator new) [CXXAllocatorCall]
+// CHECK-NEXT:  PostCall (operator new) [CXXAllocatorCall]
+// CHECK-NEXT:  PreCall (C::C) [CXXConstructorCall]
+// CHECK-NEXT:  EvalCall (C::C) {argno: 2} [CXXConstructorCall]
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]

clang/test/Analysis/new-ctor-conservative.cpp

void checkNewPOD() {
   int *k = new int(5);
   clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
 }
 
 void checkNewArray() {
   S *s = new S[10];
   // FIXME: Should be true once we inline array constructors.
   clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(s[1].x == 1); // expected-warning{{UNKNOWN}}
 }
 
 struct NullS {
   NullS() {
     if (this) {}
   }
   NullS(int x) {
     if (!this) {