[NFC][analyzer] Transitive interestingness in BugReporter

[gamesh411]

Summary

When making a region interesting, also mark the subregions interesting.

Original author: steakhal

[steakhal]

I think since it changes how taint spreads, this patch deserves a test.
Could you please demonstrate the correctness of this patch?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2267

2361–2366

The comment of symbol_iterator suggests that for SymbolData this will be always the symbol data itself; thus it would iterate only once.
Is this the case? If so, can we replace the loop with just the isa<SymbolData>(sym) check?

/// Iterator over symbols that the current symbol depends on.
///
/// For SymbolData, it's the symbol itself; for expressions, it's the
/// expression symbol and all the operands in it. Note, SymbolDerived is
/// treated as SymbolData - the iterator will NOT visit the parent region.
class symbol_iterator {...

2363

[balazske]

Is it problem if markNotInteresting is used on a symbol that was marked interesting and has subregions?

[steakhal]

In D125362#3506545, @balazske wrote:

Is it problem if markNotInteresting is used on a symbol that was marked interesting and has subregions?

In that case, the sub-symbols will remain still interesting.
We might need to think about this and also make markNotInteresting() transitive. I'm not immediately sure about the implications, but a test demonstrating the transitivity should demonstrate this as well in a slightly modified example.
The test file for this should be clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp though.

There are only two checkers using this API: stream checker and the smart ptr checker.
We should craft a real-worl-looking example pointing out the difference between the *transitive* and *direct* markNotInteresting() implementation.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

if (!sym)
     return;
 
   insertToInterestingnessMap(InterestingSymbols, sym, TKind);
 
   // FIXME: No tests exist for this code and it is questionable:
   // How to handle multiple metadata for the same region?
   if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
     markInteresting(meta->getRegion(), TKind);
+
+  auto SubSyms = llvm::make_range(sym->symbol_begin(), sym->symbol_end());
+  for (SymbolRef SubSym : SubSyms) {
+    if (SymbolData::classof(SubSym))
+      insertToInterestingnessMap(InterestingSymbols, SubSym, TKind);
+  }
 }
 
 void PathSensitiveBugReport::markNotInteresting(SymbolRef sym) {
   if (!sym)
     return;
   InterestingSymbols.erase(sym);
 
   // The metadata part of markInteresting is not reversed here.

 }
 
 Optional<bugreporter::TrackingKind>
 PathSensitiveBugReport::getInterestingnessKind(SymbolRef sym) const {
   if (!sym)
     return None;
   // We don't currently consider metadata symbols to be interesting
   // even if we know their region is interesting. Is that correct behavior?
-  auto It = InterestingSymbols.find(sym);
+  auto TryToLookupTrackingKind =
+      [this](SymbolRef Sym) -> Optional<bugreporter::TrackingKind> {
+    auto It = InterestingSymbols.find(Sym);
     if (It == InterestingSymbols.end())
       return None;
     return It->getSecond();
+  };
+
+  if (auto MaybeTK = TryToLookupTrackingKind(sym))
+    return MaybeTK;
+
+  auto SubSyms = llvm::make_range(sym->symbol_begin(), sym->symbol_end());
+  for (SymbolRef SubSym : SubSyms) {
+    if (SymbolData::classof(SubSym)) {
+      if (auto MaybeTK = TryToLookupTrackingKind(SubSym))
+        return MaybeTK;
+    }
+  }
+  return None;
 }
 
 Optional<bugreporter::TrackingKind>
 PathSensitiveBugReport::getInterestingnessKind(const MemRegion *R) const {
   if (!R)
     return None;
 
   R = R->getBaseRegion();