[clang][Analyzer] Add symbol uninterestingness to bug report.

[balazske]

Summary

PathSensitiveBughReport has a function to mark a symbol as interesting but
it was not possible to clear this flag. This can be useful in some cases,
so the functionality is added.

[balazske]

Could not make a simple test for the change. This file

BugReportInterestingnessTest.cpp6 KBDownload

is what I could do, but it prints the needed text to the console. Is it possible to get the note texts in the program?

[balazske]

The original change D104300 contained these modifications but is currently not committed. The "uninterestingness" is really a separate patch that belongs to another change. For me it is not a problem is D104300 is committed, otherwise I would need this to continue with D104925.

[NoQ]

In D105637#2864684, @balazske wrote:

Could not make a simple test for the change. This file

BugReportInterestingnessTest.cpp6 KBDownload

is what I could do, but it prints the needed text to the console. Is it possible to get the note texts in the program?

You could define your own diagnostic consumer in the unittest and intercept all the notes. But at this point i'd rather turn this into a LIT test by turning your checker mock into a debug.ExprInspection item and using -analyzer-output=text to test notes:

void foo(x) {
  clang_analyzer_noteIfInteresting(x); // no-note
  clang_analyzer_markNotInteresting(x);
  clang_analyzer_noteIfInteresting(x); // expected-note{{INTERESTING}}
  clang_analyzer_markInteresting(x);
  clang_analyzer_warnIfReached(); // expected-warning{{TRUE}}
}                                 // expected-note@-1{{TRUE}}

[NoQ]

In D105637#2867602, @NoQ wrote:

You could define your own diagnostic consumer in the unittest and intercept all the notes.

(you can find an example of this in D94476) (I can't believe I haven't landed these patches yet)

In D105637#2867602, @NoQ wrote:

But at this point i'd rather turn this into a LIT test by turning your checker mock into a debug.ExprInspection item

(dunno how reusable it'll be, maybe unittest is indeed the way to go)

[balazske]

Added the unit test.

[balazske]

Removed "garbage" from end of test file.

[NoQ]

Looks great, thanks for reusing the reusables!

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2262–2263

You're saying, e.g., "If string length is not interesting then neither is the string itself". Or, dunno, "If the raw pointer value is not interesting then neither is a smart pointer that was holding it".

I'm not sure about that. I'm pretty sure that no checkers are currently affected by this code but I still don't understand your point.

I don't understand the original code in markInteresting() either; do we have even one test to cover it?

Also note that what you've written is not a correct negation of the original code. The correct negation (that would keep the region-metadata relationship in sync as originally intended) would be "if the region loses interestingness, so does the metadata". Or it has to go both ways. I'm still not sure if any of this matters though.

Maybe we should eliminate these extra clauses entirely. If you're not willing to investigate whether this is all dead code or it actually does something, I'd like to suggest a "FIXME: Is this necessary?" (or something like that) both here and in the original code.

clang/unittests/StaticAnalyzer/Reusables.h

132

We already have VerifyDiagnosticConsumer. Maybe VerifyPathDiagnosticConsumer to put emphasis on path-sensitivity (which implies usefulness in unittests because VerifyDiagnosticConsumer operates when all diagnostics are already indistinguishably flattened) and on its similarity to VerifyDiagnosticConsumer?

[balazske]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2262–2263

The metadata interestingness was added in rG735724fb1e78 long ago, there is not more information. All test pass if the code is removed (but it may have impacts on bug paths). Is it not the job of the checker to mark the interestingness of objects? I assume that the meaning of meta->getRegion() is checker dependent (the string or smart pointer or something totally different?) and the checker should know what to make interesting (or not). The markInteresting() has no test (what I could extend here), it may be implicitly tested by the other checker tests.

2262–2263

The mark of associated region as interesting may not work correct if there are multiple metadata symbols for the same region, which should be a possible case. Specially the remove of interestingness is not correct in this way. So I would remove the if (const auto *meta = dyn_cast<SymbolMetadata>(sym)) part.
A better approach can be to make the interestingness depend totally on the associated region for metadata symbol. If a symbol is metadata, its interestingness is that of the region. And only the region is inserted into the map. isInteresting can be updated for this purpose.

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2262–2263

I assume that the meaning of meta->getRegion() is checker dependent

I completely agree with your reasoning. One way or another, that old commit didn't come with a test, or any explanation whatsoever. I think we have all the rights to question it.

[balazske]

Rename of DiagnosticVerifyConsumer.
Changes related to handling of metadata symbols.

[NoQ]

Amazing, thanks!

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

public:
   PathDiagnosticLocation getLocation() const override;
 
   /// 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);
 
+  void markNotInteresting(SymbolRef sym);
+
   /// 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);
 
+  void markNotInteresting(const MemRegion *R);
+
   /// 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;

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 
 void PathSensitiveBugReport::markInteresting(SymbolRef sym,
                                              bugreporter::TrackingKind TKind) {
   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);
 }
 
