[analyzer] Refactor trackRValueExpression into ExpressionHandler

[vsavchenko]

Rebase

[NoQ]

I guess you should mark all of these commits as NFC?

[vsavchenko]

In D103630#2809658, @NoQ wrote:

I guess you should mark all of these commits as NFC?

I thought about, but they do change one thing: return value of trackExpressionValue. With each commit it's getting more consistent.

[vsavchenko]

Rebase

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2265–2267

I checked this commit out, and failed to see how these low/high priority handlers work out in practice. I tried to

• Make the default handler low prio as well
• Make the PRValue high prio
• All other combinations of the those values
• Change the order of adding these handlers

No tests failed. Is there any one patch so far that demonstrates why we need this? A unit test?

[vsavchenko]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2265–2267

add with high priority == add to the beginning of the queue
add with low priority == add to the end of the queue

This means that it doesn't matter with what priority we add the very first one.
I guess with these two it doesn't matter which order they have one against another, that's why tests don't fail. This code here keeps the same order it was originally in trackExpressionValue to maintain parity.

In the future commits, InterestingLValueHandler HAVE TO BE before before DefaultExpressionHandler and there are tests that validate it. Actually this is the only place that I know that really cares about the order and that's why I needed to add priorities and the way for one handler to cancel all further handlers.

Does this answer your question?

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2265–2267

Perfectly, thank you! :)

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

static const ExplodedNode* findNodeForExpression(const ExplodedNode *N,
   while (N) {
     if (N->getStmtForDiagnostics() == Inner)
       return N;
     N = N->getFirstPred();
   }
   return N;
 }
 
-/// Attempts to add visitors to track an RValue expression back to its point of
-/// origin. Works similarly to trackExpressionValue, but accepts only RValues.
-static void trackRValueExpression(const ExplodedNode *InputNode, const Expr *E,
-                                  PathSensitiveBugReport &report,
-                                  bugreporter::TrackingKind TKind,
-                                  bool EnableNullFPSuppression) {
-  assert(E->isRValue() && "The expression is not an rvalue!");
-  const ExplodedNode *RVNode = findNodeForExpression(InputNode, E);
-  if (!RVNode)
-    return;
-  ProgramStateRef RVState = RVNode->getState();
-  SVal V = RVState->getSValAsScalarOrLoc(E, RVNode->getLocationContext());
-  const auto *BO = dyn_cast<BinaryOperator>(E);
-  if (!BO)
-    return;
-  if (!V.isZeroConstant())
-    return;
-  if (!BO->isMultiplicativeOp())
-    return;
-
-  SVal RHSV = RVState->getSVal(BO->getRHS(), RVNode->getLocationContext());
-  SVal LHSV = RVState->getSVal(BO->getLHS(), RVNode->getLocationContext());
-
-  // Track both LHS and RHS of a multiplication.
-  if (BO->getOpcode() == BO_Mul) {
-    if (LHSV.isZeroConstant())
-      trackExpressionValue(InputNode, BO->getLHS(), report, TKind,
-                           EnableNullFPSuppression);
-    if (RHSV.isZeroConstant())
-      trackExpressionValue(InputNode, BO->getRHS(), report, TKind,
-                           EnableNullFPSuppression);
-  } else { // Track only the LHS of a division or a modulo.
-    if (LHSV.isZeroConstant())
-      trackExpressionValue(InputNode, BO->getLHS(), report, TKind,
-                           EnableNullFPSuppression);
-  }
-}
-
 //===----------------------------------------------------------------------===//
 //                            Tracker implementation
 //===----------------------------------------------------------------------===//
 
 class DefaultExpressionHandler final : public ExpressionHandler {
 public:
   using ExpressionHandler::ExpressionHandler;
 
if (auto L = V.getAs<loc::MemRegionVal>()) {
         Report.markInteresting(RegionRVal, Opts.Kind);
         Report.addVisitor<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal),
             /*assumption=*/false);
         Result.FoundSomethingToTrack = true;
       }
     }
 
-    if (Inner->isRValue())
-      // TODO: Incorporate as Handler
-      trackRValueExpression(LVNode, Inner, Report, Opts.Kind,
-                            Opts.EnableNullFPSuppression);
-
     return Result;
   }
 };
 
+/// Attempts to add visitors to track an RValue expression back to its point of
+/// origin.
+class RValueHandler final : public ExpressionHandler {
+public:
+  using ExpressionHandler::ExpressionHandler;
+
+  Tracker::Result handle(const Expr *E, const ExplodedNode *InputNode,
+                         const ExplodedNode *ExprNode,
+                         TrackingOptions Opts) override {
+    if (!E->isRValue())
+      return {};
+
+    const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
+    if (!RVNode)
+      return {};
+
+    ProgramStateRef RVState = RVNode->getState();
+    SVal V = RVState->getSValAsScalarOrLoc(E, RVNode->getLocationContext());
+    const auto *BO = dyn_cast<BinaryOperator>(E);
+
+    if (!BO || !BO->isMultiplicativeOp() || !V.isZeroConstant())
+      return {};
+
+    SVal RHSV = RVState->getSVal(BO->getRHS(), RVNode->getLocationContext());
+    SVal LHSV = RVState->getSVal(BO->getLHS(), RVNode->getLocationContext());
+
+    // Track both LHS and RHS of a multiplication.
+    Tracker::Result CombinedResult;
+    Tracker &Parent = getParentTracker();
+
+    const auto track = [&CombinedResult, &Parent, ExprNode, Opts](Expr *Inner) {
+      CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
+    };
+
+    if (BO->getOpcode() == BO_Mul) {
+      if (LHSV.isZeroConstant())
+        track(BO->getLHS());
+      if (RHSV.isZeroConstant())
+        track(BO->getRHS());
+    } else { // Track only the LHS of a division or a modulo.
+      if (LHSV.isZeroConstant())
+        track(BO->getLHS());
+    }
+
+    return CombinedResult;
+  }
+};
+
 Tracker::Tracker(PathSensitiveBugReport &Report) : Report(Report) {
   addHighPriorityHandler<DefaultExpressionHandler>();
+  addLowPriorityHandler<RValueHandler>();
   // TODO: split trackExpressionValue and FindLastStoreBRVisitor into handlers
   //       and add them here.
 }
 
 Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
                                TrackingOptions Opts) {
   if (!E || !N)
     return {};