[analyzer][NFC] Add different interestingness kinds

We defined (on the mailing list and here on phabricator) 2 different cases where
retrieving information about a control dependency condition is very important:

* When the condition's last write happened in a different stack frame
* When the collapse point of the condition (when we can constrain it to be
true/false) didn't happen in the actual condition.

It seems like we solved this problem with the help of expression value tracking,
and have started working on better diagnostics notes about this process.

Expression value tracking is nothing more than registering a variety of visitors
to construct reports about it. Each of the registered visitors (ReturnVisitor,
FindLastStoreVisitor, NoStoreFuncVisitor, etc) have something to go by: a
MemRegion, an SVal, an ExplodedNode, etc. For this reason, better explaining a
last write is super simple, we can always just pass on some more information to
the visitor in question (as seen in D65575).

ConditionBRVisitor is a different beast, as it was built for a different
purpose. It is responsible for constructing events at, well, conditions, and is
registered only once, and isn't a part of the "expression value tracking
family". Unfortunately, it is also the visitor to tinker with for constructing
better diagnostics about the collapse point problem.

This creates a need for alternative way to communicate with ConditionBRVisitor
that a specific condition is being tracked for for the reason of being a control
dependency. Since at almost all PathDiagnosticEventPiece construction the
visitor checks interestingness, it makes sense to pair interestingness with a
reason as to why we marked an entity as such.

Differential Revision: https://reviews.llvm.org/D65723

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@369583 91177308-0d34-0410-b5e6-96231b3b80d8

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

@@ -102,14 +102,15 @@ protected:
   /// diagnostics to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
-  llvm::DenseSet<SymbolRef> InterestingSymbols;
+  llvm::DenseMap<SymbolRef, bugreporter::TrackingKind> InterestingSymbols;
 
   /// A (stack of) set of regions that are registered with this report as being
   /// "interesting", and thus used to help decide which diagnostics
   /// to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
-  llvm::DenseSet<const MemRegion *> InterestingRegions;
+  llvm::DenseMap<const MemRegion *, bugreporter::TrackingKind>
+      InterestingRegions;
 
   /// A set of location contexts that correspoind to call sites which should be
   /// considered "interesting".
@@ -209,9 +210,24 @@ public:
   /// Disable all path pruning when generating a PathDiagnostic.
   void disablePathPruning() { DoNotPrunePath = true; }
 
-  void markInteresting(SymbolRef sym);
-  void markInteresting(const MemRegion *R);
-  void markInteresting(SVal V);
+  /// Marks a symbol as interesting. Different kinds of interestingness will
+  /// be processed differently by visitors (e.g. if the tracking kind is
+  /// condition, will append "will be used as a condition" to the message).
+  void markInteresting(SymbolRef sym, bugreporter::TrackingKind TKind =
+                                          bugreporter::TrackingKind::Thorough);
+
+  /// Marks a region as interesting. Different kinds of interestingness will
+  /// be processed differently by visitors (e.g. if the tracking kind is
+  /// condition, will append "will be used as a condition" to the message).
+  void markInteresting(
+      const MemRegion *R,
+      bugreporter::TrackingKind TKind = bugreporter::TrackingKind::Thorough);
+
+  /// Marks a symbolic value as interesting. Different kinds of interestingness
+  /// will be processed differently by visitors (e.g. if the tracking kind is
+  /// condition, will append "will be used as a condition" to the message).
+  void markInteresting(SVal V, bugreporter::TrackingKind TKind =
+                                   bugreporter::TrackingKind::Thorough);
   void markInteresting(const LocationContext *LC);
 
   bool isInteresting(SymbolRef sym) const;
@@ -219,6 +235,14 @@ public:
   bool isInteresting(SVal V) const;
   bool isInteresting(const LocationContext *LC) const;
 
+  Optional<bugreporter::TrackingKind>
+  getInterestingnessKind(SymbolRef sym) const;
+
+  Optional<bugreporter::TrackingKind>
+  getInterestingnessKind(const MemRegion *R) const;
+
+  Optional<bugreporter::TrackingKind> getInterestingnessKind(SVal V) const;
+
   /// Returns whether or not this report should be considered valid.
   ///
   /// Invalid reports are those that have been classified as likely false

lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -2101,30 +2101,61 @@ void BugReport::Profile(llvm::FoldingSetNodeID& hash) const {
   }
 }
 