+void PathSensitiveBugReport::markNotInteresting(SymbolRef sym) {
+  if (!sym)
+    return;
+  InterestingSymbols.erase(sym);
+
+  // The metadata part of markInteresting is not reversed here.
+  // Just making the same region not interesting is incorrect
+  // in specific cases.
+}
+
 void PathSensitiveBugReport::markInteresting(const MemRegion *R,
                                              bugreporter::TrackingKind TKind) {
   if (!R)
     return;
 
   R = R->getBaseRegion();
   insertToInterestingnessMap(InterestingRegions, R, TKind);
 
   if (const auto *SR = dyn_cast<SymbolicRegion>(R))
     markInteresting(SR->getSymbol(), TKind);
 }
 
+void PathSensitiveBugReport::markNotInteresting(const MemRegion *R) {
+  if (!R)
+    return;
+
+  R = R->getBaseRegion();
+  InterestingRegions.erase(R);
+
+  if (const auto *SR = dyn_cast<SymbolicRegion>(R))
+    markNotInteresting(SR->getSymbol());
+}
+
 void PathSensitiveBugReport::markInteresting(SVal V,
                                              bugreporter::TrackingKind TKind) {
   markInteresting(V.getAsRegion(), TKind);
   markInteresting(V.getAsSymbol(), TKind);
 }
 
 void PathSensitiveBugReport::markInteresting(const LocationContext *LC) {
   if (!LC)

clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Reusables.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ento;
+using namespace llvm;
+
+namespace {
+
+class InterestingnessTestChecker : public Checker<check::PreCall> {
+  BugType BT_TestBug;
+
+  using HandlerFn = std::function<void(const InterestingnessTestChecker *,
+                                       const CallEvent &, CheckerContext &)>;
+
+  CallDescriptionMap<HandlerFn> Handlers = {
+      {{"setInteresting", 1}, &InterestingnessTestChecker::handleInteresting},
+      {{"setNotInteresting", 1},
+       &InterestingnessTestChecker::handleNotInteresting},
+      {{"check", 1}, &InterestingnessTestChecker::handleCheck},
+      {{"bug", 1}, &InterestingnessTestChecker::handleBug},
+  };
+
+  void handleInteresting(const CallEvent &Call, CheckerContext &C) const;
+  void handleNotInteresting(const CallEvent &Call, CheckerContext &C) const;
+  void handleCheck(const CallEvent &Call, CheckerContext &C) const;
+  void handleBug(const CallEvent &Call, CheckerContext &C) const;
+
+public:
+  InterestingnessTestChecker()
+      : BT_TestBug(this, "InterestingnessTestBug", "Test") {}
+
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+    const HandlerFn *Handler = Handlers.lookup(Call);
+    if (!Handler)
+      return;
+
+    (*Handler)(this, Call, C);
+  }
+};
+
+} // namespace
+
+void InterestingnessTestChecker::handleInteresting(const CallEvent &Call,
+                                                   CheckerContext &C) const {
+  SymbolRef Sym = Call.getArgSVal(0).getAsSymbol();
+  assert(Sym);
+  C.addTransition(nullptr, C.getNoteTag([Sym](PathSensitiveBugReport &BR) {
+    BR.markInteresting(Sym);
+    return "";
+  }));
+}
+
+void InterestingnessTestChecker::handleNotInteresting(const CallEvent &Call,
+                                                      CheckerContext &C) const {
+  SymbolRef Sym = Call.getArgSVal(0).getAsSymbol();
+  assert(Sym);
+  C.addTransition(nullptr, C.getNoteTag([Sym](PathSensitiveBugReport &BR) {
+    BR.markNotInteresting(Sym);
+    return "";
+  }));
+}
+
+void InterestingnessTestChecker::handleCheck(const CallEvent &Call,
+                                             CheckerContext &C) const {
+  SymbolRef Sym = Call.getArgSVal(0).getAsSymbol();
+  assert(Sym);
+  C.addTransition(nullptr, C.getNoteTag([Sym](PathSensitiveBugReport &BR) {
+    if (BR.isInteresting(Sym))
+      return "Interesting";
+    else
+      return "NotInteresting";
+  }));
+}
+
+void InterestingnessTestChecker::handleBug(const CallEvent &Call,
+                                           CheckerContext &C) const {
+  ExplodedNode *N = C.generateErrorNode();
+  C.emitReport(
+      std::make_unique<PathSensitiveBugReport>(BT_TestBug, "test bug", N));
+}
+
+namespace {
+
+class TestAction : public ASTFrontendAction {
+  ExpectedDiagsTy ExpectedDiags;
+
+public:
+  TestAction(ExpectedDiagsTy &&ExpectedDiags)
+      : ExpectedDiags(std::move(ExpectedDiags)) {}
+
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
+                                                 StringRef File) override {
+    std::unique_ptr<AnalysisASTConsumer> AnalysisConsumer =
+        CreateAnalysisConsumer(Compiler);
+    AnalysisConsumer->AddDiagnosticConsumer(new VerifyPathDiagnosticConsumer(
+        std::move(ExpectedDiags), Compiler.getSourceManager()));
+    AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+      Registry.addChecker<InterestingnessTestChecker>("test.Interestingness",
+                                                      "Description", "");
+    });
+    Compiler.getAnalyzerOpts()->CheckersAndPackages = {
+        {"test.Interestingness", true}};
+    return std::move(AnalysisConsumer);
+  }
+};
+
+} // namespace
+
+TEST(BugReportInterestingness, Symbols) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::make_unique<TestAction>(ExpectedDiagsTy{
+          {{15, 7},
+           "test bug",
+           "test bug",
+           "test.Interestingness",
+           "InterestingnessTestBug",
+           "Test",
+           {
+               {{8, 7}, "Interesting", {{{8, 7}, {8, 14}}}},
+               {{10, 7}, "NotInteresting", {{{10, 7}, {10, 14}}}},
+               {{12, 7}, "Interesting", {{{12, 7}, {12, 14}}}},
+               {{14, 7}, "NotInteresting", {{{14, 7}, {14, 14}}}},
+               {{15, 7}, "test bug", {{{15, 7}, {15, 12}}}},
+           }}}),
+      R"(
+    void setInteresting(int);
+    void setNotInteresting(int);
+    void check(int);
+    void bug(int);
+
+    void f(int A) {
+      check(A);
+      setInteresting(A);
+      check(A);
+      setNotInteresting(A);
+      check(A);
+      setInteresting(A);
+      check(A);
+      bug(A);
+    }
+  )",
+      "input.cpp"));
+}

