[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD abandoned this revision.
RedDocMD added a comment.

I am closing this since it has been addressed much better by the patches from 
@vsavchenko.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D103434#2795940 , @vsavchenko 
wrote:

> I was thinking a lot about this problem after our last call, and even though 
> `StoppableVisitor` helps to solve the problem that we have, it is extremely 
> unclear when this callback is called and should be called.
> I decided to restore the balance (and rid of all younglings, maybe) and put 
> together this interface: D103605 .  I still 
> need to migrate the existing code into the new architecture, but it can be 
> done incrementally.

Right. I then perhaps should focus now on D98726 
, and then return to `GetNoteEmitter`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I was thinking a lot about this problem after our last call, and even though 
`StoppableVisitor` helps to solve the problem that we have, it is extremely 
unclear when this callback is called and should be called.
I decided to restore the balance (and rid of all younglings, maybe) and put 
together this interface: D103605 .  I still 
need to migrate the existing code into the new architecture, but it can be done 
incrementally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:95
+  /// This method is run once the \ref Stop method is called.
+  virtual void OnStop(const ExplodedNode *Curr, BugReporterContext ,
+  PathSensitiveBugReport ) const = 0;

According to the conventions, should use `stop` and `onStop` instead of `Stop`, 
`OnStop`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Tests to verify that ReturnVisitor actually does what we intend it to do 
> (call the callback at the right place).

You mean write a test that demonstrates that? I guess unless we're willing to 
wait for the checker to catch up, a good approach to this would be to write a 
unit test. See `unittests/StaticAnalyzer` - there aren't many of them because 
they're relatively hard to write but these days they're getting more and more 
popular as we're trying to eliminate mutually exclusive bugs invisible on LIT 
tests. You can mock some code, emit a warning against it from a mocked checker, 
and test directly that the visitor is stopped at, say, a given node or a given 
line number.

Or you mean like, add an assert that ensures that you're doing right thing? I 
think this is also possible to some extent. For example, you can check when the 
visitor is stopped that no other visitors were added during the current 
`VisitNode()` invocation (otherwise the process has to continue). Or you could 
assert the opposite: if the visitor has run to completion and no other visitors 
were added then the callback has to be invoked. I think this can get messy 
really quickly (esp. knowing that you can't simply count all visitors to see if 
more were added, given how newly added visitors may be duplicates of existing 
visitors). But I suspect that it could be easy to check this in at least one 
direction.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:109
+// Callback type for trackExpressionValue
+using VisitorCallback = llvm::function_ref;

I think we have to use `std::function` here.
>   lang=c++
>   /// An efficient, type-erasing, non-owning reference to a callable. This is
>   /// intended for use as the type of a function parameter that is not used
>   /// after the function in question returns.
>   ///
>   /// This class does not own the callable, so it is not in general safe to 
> store
>   /// a function_ref.
>   template class function_ref;

An `llvm::function_ref` becomes a dangling reference as soon as 
`trackExpressionValue()` returns. This is too early.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

The following things are now done:

- `trackExpressionValue` must receive a callback
- The callback must be passed to a visitor (`ReturnVisitor` for now)

I still don't know a good way to test the following:

- Tests to verify that `ReturnVisitor` actually does what we intend it to do 
(call the callback at the right place).

@vsavchenko, @NoQ, @xazax.hun, @teemperor can you give some suggestions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 349295.
RedDocMD added a comment.

trackExpressionValue recieving callback, passing it to ReturnVisitor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -320,6 +320,22 @@
   return P;
 }
 
+//===--===//
+// Implementation of StoppableBugReporterVisitor
+//===--===//
+
+StoppableBugReporterVisitor::~StoppableBugReporterVisitor() {
+  assert(Stopped && "Stoppable visitor not stopped in its lifetime");
+}
+
+void StoppableBugReporterVisitor::Stop(const ExplodedNode *Curr,
+   BugReporterContext ,
+   PathSensitiveBugReport ) {
+  assert(!Stopped && "Stop() can be called only once in visitor's lifetime");
+  Stopped = true;
+  OnStop(Curr, BRC, BR);
+}
+
 //===--===//
 // Implementation of NoStoreFuncVisitor.
 //===--===//
@@ -883,7 +899,7 @@
 ///
 /// This visitor is intended to be used when another visitor discovers that an
 /// interesting value comes from an inlined function call.
