[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked 5 inline comments as done.
vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp:130
+llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
+  llvm::errs() << " {" << Call.getNumArgs() << '}';
+  llvm::errs() << " [" << Call.getKindAsString() << ']';

Szelethus wrote:
> This seems a bit cryptic, how about `{argno:`?
For more clarity, added "{argno:".



Comment at: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:669
   ProgramPoint L = ProgramPoint::getProgramPoint(
-  cast(Call.getOriginExpr()),
+  Call.getOriginExpr(),
   ProgramPoint::PostStmtKind,

This is for fixing the unit test failures. 
Previously `assert` to check the type inside `cast`  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`.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:586
   ExplodedNodeSet dstCallEvaluated;
+  EvalCallOptions CallOpts;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,

NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > If you want you can make it an optional argument of 
> > > `runCheckersForEvalCall()`, like it's done in `defaultEvalCall()`.
> > I tried to make it as default argument for `runCheckersForEvalCall()` but 
> > `struct EvalCallOptions` is forward declared in `CheckerManager.h`.
> Oh well! You probably still don't need a separate local variable, can you try 
> `EvalCallOptions()` or even `{}`?
Updated to use `EvalCallOptions()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 273117.
vrnithinkumar added a comment.

Fixing test failures


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
  clang/test/Analysis/new-ctor-conservative.cpp

Index: clang/test/Analysis/new-ctor-conservative.cpp
===
--- clang/test/Analysis/new-ctor-conservative.cpp
+++ clang/test/Analysis/new-ctor-conservative.cpp
@@ -27,6 +27,7 @@
   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 {
Index: clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
@@ -0,0 +1,33 @@
+// 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]
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -50,6 +50,7 @@
 // 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
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -584,7 +584,7 @@
   // 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.
@@ -717,7 +717,7 @@
 ExprEngine::CallInlinePolicy
 ExprEngine::mayInlineCallKind(const CallEvent , const ExplodedNode *Pred,
   AnalyzerOptions ,
-  const ExprEngine::EvalCallOptions ) {
+  const EvalCallOptions ) {
   const LocationContext *CurLC = Pred->getLocationContext();
   const StackFrameContext *CallerSFC = CurLC->getStackFrame();
   switch (Call.getKind()) {
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -615,7 +615,8 @@
   } else {
 for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
  I != E; ++I)
-  defaultEvalCall(Bldr, *I, *Call, CallOpts);
+  getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call, *this,
+ 

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp:130
+llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
+  llvm::errs() << " {" << Call.getNumArgs() << '}';
+  llvm::errs() << " [" << Call.getKindAsString() << ']';

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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 :)




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:586
   ExplodedNodeSet dstCallEvaluated;
+  EvalCallOptions CallOpts;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,

vrnithinkumar wrote:
> NoQ wrote:
> > If you want you can make it an optional argument of 
> > `runCheckersForEvalCall()`, like it's done in `defaultEvalCall()`.
> I tried to make it as default argument for `runCheckersForEvalCall()` but 
> `struct EvalCallOptions` is forward declared in `CheckerManager.h`.
Oh well! You probably still don't need a separate local variable, can you try 
`EvalCallOptions()` or even `{}`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 272275.
vrnithinkumar marked an inline comment as done.
vrnithinkumar added a comment.

Addressing review comment adding miised new line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256

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


Index: clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
===
--- clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
+++ clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
@@ -30,4 +30,4 @@
 // 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]
\ No newline at end of file
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]


Index: clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
===
--- clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
+++ clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
@@ -30,4 +30,4 @@
 // 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]
\ No newline at end of file
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 272276.
vrnithinkumar marked an inline comment as done.
vrnithinkumar added a comment.

Fixing wrongly deleted the old commit via arc


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
  clang/test/Analysis/new-ctor-conservative.cpp

Index: clang/test/Analysis/new-ctor-conservative.cpp
===
--- clang/test/Analysis/new-ctor-conservative.cpp
+++ clang/test/Analysis/new-ctor-conservative.cpp
@@ -27,6 +27,7 @@
   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 {
Index: clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
@@ -0,0 +1,33 @@
+// 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) {0} [CXXConstructorCall]
+// 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]
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -50,6 +50,7 @@
 // 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
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -583,8 +583,9 @@
   // to see if the can evaluate the function call, and get a callback at
   // defaultEvalCall if all of them fail.
   ExplodedNodeSet dstCallEvaluated;
+  EvalCallOptions CallOpts;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,
- Call, *this);
+ Call, *this, CallOpts);
 
   // If there were other constructors called for object-type arguments
   // of this call, clean them up.
@@ -717,7 +718,7 @@
 ExprEngine::CallInlinePolicy
 ExprEngine::mayInlineCallKind(const CallEvent , const ExplodedNode *Pred,
   AnalyzerOptions ,
-  const ExprEngine::EvalCallOptions ) {
+  const EvalCallOptions ) {
   const LocationContext *CurLC = Pred->getLocationContext();
   const StackFrameContext *CallerSFC = CurLC->getStackFrame();
   switch (Call.getKind()) {
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -615,7 +615,8 @@
   } else {
 for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
  I != E; ++I)
-  defaultEvalCall(Bldr, *I, *Call, 

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked 2 inline comments as done.
vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:586
   ExplodedNodeSet dstCallEvaluated;
+  EvalCallOptions CallOpts;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,

NoQ wrote:
> If you want you can make it an optional argument of 
> `runCheckersForEvalCall()`, like it's done in `defaultEvalCall()`.
I tried to make it as default argument for `runCheckersForEvalCall()` but 
`struct EvalCallOptions` is forward declared in `CheckerManager.h`.



Comment at: clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp:34
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
\ No newline at end of file


NoQ wrote:
> No NEwLiNE aT EnD Of FiLE
added new line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Fantastic, thank you!




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:586
   ExplodedNodeSet dstCallEvaluated;
+  EvalCallOptions CallOpts;
   getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,

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



Comment at: clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp:34
+// CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
\ No newline at end of file


No NEwLiNE aT EnD Of FiLE



Comment at: clang/test/Analysis/new-ctor-conservative.cpp:30
   clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(s[1].x == 1); // expected-warning{{UNKNOWN}}
 }

Thx!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked an inline comment as done.
vrnithinkumar added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:100
+/// 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

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82256/new/

https://reviews.llvm.org/D82256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits