diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -633,6 +633,9 @@ /// Descendants can define what a "state change is", like a change of value /// to a memory region, liveness, etc. For function calls where the state did /// not change as defined, a custom note may be constructed. +/// +/// For a minimal example, check out +/// clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp. class NoStateChangeFuncVisitor : public BugReporterVisitor { private: /// Frames modifying the state as defined in \c wasModifiedBeforeCallExit. @@ -643,6 +646,8 @@ /// many times (going up the path for each node and checking whether the /// region was written into) we instead lazily compute the stack frames /// along the path. + // TODO: Can't we just use a map instead? This is likely not as cheap as it + // makes the code difficult to read. llvm::SmallPtrSet FramesModifying; llvm::SmallPtrSet FramesModifyingCalculated; @@ -651,6 +656,8 @@ /// The calculation is cached in FramesModifying. bool isModifiedInFrame(const ExplodedNode *CallExitBeginN); + void markFrameAsModifying(const StackFrameContext *SCtx); + /// Write to \c FramesModifying all stack frames along the path in the current /// stack frame which modifies the state. void findModifyingFrames(const ExplodedNode *const CallExitBeginN); @@ -658,11 +665,37 @@ protected: bugreporter::TrackingKind TKind; - /// \return Whether the state was modified from the current node, \CurrN, to - /// the end of the stack fram, at \p CallExitBeginN. - virtual bool - wasModifiedBeforeCallExit(const ExplodedNode *CurrN, - const ExplodedNode *CallExitBeginN) = 0; + /// \return Whether the state was modified from the current node, \p CurrN, to + /// the end of the stack frame, at \p CallExitBeginN. \p CurrN and + /// \p CallExitBeginN are always in the same stack frame. + /// Clients should override this callback when a state change is important + /// not only on the entire function call, but inside of it as well. + /// Example: we may want to leave a note about the lack of locking/unlocking + /// on a particular mutex, but not if inside the function its state was + /// changed, but also restored. wasModifiedInFunction() wouldn't know of this + /// change. + virtual bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN, + const ExplodedNode *CallExitBeginN) { + return false; + } + + /// \return Whether the state was modified in the inlined function call in + /// between \p CallEnterN and \p CallExitEndN. Mind that the stack frame + /// retrieved from a CallEnterN and CallExitEndN is the *caller's* stack + /// frame! The inlined function's stack should be retrieved from either the + /// immediate successor to \p CallEnterN or immediate predecessor to + /// \p CallExitEndN. + /// Clients should override this function if a state changes local to the + /// inlined function are not interesting, only the change occuring as a + /// result of it. + /// Example: we want to leave a not about a leaked resource object not being + /// deallocated / its ownership changed inside a function, and we don't care + /// if it was assigned to a local variable (its change in ownership is + /// inconsequential). + virtual bool wasModifiedInFunction(const ExplodedNode *CallEnterN, + const ExplodedNode *CallExitEndN) { + return false; + } /// Consume the information on the non-modifying stack frame in order to /// either emit a note or not. May suppress the report entirely. @@ -702,7 +735,6 @@ }; } // namespace ento - } // namespace clang #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -52,6 +52,7 @@ #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/ParentMap.h" +#include "clang/Analysis/ProgramPoint.h" #include "clang/Basic/LLVM.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetInfo.h" @@ -76,8 +77,10 @@ #include "llvm/ADT/SetOperations.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/Support/Casting.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/raw_ostream.h" #include #include #include @@ -755,6 +758,17 @@ Owners.insert(Region); return true; } + + LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); } + LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const { + out << "Owners: {\n"; + for (const MemRegion *Owner : Owners) { + out << " "; + Owner->dumpToStream(out); + out << ",\n"; + } + out << "}\n"; + } }; protected: @@ -768,31 +782,24 @@ return Ret; } - static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) { - while (N && !N->getLocationAs()) - N = N->getFirstSucc(); - return N; + LLVM_DUMP_METHOD static std::string + getFunctionName(const ExplodedNode *CallEnterN) { + if (const CallExpr *CE = llvm::dyn_cast_or_null( + CallEnterN->getLocationAs()->getCallExpr())) + if (const FunctionDecl *FD = CE->getDirectCallee()) + return FD->getQualifiedNameAsString(); + return ""; } virtual bool - wasModifiedBeforeCallExit(const ExplodedNode *CurrN, - const ExplodedNode *CallExitN) override { - if (CurrN->getLocationAs()) - return true; - - // Its the state right *after* the call that is interesting. Any pointers - // inside the call that pointed to the allocated memory are of little - // consequence if their lifetime ends within the function. - CallExitN = getCallExitEnd(CallExitN); - if (!CallExitN) - return true; - - if (CurrN->getState()->get(Sym) != - CallExitN->getState()->get(Sym)) + wasModifiedInFunction(const ExplodedNode *CallEnterN, + const ExplodedNode *CallExitEndN) override { + if (CallEnterN->getState()->get(Sym) != + CallExitEndN->getState()->get(Sym)) return true; - OwnerSet CurrOwners = getOwnersAtNode(CurrN); - OwnerSet ExitOwners = getOwnersAtNode(CallExitN); + OwnerSet CurrOwners = getOwnersAtNode(CallEnterN); + OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN); // Owners in the current set may be purged from the analyzer later on. // If a variable is dead (is not referenced directly or indirectly after diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -355,43 +355,81 @@ return FramesModifying.count(SCtx); } +void NoStateChangeFuncVisitor::markFrameAsModifying( + const StackFrameContext *SCtx) { + while (SCtx) { + auto p = FramesModifying.insert(SCtx); + if (!p.second) + break; // Frame and all its parents already inserted. + SCtx = SCtx->getParent()->getStackFrame(); + } +} + +static const ExplodedNode *getMatchingCallExitEnd(const ExplodedNode *N) { + assert(N->getLocationAs()); + // The stackframe of the callee is only found in the nodes succeeding + // the CallEnter node. CallEnter's stack frame refers to the caller. + const StackFrameContext *OrigSCtx = N->getFirstSucc()->getStackFrame(); + + // Similarly, the nodes preceding CallExitEnd refer to the callee's stack + // frame. + auto IsMatchingCallExitEnd = [OrigSCtx](const ExplodedNode *N) { + return N->getLocationAs() && + OrigSCtx == N->getFirstPred()->getStackFrame(); + }; + while (N && !IsMatchingCallExitEnd(N)) { + assert(N->succ_size() <= 1 && + "This function is to be used on the trimmed ExplodedGraph!"); + N = N->getFirstSucc(); + } + return N; +} + void NoStateChangeFuncVisitor::findModifyingFrames( const ExplodedNode *const CallExitBeginN) { assert(CallExitBeginN->getLocationAs()); - const ExplodedNode *LastReturnN = CallExitBeginN; + const StackFrameContext *const OriginalSCtx = CallExitBeginN->getLocationContext()->getStackFrame(); - const ExplodedNode *CurrN = CallExitBeginN; - - do { - ProgramStateRef State = CurrN->getState(); - auto CallExitLoc = CurrN->getLocationAs(); - if (CallExitLoc) { - LastReturnN = CurrN; + const ExplodedNode *CurrCallExitBeginN = CallExitBeginN; + const StackFrameContext *CurrentSCtx = OriginalSCtx; + + for (const ExplodedNode *CurrN = CallExitBeginN; CurrN; + CurrN = CurrN->getFirstPred()) { + // Found a new inlined call. + if (CurrN->getLocationAs()) { + CurrCallExitBeginN = CurrN; + CurrentSCtx = CurrN->getStackFrame(); + FramesModifyingCalculated.insert(CurrentSCtx); + // We won't see a change in between two identical exploded nodes: skip. + continue; } - FramesModifyingCalculated.insert( - CurrN->getLocationContext()->getStackFrame()); - - if (wasModifiedBeforeCallExit(CurrN, LastReturnN)) { - const StackFrameContext *SCtx = CurrN->getStackFrame(); - while (!SCtx->inTopFrame()) { - auto p = FramesModifying.insert(SCtx); - if (!p.second) - break; // Frame and all its parents already inserted. - SCtx = SCtx->getParent()->getStackFrame(); + if (auto CE = CurrN->getLocationAs()) { + if (const ExplodedNode *CallExitEndN = getMatchingCallExitEnd(CurrN)) + if (wasModifiedInFunction(CurrN, CallExitEndN)) + markFrameAsModifying(CurrentSCtx); + + // We exited this inlined call, lets actualize the stack frame. + CurrentSCtx = CurrN->getStackFrame(); + + // Stop calculating at the current function, but always regard it as + // modifying, so we can avoid notes like this: + // void f(Foo &F) { + // F.field = 0; // note: 0 assigned to 'F.field' + // // note: returning without writing to 'F.field' + // } + if (CE->getCalleeContext() == OriginalSCtx) { + markFrameAsModifying(CurrentSCtx); + break; } } - // Stop calculation at the call to the current function. - if (auto CE = CurrN->getLocationAs()) - if (CE->getCalleeContext() == OriginalSCtx) - break; - - CurrN = CurrN->getFirstPred(); - } while (CurrN); + if (wasModifiedBeforeCallExit(CurrN, CurrCallExitBeginN)) + markFrameAsModifying(CurrentSCtx); + } } PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode( 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 @@ -9,6 +9,7 @@ CallDescriptionTest.cpp CallEventTest.cpp FalsePositiveRefutationBRVisitorTest.cpp + NoStateChangeFuncVisitorTest.cpp ParamRegionTest.cpp RangeSetTest.cpp RegisterCustomCheckersTest.cpp diff --git a/clang/unittests/StaticAnalyzer/CallEventTest.cpp b/clang/unittests/StaticAnalyzer/CallEventTest.cpp --- a/clang/unittests/StaticAnalyzer/CallEventTest.cpp +++ b/clang/unittests/StaticAnalyzer/CallEventTest.cpp @@ -81,7 +81,7 @@ } )", Diags)); - EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n"); + EXPECT_EQ(Diags, "test.CXXDeallocator: NumArgs: 1\n"); } } // namespace diff --git a/clang/unittests/StaticAnalyzer/CheckerRegistration.h b/clang/unittests/StaticAnalyzer/CheckerRegistration.h --- a/clang/unittests/StaticAnalyzer/CheckerRegistration.h +++ b/clang/unittests/StaticAnalyzer/CheckerRegistration.h @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "clang/Analysis/PathDiagnostic.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" @@ -26,8 +27,23 @@ DiagConsumer(llvm::raw_ostream &Output) : Output(Output) {} void FlushDiagnosticsImpl(std::vector &Diags, FilesMade *filesMade) override { - for (const auto *PD : Diags) - Output << PD->getCheckerName() << ":" << PD->getShortDescription() << '\n'; + for (const auto *PD : Diags) { + Output << PD->getCheckerName() << ": "; + + for (PathDiagnosticPieceRef Piece : + PD->path.flatten(/*ShouldFlattenMacros*/ true)) { + if (Piece->getKind() != PathDiagnosticPiece::Event) + continue; + if (Piece->getString().empty()) + continue; + // The last event is usually the same as the warning message, skip. + if (Piece->getString() == PD->getShortDescription()) + continue; + + Output << Piece->getString() << " | "; + } + Output << PD->getShortDescription() << '\n'; + } } StringRef getName() const override { return "Test"; } diff --git a/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp b/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp --- a/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp +++ b/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp @@ -128,7 +128,7 @@ EXPECT_TRUE(runCheckerOnCodeWithArgs( Code, LazyAssumeAndCrossCheckArgs, Diags)); EXPECT_EQ(Diags, - "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"); + "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"); // Single warning. The second report was invalidated by the visitor. // Without enabling the crosscheck-with-z3 both reports are displayed. @@ -136,8 +136,8 @@ EXPECT_TRUE(runCheckerOnCodeWithArgs( Code, LazyAssumeArgs, Diags2)); EXPECT_EQ(Diags2, - "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n" - "test.FalsePositiveGenerator:REACHED_WITH_CONTRADICTION\n"); + "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n" + "test.FalsePositiveGenerator: REACHED_WITH_CONTRADICTION\n"); } TEST_F(FalsePositiveRefutationBRVisitorTestBase, @@ -159,7 +159,7 @@ EXPECT_TRUE(runCheckerOnCodeWithArgs( Code, LazyAssumeAndCrossCheckArgs, Diags)); EXPECT_EQ(Diags, - "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"); + "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"); // Single warning. The second report was invalidated by the visitor. // Without enabling the crosscheck-with-z3 both reports are displayed. @@ -167,8 +167,8 @@ EXPECT_TRUE(runCheckerOnCodeWithArgs( Code, LazyAssumeArgs, Diags2)); EXPECT_EQ(Diags2, - "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n" - "test.FalsePositiveGenerator:CAN_BE_TRUE\n"); + "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n" + "test.FalsePositiveGenerator: CAN_BE_TRUE\n"); } TEST_F(FalsePositiveRefutationBRVisitorTestBase, @@ -206,7 +206,7 @@ EXPECT_TRUE(runCheckerOnCodeWithArgs( Code, LazyAssumeAndCrossCheckArgs, Diags)); EXPECT_EQ(Diags, - "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"); + "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"); // Single warning. The second report was invalidated by the visitor. // Without enabling the crosscheck-with-z3 both reports are displayed. @@ -214,8 +214,8 @@ EXPECT_TRUE(runCheckerOnCodeWithArgs( Code, LazyAssumeArgs, Diags2)); EXPECT_EQ(Diags2, - "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n" - "test.FalsePositiveGenerator:CAN_BE_TRUE\n"); + "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n" + "test.FalsePositiveGenerator: CAN_BE_TRUE\n"); } } // namespace diff --git a/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp b/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp new file mode 100644 --- /dev/null +++ b/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp @@ -0,0 +1,302 @@ +//===- unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.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 "CheckerRegistration.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.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/Core/PathSensitive/ExplodedGraph.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" +#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h" +#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h" +#include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/raw_ostream.h" +#include "gtest/gtest.h" +#include + +//===----------------------------------------------------------------------===// +// Base classes for testing NoStateChangeFuncVisitor. +// +// Testing is done by observing a very simple trait change from one node to +// another -- the checker sets the ErrorPrevented trait to true if +// 'preventError()' is called in the source code, and sets it to false if +// 'allowError()' is called. If this trait is false when 'error()' is called, +// a warning is emitted. +// +// The checker then registers a simple NoStateChangeFuncVisitor to add notes to +// inlined functions that could have, but neglected to prevent the error. +//===----------------------------------------------------------------------===// + +REGISTER_TRAIT_WITH_PROGRAMSTATE(ErrorPrevented, bool) + +namespace clang { +namespace ento { +namespace { + +class ErrorNotPreventedFuncVisitor : public NoStateChangeFuncVisitor { +public: + ErrorNotPreventedFuncVisitor() + : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough) {} + + virtual PathDiagnosticPieceRef + maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, + const ObjCMethodCall &Call, + const ExplodedNode *N) override { + return nullptr; + } + + virtual PathDiagnosticPieceRef + maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, + const CXXConstructorCall &Call, + const ExplodedNode *N) override { + return nullptr; + } + + virtual PathDiagnosticPieceRef + maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call, + const ExplodedNode *N) override { + PathDiagnosticLocation L = PathDiagnosticLocation::create( + N->getLocation(), + N->getState()->getStateManager().getContext().getSourceManager()); + return std::make_shared( + L, "Returning without prevening the error"); + } + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int Tag = 0; + ID.AddPointer(&Tag); + } +}; + +template +class StatefulChecker : public Checker { + mutable std::unique_ptr BT; + +public: + void checkPreCall(const CallEvent &Call, CheckerContext &C) const { + if (Call.isCalled(CallDescription{"preventError", 0})) { + C.addTransition(C.getState()->set(true)); + return; + } + + if (Call.isCalled(CallDescription{"allowError", 0})) { + C.addTransition(C.getState()->set(false)); + return; + } + + if (Call.isCalled(CallDescription{"error", 0})) { + if (C.getState()->get()) + return; + const ExplodedNode *N = C.generateErrorNode(); + if (!N) + return; + if (!BT) + BT.reset(new BugType(this->getCheckerName(), "error()", + categories::SecurityError)); + auto R = + std::make_unique(*BT, "error() called", N); + R->addVisitor(); + C.emitReport(std::move(R)); + } + } +}; + +} // namespace +} // namespace ento +} // namespace clang + +//===----------------------------------------------------------------------===// +// Non-thorough analysis: only the state right before and right after the +// function call is checked for the difference in trait value. +//===----------------------------------------------------------------------===// + +namespace clang { +namespace ento { +namespace { + +class NonThoroughErrorNotPreventedFuncVisitor + : public ErrorNotPreventedFuncVisitor { +public: + virtual bool + wasModifiedInFunction(const ExplodedNode *CallEnterN, + const ExplodedNode *CallExitEndN) override { + return CallEnterN->getState()->get() != + CallExitEndN->getState()->get(); + } +}; + +void addNonThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer, + AnalyzerOptions &AnOpts) { + AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}}; + AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) { + Registry + .addChecker>( + "test.StatefulChecker", "Description", ""); + }); +} + +TEST(NoStateChangeFuncVisitor, NonThoroughFunctionAnalysis) { + std::string Diags; + EXPECT_TRUE(runCheckerOnCode(R"( + void error(); + void preventError(); + void allowError(); + + void g() { + //preventError(); + } + + void f() { + g(); + error(); + } + )", Diags)); + EXPECT_EQ(Diags, + "test.StatefulChecker: Calling 'g' | Returning without prevening " + "the error | Returning from 'g' | error() called\n"); + + Diags.clear(); + + EXPECT_TRUE(runCheckerOnCode(R"( + void error(); + void preventError(); + void allowError(); + + void g() { + preventError(); + allowError(); + } + + void f() { + g(); + error(); + } + )", Diags)); + EXPECT_EQ(Diags, + "test.StatefulChecker: Calling 'g' | Returning without prevening " + "the error | Returning from 'g' | error() called\n"); + + Diags.clear(); + + EXPECT_TRUE(runCheckerOnCode(R"( + void error(); + void preventError(); + void allowError(); + + void g() { + preventError(); + } + + void f() { + g(); + error(); + } + )", Diags)); + EXPECT_EQ(Diags, ""); +} + +} // namespace +} // namespace ento +} // namespace clang + +//===----------------------------------------------------------------------===// +// Thorough analysis: only the state right before and right after the +// function call is checked for the difference in trait value. +//===----------------------------------------------------------------------===// + +namespace clang { +namespace ento { +namespace { + +class ThoroughErrorNotPreventedFuncVisitor + : public ErrorNotPreventedFuncVisitor { +public: + virtual bool + wasModifiedBeforeCallExit(const ExplodedNode *CurrN, + const ExplodedNode *CallExitBeginN) override { + return CurrN->getState()->get() != + CallExitBeginN->getState()->get(); + } +}; + +void addThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer, + AnalyzerOptions &AnOpts) { + AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}}; + AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) { + Registry.addChecker>( + "test.StatefulChecker", "Description", ""); + }); +} + +TEST(NoStateChangeFuncVisitor, ThoroughFunctionAnalysis) { + std::string Diags; + EXPECT_TRUE(runCheckerOnCode(R"( + void error(); + void preventError(); + void allowError(); + + void g() { + //preventError(); + } + + void f() { + g(); + error(); + } + )", Diags)); + EXPECT_EQ(Diags, + "test.StatefulChecker: Calling 'g' | Returning without prevening " + "the error | Returning from 'g' | error() called\n"); + + Diags.clear(); + + EXPECT_TRUE(runCheckerOnCode(R"( + void error(); + void preventError(); + void allowError(); + + void g() { + preventError(); + allowError(); + } + + void f() { + g(); + error(); + } + )", Diags)); + EXPECT_EQ(Diags, "test.StatefulChecker: error() called\n"); + + Diags.clear(); + + EXPECT_TRUE(runCheckerOnCode(R"( + void error(); + void preventError(); + void allowError(); + + void g() { + preventError(); + } + + void f() { + g(); + error(); + } + )", Diags)); + EXPECT_EQ(Diags, ""); +} + +} // namespace +} // namespace ento +} // namespace clang diff --git a/clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp b/clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp --- a/clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp +++ b/clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp @@ -51,7 +51,7 @@ TEST(RegisterCustomCheckers, RegisterChecker) { std::string Diags; EXPECT_TRUE(runCheckerOnCode("void f() {;}", Diags)); - EXPECT_EQ(Diags, "test.CustomChecker:Custom diagnostic description\n"); + EXPECT_EQ(Diags, "test.CustomChecker: Custom diagnostic description\n"); } //===----------------------------------------------------------------------===// @@ -169,7 +169,7 @@ TEST(RegisterDeps, UnsatisfiedDependency) { std::string Diags; EXPECT_TRUE(runCheckerOnCode("void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.RegistrationOrder\n"); + EXPECT_EQ(Diags, "test.RegistrationOrder: test.RegistrationOrder\n"); } //===----------------------------------------------------------------------===// @@ -272,7 +272,7 @@ std::string Diags; EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep\ntest." "Dep\ntest.RegistrationOrder\n"); Diags.clear(); @@ -280,31 +280,33 @@ // but the dependencies are switched. EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.Dep\ntest." "RegistrationOrder\ntest.WeakDep\n"); Diags.clear(); // Weak dependencies dont prevent dependent checkers from being enabled. EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n"); + EXPECT_EQ(Diags, + "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n"); Diags.clear(); // Nor will they be enabled just because a dependent checker is. EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n"); + EXPECT_EQ(Diags, + "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n"); Diags.clear(); EXPECT_TRUE( runCheckerOnCode("void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep2\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep2\ntest." "Dep\ntest.RegistrationOrder\n"); Diags.clear(); EXPECT_TRUE( runCheckerOnCode("void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep2\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep2\ntest." "WeakDep\ntest.Dep\ntest.RegistrationOrder\n"); Diags.clear(); } @@ -414,7 +416,7 @@ std::string Diags; EXPECT_TRUE( runCheckerOnCode("void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.StrongDep\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.StrongDep\ntest." "WeakDep\ntest.Dep\ntest.RegistrationOrder\n"); Diags.clear(); @@ -424,14 +426,14 @@ // established in between the modeling portion and the weak dependency. EXPECT_TRUE( runCheckerOnCode("void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep\ntest." "StrongDep\ntest.Dep\ntest.RegistrationOrder\n"); Diags.clear(); // If a weak dependency is disabled, the checker itself can still be enabled. EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.Dep\ntest." "RegistrationOrder\ntest.StrongDep\n"); Diags.clear(); @@ -439,20 +441,22 @@ // but it shouldn't enable a strong unspecified dependency. EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n"); + EXPECT_EQ(Diags, + "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n"); Diags.clear(); // A strong dependency of a weak dependency is disabled, so neither of them // should be enabled. EXPECT_TRUE(runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n"); + EXPECT_EQ(Diags, + "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n"); Diags.clear(); EXPECT_TRUE( runCheckerOnCode( "void f() {int i;}", Diags)); - EXPECT_EQ(Diags, "test.RegistrationOrder:test.StrongDep\ntest.WeakDep\ntest." + EXPECT_EQ(Diags, "test.RegistrationOrder: test.StrongDep\ntest.WeakDep\ntest." "Dep\ntest.Dep2\ntest.RegistrationOrder\n"); Diags.clear(); }