subject:"\[PATCH\] D69745\: \[analyzer\] Checker\: check\:\:BeginAnalysis"

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

In D69745#1732744 , @Charusso wrote:

> I think every `UnknownSpaceRegion` stuff is tainted, and that is a strong 
> mark here.

Mmm, no, that's definitely not true. Basically, if you analyze a method of a 
class as your top frame, you have your whole class reside in 
`UnknownSpaceRegion`, being represented as `SymRegion{reg_$0}`. It 
doesn't mean that there are no contracts that constrain possible values of 
fields within the class.

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

https://reviews.llvm.org/D69745

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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso abandoned this revision.
Charusso marked an inline comment as done.
Charusso added a comment.

In D69745#1732739 , @NoQ wrote:

> Basically this, but the other way round: your `BeginAnalysis` is 
> `BeginFunction` with extra steps (namely, checking `C.inTopFrame()`).
>
> Backstory: I wanted to add `BeginAnalysis` a few years ago because i wanted 
> to mark `argc` and `argv` as tainted when analyzing `main()`. But then i 
> noticed that we already have `BeginFunction` so i decided not to add it. No, 
> i didn't mark them as tainted yet :/


Hm, that is a cool workaround. I think every `UnknownSpaceRegion` stuff is 
tainted, and that is a strong mark here.


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

https://reviews.llvm.org/D69745



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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

> I think we need to merge the `BeginFunction` into the `BeginWorklist`.

Basically this, but the other way round: your `BeginAnalysis` is 
`BeginFunction` with extra steps (namely, checking `C.inTopFrame()`).

Backstory: I wanted to add `BeginAnalysis` a few years ago because i wanted to 
mark `argc` and `argv` as tainted when analyzing `main()`. But then i noticed 
that we already have `BeginFunction` so i decided not to add it. No, i didn't 
mark them as tainted yet :/

> YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely 
> on the analysis not actually crashing.

There are currently two such checkers: `debug.ViewExplodedGraph` and 
`debug.Stats`. It doesn't make sense to move them to `BeginAnalysis` though, 
because the data that they're dumping isn't available yet. If the checker dumps 
data that isn't collected during path sensitive analysis, it should subscribe 
to `ASTCodeBody` instead. This way activating the checker won't trigger 
path-sensitive analysis to occur.


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

https://reviews.llvm.org/D69745



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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso marked 3 inline comments as done.
Charusso added a comment.

In D69745#1732348 , @Szelethus wrote:

> YES PLEASE. Debug checkers that only dump from `check::EndAnalysis` won't 
> rely on the analysis not actually crashing. Which is ironically exactly when 
> we want to use them.


Thanks for the idea! I am not that cool with debug checkers and I think you are 
the first user who made a creative decision how to use this. My use case is the 
`Preprocessor` only if I do not touch the checker-registry based construction.




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:199
+  ///
+  /// This callback should be used to construct the checker's fields.
+  ///

Szelethus wrote:
> Szelethus wrote:
> > Hmmm, what use case do you have in mind? Do we actually want this to 
> > happen? By the time checkers are constructed, the AST is already built, 
> > alongside all the other data structures that will be inspected by the 
> > analyzer. Since checkers are constructed once for each clang invocation 
> > (not for every `BeginAnalysis` -- `EndAnalysis` cycle), I would imagine 
> > that the checker registry functions are more appropriate for such 
> > initializations.
> > 
> > I'm only aware of one non-debug checker that uses `EndAnalysis` (maybe 
> > `RetailCount`?), that clears some sort of a locally stored map that it 
> > shouldn't really have anyways (since checkers are supposed to be 
> > stateless), and I struggle to think of a realistic, supported use case for 
> > this callback other than debug checkers.
> Turns out `deadcode.UnreachableCode` uses `EndAnalysis` quite cleverly as 
> well.
Yes, good point. It is made for the `Preprocessor` mainly which is very 
path-sensitive, but that is not that good idea to construct e.g. the `BugType` 
multiple times. So that, I do not want to restrict creativity and I have 
removed that line.


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

https://reviews.llvm.org/D69745



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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso updated this revision to Diff 227710.
Charusso edited the summary of this revision.
Charusso added a comment.

- Do not restrict what you supposed to do with the callback.


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

https://reviews.llvm.org/D69745

Files:
  clang/include/clang/StaticAnalyzer/Core/Checker.h
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
  clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/traversal-begin-end-function.c

Index: clang/test/Analysis/traversal-begin-end-function.c
===
--- clang/test/Analysis/traversal-begin-end-function.c
+++ clang/test/Analysis/traversal-begin-end-function.c
@@ -2,17 +2,9 @@
 
 void inline_callee(int i);
 