-class ReturnVisitor : public BugReporterVisitor {
+class ReturnVisitor : public StoppableBugReporterVisitor {
   const StackFrameContext *CalleeSFC;
   enum {
 Initial,
@@ -895,12 +911,14 @@
   bool ShouldInvalidate = true;
   AnalyzerOptions& Options;
   bugreporter::TrackingKind TKind;
+  VisitorCallback Callback;
 
 public:
   ReturnVisitor(const StackFrameContext *Frame, bool Suppressed,
-AnalyzerOptions , bugreporter::TrackingKind TKind)
-  : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
-Options(Options), TKind(TKind) {}
+AnalyzerOptions , bugreporter::TrackingKind TKind,
+VisitorCallback &)
+  : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed), Options(Options),
+TKind(TKind), Callback(Callback) {}
 
   static void *getTag() {
 static int Tag = 0;
@@ -913,6 +931,13 @@
 ID.AddBoolean(EnableNullFPSuppression);
   }
 
+  void OnStop(const ExplodedNode *Curr, BugReporterContext ,
+  PathSensitiveBugReport ) const override {
+if (Callback) {
+  Callback(Curr, BRC, BR);
+}
+  }
+
   /// Adds a ReturnVisitor if the given statement represents a call that was
   /// inlined.
   ///
@@ -923,7 +948,8 @@
   static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
 PathSensitiveBugReport ,
 bool InEnableNullFPSuppression,
-bugreporter::TrackingKind TKind) {
+bugreporter::TrackingKind TKind,
+VisitorCallback Callback) {
 if (!CallEvent::isCallStmt(S))
   return;
 
@@ -994,9 +1020,9 @@
   if (Optional RetLoc = RetVal.getAs())
 EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
-BR.addVisitor(std::make_unique(CalleeContext,
-   EnableNullFPSuppression,
-   Options, TKind));
+BR.addVisitor(
+std::make_unique(CalleeContext, EnableNullFPSuppression,
+Options, TKind, std::move(Callback)));
   }
 
   PathDiagnosticPieceRef visitNodeInitial(const ExplodedNode *N,
@@ -1123,6 +1149,10 @@
 else
   BR.markInteresting(CalleeSFC);
 
+// Since we know that no other notes will be emitted subsequently by this
+// visitor.
+Stop(N, BRC, BR);
+
 return EventPiece;
   }
 
@@ -1966,7 +1996,8 @@
const Expr *E,
PathSensitiveBugReport ,
bugreporter::TrackingKind TKind,
-   bool EnableNullFPSuppression) {
+   bool EnableNullFPSuppression,
+   VisitorCallback Callback) {
 
   if (!E || !InputNode)
 return false;
@@ -2067,7 +2098,7 @@
   SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
 
   ReturnVisitor::addVisitorIfNecessary(
-LVNode, Inner, report, EnableNullFPSuppression, TKind);
+  LVNode, Inner, report, 

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:95
+  /// This method is run once the \ref Stop method is called.
+  virtual void OnStop(const ExplodedNode *Curr, BugReporterContext ,
+  PathSensitiveBugReport ) const = 0;

xazax.hun wrote:
> Are we anticipating visitors that do something else other than calling a 
> callback when they are stopped? If that is not the case, I wonder if 
> storing/invoking the visitor in this class would make more sense. 
I think it was decided in the last meet that we should not make this new 
abstraction overtly  //specific//. So a stoppable visitor is free to have a 
callback (like the `ReturnVisitor` now does) and is also free to have some more 
behavior in the `OnStop` method. Also making the `OnStop` method pure-virtual 
means that the sub-classes are required to override it (so it can't be ignored).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:97
+  PathSensitiveBugReport ) const = 0;
+  bool Stopped;
+

xazax.hun wrote:
> Should we allow derived classes or even others query if a visitor was 
> stopped? Or is this only for catching bugs? 
Right now it can't be queried. The purpose of the `Stopped` field is primarily 
for catching bugs. (this ensures visitors are stopped exactly //once//).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:104
+  /// Call this method once the visitor runs out of useful nodes to process.
+  void Stop(const ExplodedNode *Curr, BugReporterContext ,
+PathSensitiveBugReport );

xazax.hun wrote:
> Do you anticipate cases where the visitor itself does not know that it needs 
> to stop (e.g. in the Visit functions) so someone needs to call this from the 
> outside?
No, I don't. Are you suggesting I should make it a protected method?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:895
 
+using VisitorCallback = llvm::function_ref;

vsavchenko wrote:
> `function_ref` is a reference, it doesn't own the function. It means that it 
> shouldn't outlive the actual function object (very similar to `StringRef`).
So I guess this means that the callback must be a method/lambda that 
//persists// (ie, it can't be a lambda in a `VisitNode` method, but must be 
stored a field in the class).



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1156
+// visitor.
+Stop(N, BRC, BR);
+

I am **not** entirely certain this is the right place to call the Stop method. 
It seems reasonable but I am not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:95
+  /// This method is run once the \ref Stop method is called.
+  virtual void OnStop(const ExplodedNode *Curr, BugReporterContext ,
+  PathSensitiveBugReport ) const = 0;

Are we anticipating visitors that do something else other than calling a 
callback when they are stopped? If that is not the case, I wonder if 
storing/invoking the visitor in this class would make more sense. 



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:97
+  PathSensitiveBugReport ) const = 0;
+  bool Stopped;
+

