[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 ↗

(On Diff #272251)

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 ↗

(On Diff #272251)

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 ↗

(On Diff #272251)

Thx!

[vrnithinkumar]

Addressing review comment adding miised new line

[vrnithinkumar]

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

586 ↗

(On Diff #272251)

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 ↗

(On Diff #272251)

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 ↗

(On Diff #272276)

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

[vrnithinkumar]

Fixing test failures

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

130 ↗

(On Diff #272276)

For more clarity, added "{argno:".

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

669 ↗

(On Diff #273117)

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 ↗

(On Diff #272251)

Updated to use EvalCallOptions()

[vrnithinkumar]

clang-format fix

[NoQ]

Thanks! I'll commit.

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

669 ↗

(On Diff #273117)

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/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.

 // CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
 // CHECK-NEXT:  PreCall (C::C) [CXXConstructorCall]
 // CHECK-NEXT:  EvalCall (C::C) {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) {2} [CXXConstructorCall]
-// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]