-// CHECK: --BEGIN FUNCTION--
 void inline_caller() {
-  // CHECK: --BEGIN FUNCTION--
-  // CHECK: --BEGIN FUNCTION--
-  // CHECK: --BEGIN FUNCTION--
   inline_callee(3);
-  // CHECK: --END FUNCTION--
-  // CHECK: --END FUNCTION--
-  // CHECK: --END FUNCTION--
 }
-// CHECK: --END FUNCTION--
 
 void inline_callee(int i) {
   if (i <= 1)
@@ -20,3 +12,17 @@
 
   inline_callee(i - 1);
 }
+
+// CHECK:  --BEGIN ANALYSIS--
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: IfStmt
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: IfStmt
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: IfStmt
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END ANALYSIS--
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -620,6 +620,13 @@
IsDot);
 }
 
+void ExprEngine::processBeginWorklist(NodeBuilderContext ,
+  ExplodedNode *Pred, ExplodedNodeSet ,
+  const BlockEdge ) {
+  SaveAndRestore NodeContextRAII(currBldrCtx, );
+  getCheckerManager().runCheckersForBeginAnalysis(Dst, L, Pred, *this);
+}
+
 void ExprEngine::processEndWorklist() {
   getCheckerManager().runCheckersForEndAnalysis(G, BR, *this);
 }
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -113,6 +113,8 @@
 
 NodeBuilderContext BuilderCtx(*this, StartLoc.getDst(), Node);
 ExplodedNodeSet DstBegin;
+
+SubEng.processBeginWorklist(BuilderCtx, Node, DstBegin, StartLoc);
 SubEng.processBeginOfFunction(BuilderCtx, Node, DstBegin, StartLoc);
 
 enqueue(DstBegin);
Index: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -399,6 +399,41 @@
   expandGraphWithCheckers(C, Dst, Src);
 }
 
+namespace {
+struct CheckBeginAnalysisContext {
+  using CheckersTy = std::vector;
+
+  const CheckersTy 
+  ExprEngine 
+  const ProgramPoint 
+
+  CheckBeginAnalysisContext(const CheckersTy , ExprEngine ,
+const ProgramPoint )
+  : Checkers(Checkers), Eng(Eng), PP(PP) {}
+
+  CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
+  CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
+
+  void runChecker(CheckerManager::CheckBeginAnalysisFunc checkFn,
+  NodeBuilder , ExplodedNode *Pred) {
+const ProgramPoint  = PP.withTag(checkFn.Checker);
+CheckerContext C(Bldr, Eng, Pred, L);
+
+checkFn(C);
+  }
+};
+} // namespace
+
+void CheckerManager::runCheckersForBeginAnalysis(ExplodedNodeSet ,
+ const BlockEdge ,
+ ExplodedNode *Pred,
+ ExprEngine ) {
+  ExplodedNodeSet Src;
+  Src.insert(Pred);
+  CheckBeginAnalysisContext C(BeginAnalysisCheckers, Eng, L);
+  expandGraphWithCheckers(C, Dst, Src);
+}
+
 void CheckerManager::runCheckersForEndAnalysis(ExplodedGraph ,
BugReporter ,
ExprEngine ) {
@@ -827,6 +862,10 @@
   BindCheckers.push_back(checkfn);
 }
 
+void

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

YES PLEASE. Debug checkers that only dump from `check::EndAnalysis` won't rely 
on the analysis not actually crashing. Which is ironically exactly when we want 
to use them.




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:199
+  ///
+  /// This callback should be used to construct the checker's fields.
+  ///

Hmmm, what use case do you have in mind? Do we actually want this to happen? By 
the time checkers are constructed, the AST is already built, alongside all the 
other data structures that will be inspected by the analyzer. Since checkers 
are constructed once for each clang invocation (not for every `BeginAnalysis` 
-- `EndAnalysis` cycle), I would imagine that the checker registry functions 
are more appropriate for such initializations.

I'm only aware of one non-debug checker that uses `EndAnalysis` (maybe 
`RetailCount`?), that clears some sort of a locally stored map that it 
shouldn't really have anyways (since checkers are supposed to be stateless), 
and I struggle to think of a realistic, supported use case for this callback 
other than debug checkers.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69745



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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-04 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:199
+  ///
+  /// This callback should be used to construct the checker's fields.
+  ///

Szelethus wrote:
> Hmmm, what use case do you have in mind? Do we actually want this to happen? 
> By the time checkers are constructed, the AST is already built, alongside all 
> the other data structures that will be inspected by the analyzer. Since 
> checkers are constructed once for each clang invocation (not for every 
> `BeginAnalysis` -- `EndAnalysis` cycle), I would imagine that the checker 
> registry functions are more appropriate for such initializations.
> 
> I'm only aware of one non-debug checker that uses `EndAnalysis` (maybe 
> `RetailCount`?), that clears some sort of a locally stored map that it 
> shouldn't really have anyways (since checkers are supposed to be stateless), 
> and I struggle to think of a realistic, supported use case for this callback 
> other than debug checkers.
Turns out `deadcode.UnreachableCode` uses `EndAnalysis` quite cleverly as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69745



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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-01 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso marked an inline comment as done.
Charusso added a comment.

