[analyzer][NFC] Pass the diagnostic message to the TrackConstraintBRVisitor

[donat.nagy]

Summary

The TrackConstraintBRVisitor should accept a message for the note instead of creating one.
It would let us inject domain-specific knowledge in a non-intrusive way, leading to a more generic visitor.

Meta: This small NFC commit was added to our internal fork by @gamesh411 (although if I understand it correctly @steakhal was also involved in its creation). I'd like to upstream it because I'm planning to release one of our internal checkers (which reports invalid use of bitwise shifts) and it depends on this commit.

[steakhal]

LGTM

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

void trackStoredValue(KnownSVal V, const MemRegion *R,
                       PathSensitiveBugReport &Report, TrackingOptions Opts = {},
                       const StackFrameContext *Origin = nullptr);
 
 const Expr *getDerefExpr(const Stmt *S);
 
 } // namespace bugreporter
 
 class TrackConstraintBRVisitor final : public BugReporterVisitor {
-  DefinedSVal Constraint;
-  bool Assumption;
+  const SmallString<64> Message;
+  const DefinedSVal Constraint;
+  const bool Assumption;
   bool IsSatisfied = false;
-  bool IsZeroCheck;
 
   /// We should start tracking from the last node along the path in which the
   /// value is constrained.
   bool IsTrackingTurnedOn = false;
 
 public:
-  TrackConstraintBRVisitor(DefinedSVal constraint, bool assumption)
-      : Constraint(constraint), Assumption(assumption),
-        IsZeroCheck(!Assumption && isa<Loc>(Constraint)) {}
+  TrackConstraintBRVisitor(DefinedSVal constraint, bool assumption,
+                           StringRef Message)
+      : Message(Message), Constraint(constraint), Assumption(assumption) {}
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   /// Return the tag associated with this visitor.  This tag will be used
   /// to make all PathDiagnosticPieces created by this visitor.
   static const char *getTag();
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BRC,
                                    PathSensitiveBugReport &BR) override;
 
 private:
+  /// Checks if the constraint refers to a null-location.
+  bool isZeroCheck() const;
+
   /// Checks if the constraint is valid in the current state.
   bool isUnderconstrained(const ExplodedNode *N) const;
 };
 
 /// \class NilReceiverBRVisitor
 /// Prints path notes when a message is sent to a nil receiver.
 class NilReceiverBRVisitor final : public BugReporterVisitor {
 public:

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 
 //===----------------------------------------------------------------------===//
 // Implementation of TrackConstraintBRVisitor.
 //===----------------------------------------------------------------------===//
 
 void TrackConstraintBRVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
   static int tag = 0;
   ID.AddPointer(&tag);
+  ID.AddString(Message);
   ID.AddBoolean(Assumption);
   ID.Add(Constraint);
 }
 
 /// Return the tag associated with this visitor.  This tag will be used
 /// to make all PathDiagnosticPieces created by this visitor.
 const char *TrackConstraintBRVisitor::getTag() {
   return "TrackConstraintBRVisitor";
 }
 
+bool TrackConstraintBRVisitor::isZeroCheck() const {
+  return !Assumption && Constraint.getAs<Loc>();
+}
+
 bool TrackConstraintBRVisitor::isUnderconstrained(const ExplodedNode *N) const {
-  if (IsZeroCheck)
+  if (isZeroCheck())
     return N->getState()->isNull(Constraint).isUnderconstrained();
   return (bool)N->getState()->assume(Constraint, !Assumption);
 }
 
 PathDiagnosticPieceRef TrackConstraintBRVisitor::VisitNode(
     const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &) {
   const ExplodedNode *PrevN = N->getFirstPred();
   if (IsSatisfied)
if (isUnderconstrained(PrevN)) {
     IsSatisfied = true;
 
     // At this point, the negation of the constraint should be infeasible. If it
     // is feasible, make sure that the negation of the constrainti was
     // infeasible in the current state.  If it is feasible, we somehow missed
     // the transition point.
     assert(!isUnderconstrained(N));
 
-    // We found the transition point for the constraint.  We now need to
-    // pretty-print the constraint. (work-in-progress)
-    SmallString<64> sbuf;
-    llvm::raw_svector_ostream os(sbuf);
-
-    if (isa<Loc>(Constraint)) {
-      os << "Assuming pointer value is ";
-      os << (Assumption ? "non-null" : "null");
-    }
-
-    if (os.str().empty())
-      return nullptr;
-
     // Construct a new PathDiagnosticPiece.
     ProgramPoint P = N->getLocation();
 
     // If this node already have a specialized note, it's probably better
     // than our generic note.
     // FIXME: This only looks for note tags, not for other ways to add a note.
     if (isa_and_nonnull<NoteTag>(P.getTag()))
       return nullptr;
 
     PathDiagnosticLocation L =
       PathDiagnosticLocation::create(P, BRC.getSourceManager());
     if (!L.isValid())
       return nullptr;
 
-    auto X = std::make_shared<PathDiagnosticEventPiece>(L, os.str());
+    auto X = std::make_shared<PathDiagnosticEventPiece>(L, Message);
     X->setTag(getTag());
     return std::move(X);
   }
 
   return nullptr;
 }
 
 //===----------------------------------------------------------------------===//
if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
 
         Report.markInteresting(V, Opts.Kind);
         Report.addVisitor<UndefOrNullArgVisitor>(R);
 
         // If the contents are symbolic and null, find out when they became
         // null.
         if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true))
           if (LVState->isNull(V).isConstrainedTrue())
-            Report.addVisitor<TrackConstraintBRVisitor>(V.castAs<DefinedSVal>(),
-                                                        false);
+            Report.addVisitor<TrackConstraintBRVisitor>(
+                V.castAs<DefinedSVal>(),
+                /*Assumption=*/false, "Assuming pointer value is null");
 
         // Add visitor, which will suppress inline defensive checks.
         if (auto DV = V.getAs<DefinedSVal>())
           if (!DV->isZeroConstant() && Opts.EnableNullFPSuppression)
             // Note that LVNode may be too late (i.e., too far from the
             // InputNode) because the lvalue may have been computed before the
             // inlined call was evaluated. InputNode may as well be too early
             // here, because the symbol is already dead; this, however, is fine
if (auto L = V.getAs<loc::MemRegionVal>()) {
               getParentTracker().track(*KV, L->getRegion(), Opts, SFC));
       }
 
       const MemRegion *RegionRVal = RVal.getAsRegion();
       if (isa_and_nonnull<SymbolicRegion>(RegionRVal)) {
         Report.markInteresting(RegionRVal, Opts.Kind);
         Report.addVisitor<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal),
-            /*assumption=*/false);
+            /*Assumption=*/false, "Assuming pointer value is null");
         Result.FoundSomethingToTrack = true;
       }
     }
 
     return Result;
   }
 };