diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -432,6 +432,8 @@ 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). @@ -439,6 +441,8 @@ 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). diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2249,10 +2249,22 @@ 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(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) @@ -2265,6 +2277,17 @@ 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(R)) + markNotInteresting(SR->getSymbol()); +} + void PathSensitiveBugReport::markInteresting(SVal V, bugreporter::TrackingKind TKind) { markInteresting(V.getAsRegion(), TKind); diff --git a/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp b/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp new file mode 100644 --- /dev/null +++ b/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp @@ -0,0 +1,162 @@ +//===----------------------------------------------------------------------===// +// +// 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 { + BugType BT_TestBug; + + using HandlerFn = std::function; + + CallDescriptionMap 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(BT_TestBug, "test bug", N)); +} + +namespace { + +class TestAction : public ASTFrontendAction { + ExpectedDiagsTy ExpectedDiags; + +public: + TestAction(ExpectedDiagsTy &&ExpectedDiags) + : ExpectedDiags(std::move(ExpectedDiags)) {} + + std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler, + StringRef File) override { + std::unique_ptr AnalysisConsumer = + CreateAnalysisConsumer(Compiler); + AnalysisConsumer->AddDiagnosticConsumer(new VerifyPathDiagnosticConsumer( + std::move(ExpectedDiags), Compiler.getSourceManager())); + AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) { + Registry.addChecker("test.Interestingness", + "Description", ""); + }); + Compiler.getAnalyzerOpts()->CheckersAndPackages = { + {"test.Interestingness", true}}; + return std::move(AnalysisConsumer); + } +}; + +} // namespace + +TEST(BugReportInterestingness, Symbols) { + EXPECT_TRUE(tooling::runToolOnCode( + std::make_unique(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")); +} diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt --- a/clang/unittests/StaticAnalyzer/CMakeLists.txt +++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt @@ -5,6 +5,7 @@ add_clang_unittest(StaticAnalysisTests AnalyzerOptionsTest.cpp + BugReportInterestingnessTest.cpp CallDescriptionTest.cpp CallEventTest.cpp FalsePositiveRefutationBRVisitorTest.cpp diff --git a/clang/unittests/StaticAnalyzer/Reusables.h b/clang/unittests/StaticAnalyzer/Reusables.h --- a/clang/unittests/StaticAnalyzer/Reusables.h +++ b/clang/unittests/StaticAnalyzer/Reusables.h @@ -10,9 +10,10 @@ #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 { @@ -66,6 +67,88 @@ 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 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 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; + +// 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 &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