I felt that it will be easier, eh.




Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:118
+SubEng.processBeginWorklist(BuilderCtx, Node, DstBegin, StartLoc);
 SubEng.processBeginOfFunction(BuilderCtx, Node, DstBegin, StartLoc);
 

I think we need to merge the `BeginFunction` into the `BeginWorklist`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69745



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

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

2019-11-01 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Charusso added a parent revision: D69731: [analyzer] CheckerContext: Make the 
Preprocessor available.

This callback is called when the analysis starts. It could be used to
construct the fields of the checker. Here is an example:

  void CoolFuncChecker::checkBeginAnalysis(CheckerContext ) const {
CheckerManager  = C.getCheckerManager();
  
CheckCoolFunctions = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
this, "WantToCheckCoolFunctions");
  
BT = std::make_unique(Mgr.getCurrentCheckerName(),
   "Cool function call", categories::TooCool);
  
Preprocessor  = C.getPreprocessor();
UseMyCoolLib = PP.isMacroDefined("__MY_COOL_LIB__");
  }


Repository:
  rC Clang

https://reviews.llvm.org/D69745

Files:
  clang/include/clang/StaticAnalyzer/Core/Checker.h
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
  clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/traversal-begin-end-function.c

Index: clang/test/Analysis/traversal-begin-end-function.c
===
--- clang/test/Analysis/traversal-begin-end-function.c
+++ clang/test/Analysis/traversal-begin-end-function.c
@@ -2,17 +2,9 @@
 
 void inline_callee(int i);
 
-// CHECK: --BEGIN FUNCTION--
 void inline_caller() {
-  // CHECK: --BEGIN FUNCTION--
-  // CHECK: --BEGIN FUNCTION--
-  // CHECK: --BEGIN FUNCTION--
   inline_callee(3);
-  // CHECK: --END FUNCTION--
-  // CHECK: --END FUNCTION--
-  // CHECK: --END FUNCTION--
 }
-// CHECK: --END FUNCTION--
 
 void inline_callee(int i) {
   if (i <= 1)
@@ -20,3 +12,17 @@
 
   inline_callee(i - 1);
 }
+
+// CHECK:  --BEGIN ANALYSIS--
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: IfStmt
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: IfStmt
+// CHECK-NEXT: --BEGIN FUNCTION--
+// CHECK-NEXT: IfStmt
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END FUNCTION--
+// CHECK-NEXT: --END ANALYSIS--
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -620,6 +620,13 @@
IsDot);
 }
 
+void ExprEngine::processBeginWorklist(NodeBuilderContext ,
+  ExplodedNode *Pred, ExplodedNodeSet ,
+  const BlockEdge ) {
+  SaveAndRestore NodeContextRAII(currBldrCtx, );
+  getCheckerManager().runCheckersForBeginAnalysis(Dst, L, Pred, *this);
+}
+
 void ExprEngine::processEndWorklist() {
   getCheckerManager().runCheckersForEndAnalysis(G, BR, *this);
 }
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -113,6 +113,8 @@
 
 NodeBuilderContext BuilderCtx(*this, StartLoc.getDst(), Node);
 ExplodedNodeSet DstBegin;
+
+SubEng.processBeginWorklist(BuilderCtx, Node, DstBegin, StartLoc);
 SubEng.processBeginOfFunction(BuilderCtx, Node, DstBegin, StartLoc);
 
 enqueue(DstBegin);
Index: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -399,6 +399,41 @@
   expandGraphWithCheckers(C, Dst, Src);
 }
 
+namespace {
+struct CheckBeginAnalysisContext {
+  using CheckersTy = std::vector;
+
+  const CheckersTy 
+  ExprEngine 
+  const ProgramPoint 
+
+  CheckBeginAnalysisContext(const CheckersTy , ExprEngine ,
+const ProgramPoint )
+  : Checkers(Checkers), Eng(Eng), PP(PP) {}
+
+  CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
+  CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
+
+  void runChecker(CheckerManager::CheckBeginAnalysisFunc checkFn,
+  NodeBuilder , ExplodedNode *Pred) {
+const ProgramPoint  = PP.withTag(checkFn.Checker);
+CheckerContext C(Bldr, Eng, Pred, L);
+
+checkFn(C);
+  }
+};
+} //

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

9 matches

Site Navigation

Mail list logo

Footer information