clang/unittests/StaticAnalyzer/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
+  BugReportInterestingnessTest.cpp
   CallDescriptionTest.cpp
   CallEventTest.cpp
   FalsePositiveRefutationBRVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
   RegisterCustomCheckersTest.cpp
   StoreTest.cpp
   SymbolReaperTest.cpp

clang/unittests/StaticAnalyzer/Reusables.h

 //===- unittests/StaticAnalyzer/Reusables.h -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_UNITTESTS_STATICANALYZER_REUSABLES_H
 #define LLVM_CLANG_UNITTESTS_STATICANALYZER_REUSABLES_H
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Frontend/CompilerInstance.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "gtest/gtest.h"
 
 namespace clang {
 namespace ento {
 
 // Find a node in the current AST that matches a matcher.
 template <typename T, typename MatcherT>
 const T *findNode(const Decl *Where, MatcherT What) {
   using namespace ast_matchers;
ExprEngineConsumer(CompilerInstance &C)
         CTU(C), Consumers(),
         AMgr(C.getASTContext(), C.getPreprocessor(), Consumers,
              CreateRegionStoreManager, CreateRangeConstraintManager, &ChkMgr,
              *C.getAnalyzerOpts()),
         VisitedCallees(), FS(),
         Eng(CTU, AMgr, &VisitedCallees, &FS, ExprEngine::Inline_Regular) {}
 };
 
+struct ExpectedLocationTy {
+  unsigned Line;
+  unsigned Column;
+
+  void testEquality(SourceLocation L, SourceManager &SM) const {
+    EXPECT_EQ(SM.getSpellingLineNumber(L), Line);
+    EXPECT_EQ(SM.getSpellingColumnNumber(L), Column);
+  }
+};
+
+struct ExpectedRangeTy {
+  ExpectedLocationTy Begin;
+  ExpectedLocationTy End;
+
+  void testEquality(SourceRange R, SourceManager &SM) const {
+    Begin.testEquality(R.getBegin(), SM);
+    End.testEquality(R.getEnd(), SM);
+  }
+};
+
+struct ExpectedPieceTy {
+  ExpectedLocationTy Loc;
+  std::string Text;
+  std::vector<ExpectedRangeTy> Ranges;
+
+  void testEquality(const PathDiagnosticPiece &Piece, SourceManager &SM) {
+    Loc.testEquality(Piece.getLocation().asLocation(), SM);
+    EXPECT_EQ(Piece.getString(), Text);
+    EXPECT_EQ(Ranges.size(), Piece.getRanges().size());
+    for (const auto &RangeItem : llvm::enumerate(Piece.getRanges()))
+      Ranges[RangeItem.index()].testEquality(RangeItem.value(), SM);
+  }
+};
+
+struct ExpectedDiagTy {
+  ExpectedLocationTy Loc;
+  std::string VerboseDescription;
+  std::string ShortDescription;
+  std::string CheckerName;
+  std::string BugType;
+  std::string Category;
+  std::vector<ExpectedPieceTy> Path;
+
+  void testEquality(const PathDiagnostic &Diag, SourceManager &SM) {
+    Loc.testEquality(Diag.getLocation().asLocation(), SM);
+    EXPECT_EQ(Diag.getVerboseDescription(), VerboseDescription);
+    EXPECT_EQ(Diag.getShortDescription(), ShortDescription);
+    EXPECT_EQ(Diag.getCheckerName(), CheckerName);
+    EXPECT_EQ(Diag.getBugType(), BugType);
+    EXPECT_EQ(Diag.getCategory(), Category);
+
+    EXPECT_EQ(Path.size(), Diag.path.size());
+    for (const auto &PieceItem : llvm::enumerate(Diag.path)) {
+      if (PieceItem.index() < Path.size())
+        Path[PieceItem.index()].testEquality(*PieceItem.value(), SM);
+    }
+  }
+};
+
+using ExpectedDiagsTy = std::vector<ExpectedDiagTy>;
+
+// A consumer to verify the generated diagnostics.
+class VerifyPathDiagnosticConsumer : public PathDiagnosticConsumer {
+  ExpectedDiagsTy ExpectedDiags;
+  SourceManager &SM;
+
+public:
+  VerifyPathDiagnosticConsumer(ExpectedDiagsTy &&ExpectedDiags,
+                               SourceManager &SM)
+      : ExpectedDiags(ExpectedDiags), SM(SM) {}
+
+  StringRef getName() const override { return "verify test diagnostics"; }
+
+  void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
+                            FilesMade *filesMade) override {
+    EXPECT_EQ(Diags.size(), ExpectedDiags.size());
+    for (const auto &Item : llvm::enumerate(Diags))
+      if (Item.index() < ExpectedDiags.size())
+        ExpectedDiags[Item.index()].testEquality(*Item.value(), SM);
+  }
+};
+
 } // namespace ento
 } // namespace clang
 
 #endif