Should we allow derived classes or even others query if a visitor was stopped? 
Or is this only for catching bugs? 



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:101
+  StoppableBugReporterVisitor() : Stopped{false} {}
+  virtual ~StoppableBugReporterVisitor();
+

I prefer really small functions, like this dtor to be in header files. Feel 
free to ignore.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:104
+  /// Call this method once the visitor runs out of useful nodes to process.
+  void Stop(const ExplodedNode *Curr, BugReporterContext ,
+PathSensitiveBugReport );

Do you anticipate cases where the visitor itself does not know that it needs to 
stop (e.g. in the Visit functions) so someone needs to call this from the 
outside?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:895
 
+using VisitorCallback = llvm::function_ref;

`function_ref` is a reference, it doesn't own the function. It means that it 
shouldn't outlive the actual function object (very similar to `StringRef`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

**Note: This patch is only partially done.**

The following things are still left:

- `trackExpressionValue` must receive a callback
- The callback must be passed to a visitor (`ReturnVisitor` for now)
- Tests to verify that `ReturnVisitor` actually does what we intend it to do 
(call the callback at the right place).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434

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

[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD created this revision.
RedDocMD added reviewers: NoQ, vsavchenko, xazax.hun, teemperor.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware.
RedDocMD requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch introduces the concept of *Stoppable* visitors, ie, a
visitor that must perform some task when it runs out of nodes on which
it has work to do.
Initially, the `ReturnVisitor` is being made *stoppable*, thus allowing
it to run callbacks on completion. These callbacks are meant to be
supplied from `trackExpressionValue`.
Thus, this patch will also allow `trackExpressionValue` to recieve a
callback to run when it is done tracking.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103434

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -320,6 +320,22 @@
   return P;
 }
 
+//===--===//
+// Implementation of StoppableBugReporterVisitor
+//===--===//
+
+StoppableBugReporterVisitor::~StoppableBugReporterVisitor() {
+  assert(Stopped && "Stoppable visitor not stopped in its lifetime");
+}
+
+void StoppableBugReporterVisitor::Stop(const ExplodedNode *Curr,
+   BugReporterContext ,
+   PathSensitiveBugReport ) {
+  assert(!Stopped && "Stop() can be called only once in visitor's lifetime");
+  Stopped = true;
+  OnStop(Curr, BRC, BR);
+}
+
 //===--===//
 // Implementation of NoStoreFuncVisitor.
 //===--===//
@@ -876,6 +892,9 @@
 
 namespace {
 
+using VisitorCallback = llvm::function_ref;
+
 /// Emits an extra note at the return statement of an interesting stack frame.
 ///
 /// The returned value is marked as an interesting value, and if it's null,
@@ -883,7 +902,7 @@
 ///
 /// This visitor is intended to be used when another visitor discovers that an
 /// interesting value comes from an inlined function call.
-class ReturnVisitor : public BugReporterVisitor {
+class ReturnVisitor : public StoppableBugReporterVisitor {
   const StackFrameContext *CalleeSFC;
   enum {
 Initial,
@@ -895,12 +914,14 @@
   bool ShouldInvalidate = true;
   AnalyzerOptions& Options;
   bugreporter::TrackingKind TKind;
+  VisitorCallback Callback;
 
 public:
   ReturnVisitor(const StackFrameContext *Frame, bool Suppressed,
-AnalyzerOptions , bugreporter::TrackingKind TKind)
-  : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
-Options(Options), TKind(TKind) {}
+AnalyzerOptions , bugreporter::TrackingKind TKind,
+VisitorCallback & = nullptr)
+  : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed), Options(Options),
+TKind(TKind), Callback(Callback) {}
 
   static void *getTag() {
 static int Tag = 0;
@@ -913,6 +934,13 @@
 ID.AddBoolean(EnableNullFPSuppression);
   }
 
+  void OnStop(const ExplodedNode *Curr, BugReporterContext ,
+  PathSensitiveBugReport ) const override {
+if (Callback) {
+  Callback(Curr, BRC, BR);
+}
+  }
+
   /// Adds a ReturnVisitor if the given statement represents a call that was
   /// inlined.
   ///
@@ -1123,6 +1151,10 @@
 else
   BR.markInteresting(CalleeSFC);
 
+// Since we know that no other notes will be emitted subsequently by this
+// visitor.
+Stop(N, BRC, BR);
+
 return EventPiece;
   }
 
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -85,6 +85,26 @@
 const PathSensitiveBugReport );
 };
 
+/// StoppableBugReporterVisitor is a special type of visitor that is known to
+/// not have any useful work to do after it reaches a certain node in its
+/// traversal. When it does reach that node, the \ref Stop method must be
+/// called. This in turn calls the \ref OnStop method (which must be overriden
+/// in the sub-class). Note that such visitors *must* be stopped exactly once.
+class StoppableBugReporterVisitor : public BugReporterVisitor {
+  /// This method is run once the \ref Stop method is called.
+  virtual void