-void BugReport::markInteresting(SymbolRef sym) {
+template <class T>
+static void insertToInterestingnessMap(
+    llvm::DenseMap<T, bugreporter::TrackingKind> &InterestingnessMap, T Val,
+    bugreporter::TrackingKind TKind) {
+  auto Result = InterestingnessMap.insert({Val, TKind});
+
+  if (Result.second)
+    return;
+
+  // Even if this symbol/region was already marked as interesting as a
+  // condition, if we later mark it as interesting again but with
+  // thorough tracking, overwrite it. Entities marked with thorough
+  // interestiness are the most important (or most interesting, if you will),
+  // and we wouldn't like to downplay their importance.
+
+  switch (TKind) {
+    case bugreporter::TrackingKind::Thorough:
+      Result.first->getSecond() = bugreporter::TrackingKind::Thorough;
+      return;
+    case bugreporter::TrackingKind::Condition:
+      return;
+  }
+
+  llvm_unreachable(
+      "BugReport::markInteresting currently can only handle 2 different "
+      "tracking kinds! Please define what tracking kind should this entitiy"
+      "have, if it was already marked as interesting with a different kind!");
+}
+
+void BugReport::markInteresting(SymbolRef sym,
+                                bugreporter::TrackingKind TKind) {
   if (!sym)
     return;
 
-  InterestingSymbols.insert(sym);
+  insertToInterestingnessMap(InterestingSymbols, sym, TKind);
 
   if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
-    InterestingRegions.insert(meta->getRegion());
+    markInteresting(meta->getRegion(), TKind);
 }
 
-void BugReport::markInteresting(const MemRegion *R) {
+void BugReport::markInteresting(const MemRegion *R,
+                                bugreporter::TrackingKind TKind) {
   if (!R)
     return;
 
   R = R->getBaseRegion();
-  InterestingRegions.insert(R);
+  insertToInterestingnessMap(InterestingRegions, R, TKind);
 
   if (const auto *SR = dyn_cast<SymbolicRegion>(R))
-    InterestingSymbols.insert(SR->getSymbol());
+    markInteresting(SR->getSymbol(), TKind);
 }
 
-void BugReport::markInteresting(SVal V) {
-  markInteresting(V.getAsRegion());
-  markInteresting(V.getAsSymbol());
+void BugReport::markInteresting(SVal V, bugreporter::TrackingKind TKind) {
+  markInteresting(V.getAsRegion(), TKind);
+  markInteresting(V.getAsSymbol(), TKind);
 }
 
 void BugReport::markInteresting(const LocationContext *LC) {
@@ -2133,28 +2164,68 @@ void BugReport::markInteresting(const LocationContext *LC) {
   InterestingLocationContexts.insert(LC);
 }
 
-bool BugReport::isInteresting(SVal V)  const {
-  return isInteresting(V.getAsRegion()) || isInteresting(V.getAsSymbol());
+Optional<bugreporter::TrackingKind>
+BugReport::getInterestingnessKind(SVal V) const {
+  auto RKind = getInterestingnessKind(V.getAsRegion());
+  auto SKind = getInterestingnessKind(V.getAsSymbol());
+  if (!RKind)
+    return SKind;
+  if (!SKind)
+    return RKind;
+
+  // If either is marked with throrough tracking, return that, we wouldn't like
+  // to downplay a note's importance by 'only' mentioning it as a condition.
+  switch(*RKind) {
+    case bugreporter::TrackingKind::Thorough:
+      return RKind;
+    case bugreporter::TrackingKind::Condition:
+      return SKind;
+  }
+
+  llvm_unreachable(
+      "BugReport::getInterestingnessKind currently can only handle 2 different "
+      "tracking kinds! Please define what tracking kind should we return here "
+      "when the kind of getAsRegion() and getAsSymbol() is different!");
+  return None;
 }
 
-bool BugReport::isInteresting(SymbolRef sym)  const {
+Optional<bugreporter::TrackingKind>
+BugReport::getInterestingnessKind(SymbolRef sym) const {
   if (!sym)
-    return false;
+    return None;
   // We don't currently consider metadata symbols to be interesting
   // even if we know their region is interesting. Is that correct behavior?
-  return InterestingSymbols.count(sym);
+  auto It = InterestingSymbols.find(sym);
+  if (It == InterestingSymbols.end())
+    return None;
+  return It->getSecond();
 }
 
-bool BugReport::isInteresting(const MemRegion *R)  const {
+Optional<bugreporter::TrackingKind>
+BugReport::getInterestingnessKind(const MemRegion *R) const {
   if (!R)
-    return false;
+    return None;
+
   R = R->getBaseRegion();
-  bool b = InterestingRegions.count(R);
-  if (b)
-    return true;
+  auto It = InterestingRegions.find(R);
+  if (It != InterestingRegions.end())
+    return It->getSecond();
+
   if (const auto *SR = dyn_cast<SymbolicRegion>(R))
-    return InterestingSymbols.count(SR->getSymbol());
-  return false;
+    return getInterestingnessKind(SR->getSymbol());
+  return None;
+}
+
+bool BugReport::isInteresting(SVal V) const {
+  return getInterestingnessKind(V).hasValue();
+}
+
+bool BugReport::isInteresting(SymbolRef sym) const {
+  return getInterestingnessKind(sym).hasValue();
+}
+
+bool BugReport::isInteresting(const MemRegion *R) const {
+  return getInterestingnessKind(R).hasValue();
 }
 
 bool BugReport::isInteresting(const LocationContext *LC)  const {

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

@@ -1950,7 +1950,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
       MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
           LVNode, R, EnableNullFPSuppression, report, V);
 
-      report.markInteresting(V);
+      report.markInteresting(V, TKind);
       report.addVisitor(std::make_unique<UndefOrNullArgVisitor>(R));
 
       // If the contents are symbolic, find out when they became null.
@@ -2011,7 +2011,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
 
     const MemRegion *RegionRVal = RVal.getAsRegion();
     if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
-      report.markInteresting(RegionRVal);
+      report.markInteresting(RegionRVal, TKind);
       report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal), /*assumption=*/false));
     }