Author: Kristóf Umann Date: 2021-09-02T17:19:49+02:00 New Revision: 3891b45a06f9192b7185d03269717cd60dfdea13
URL: https://github.com/llvm/llvm-project/commit/3891b45a06f9192b7185d03269717cd60dfdea13 DIFF: https://github.com/llvm/llvm-project/commit/3891b45a06f9192b7185d03269717cd60dfdea13.diff LOG: Revert "[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it" This reverts commit 7d0e62bfb773c68d2bc8831fddcc8536f4613190. Added: Modified: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp clang/unittests/StaticAnalyzer/CMakeLists.txt clang/unittests/StaticAnalyzer/CallEventTest.cpp clang/unittests/StaticAnalyzer/CheckerRegistration.h clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp Removed: clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index c42521376af92..139b0dcd51704 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -633,9 +633,6 @@ class CXXConstructorCall; /// 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. @@ -646,8 +643,6 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor { /// 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 diff icult to read. llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying; llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated; @@ -656,8 +651,6 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor { /// 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); @@ -665,37 +658,11 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor { protected: bugreporter::TrackingKind TKind; - /// \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; - } + /// \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; /// Consume the information on the non-modifying stack frame in order to /// either emit a note or not. May suppress the report entirely. @@ -735,6 +702,7 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor { }; } // 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 index d97e8c33ce254..7db4066653cbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -52,7 +52,6 @@ #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" @@ -77,10 +76,8 @@ #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 <climits> #include <functional> #include <utility> @@ -758,17 +755,6 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { 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: @@ -782,24 +768,31 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { return Ret; } - LLVM_DUMP_METHOD static std::string - getFunctionName(const ExplodedNode *CallEnterN) { - if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>( - CallEnterN->getLocationAs<CallEnter>()->getCallExpr())) - if (const FunctionDecl *FD = CE->getDirectCallee()) - return FD->getQualifiedNameAsString(); - return ""; + static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) { + while (N && !N->getLocationAs<CallExitEnd>()) + N = N->getFirstSucc(); + return N; } virtual bool - wasModifiedInFunction(const ExplodedNode *CallEnterN, - const ExplodedNode *CallExitEndN) override { - if (CallEnterN->getState()->get<RegionState>(Sym) != - CallExitEndN->getState()->get<RegionState>(Sym)) + wasModifiedBeforeCallExit(const ExplodedNode *CurrN, + const ExplodedNode *CallExitN) override { + if (CurrN->getLocationAs<CallEnter>()) + 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<RegionState>(Sym) != + CallExitN->getState()->get<RegionState>(Sym)) return true; - OwnerSet CurrOwners = getOwnersAtNode(CallEnterN); - OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN); + OwnerSet CurrOwners = getOwnersAtNode(CurrN); + OwnerSet ExitOwners = getOwnersAtNode(CallExitN); // 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 index 6f4143519ca61..ecd9b649c4f46 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -355,81 +355,43 @@ bool NoStateChangeFuncVisitor::isModifiedInFrame(const ExplodedNode *N) { 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<CallEnter>()); - // 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<CallExitEnd>() && - 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<CallExitBegin>()); - + const ExplodedNode *LastReturnN = CallExitBeginN; const StackFrameContext *const OriginalSCtx = CallExitBeginN->getLocationContext()->getStackFrame(); - 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<CallExitBegin>()) { - CurrCallExitBeginN = CurrN; - CurrentSCtx = CurrN->getStackFrame(); - FramesModifyingCalculated.insert(CurrentSCtx); - // We won't see a change in between two identical exploded nodes: skip. - continue; + const ExplodedNode *CurrN = CallExitBeginN; + + do { + ProgramStateRef State = CurrN->getState(); + auto CallExitLoc = CurrN->getLocationAs<CallExitBegin>(); + if (CallExitLoc) { + LastReturnN = CurrN; } - if (auto CE = CurrN->getLocationAs<CallEnter>()) { - 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; + 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 (wasModifiedBeforeCallExit(CurrN, CurrCallExitBeginN)) - markFrameAsModifying(CurrentSCtx); - } + // Stop calculation at the call to the current function. + if (auto CE = CurrN->getLocationAs<CallEnter>()) + if (CE->getCalleeContext() == OriginalSCtx) + break; + + CurrN = CurrN->getFirstPred(); + } while (CurrN); } PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode( diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt index 985edf4db3408..d131e03057b5d 100644 --- a/clang/unittests/StaticAnalyzer/CMakeLists.txt +++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt @@ -9,7 +9,6 @@ add_clang_unittest(StaticAnalysisTests 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 index b1ae591eaf7ec..43ee7320721b3 100644 --- a/clang/unittests/StaticAnalyzer/CallEventTest.cpp +++ b/clang/unittests/StaticAnalyzer/CallEventTest.cpp @@ -81,7 +81,7 @@ TEST(CXXDeallocatorCall, SimpleDestructor) { } )", 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 index de804236229ad..e02b856e0e9c4 100644 --- a/clang/unittests/StaticAnalyzer/CheckerRegistration.h +++ b/clang/unittests/StaticAnalyzer/CheckerRegistration.h @@ -6,7 +6,6 @@ // //===----------------------------------------------------------------------===// -#include "clang/Analysis/PathDiagnostic.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" @@ -27,23 +26,8 @@ class DiagConsumer : public PathDiagnosticConsumer { DiagConsumer(llvm::raw_ostream &Output) : Output(Output) {} void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags, FilesMade *filesMade) override { - 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'; - } + for (const auto *PD : Diags) + Output << PD->getCheckerName() << ":" << PD->getShortDescription() << '\n'; } StringRef getName() const override { return "Test"; } diff --git a/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp b/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp index beaaebdd36cf9..32bd3950d526c 100644 --- a/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp +++ b/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp @@ -128,7 +128,7 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, UnSatInTheMiddleNoReport) { EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>( 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 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, UnSatInTheMiddleNoReport) { EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>( 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 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>( 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 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>( 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 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>( 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 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>( 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 deleted file mode 100644 index 7e93ff0bd4dff..0000000000000 --- a/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp +++ /dev/null @@ -1,302 +0,0 @@ -//===- 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 <memory> - -//===----------------------------------------------------------------------===// -// 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<PathDiagnosticEventPiece>( - L, "Returning without prevening the error"); - } - - void Profile(llvm::FoldingSetNodeID &ID) const override { - static int Tag = 0; - ID.AddPointer(&Tag); - } -}; - -template <class Visitor> -class StatefulChecker : public Checker<check::PreCall> { - mutable std::unique_ptr<BugType> BT; - -public: - void checkPreCall(const CallEvent &Call, CheckerContext &C) const { - if (Call.isCalled(CallDescription{"preventError", 0})) { - C.addTransition(C.getState()->set<ErrorPrevented>(true)); - return; - } - - if (Call.isCalled(CallDescription{"allowError", 0})) { - C.addTransition(C.getState()->set<ErrorPrevented>(false)); - return; - } - - if (Call.isCalled(CallDescription{"error", 0})) { - if (C.getState()->get<ErrorPrevented>()) - 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<PathSensitiveBugReport>(*BT, "error() called", N); - R->addVisitor<Visitor>(); - 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 diff erence 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<ErrorPrevented>() != - CallExitEndN->getState()->get<ErrorPrevented>(); - } -}; - -void addNonThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer, - AnalyzerOptions &AnOpts) { - AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}}; - AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) { - Registry - .addChecker<StatefulChecker<NonThoroughErrorNotPreventedFuncVisitor>>( - "test.StatefulChecker", "Description", ""); - }); -} - -TEST(NoStateChangeFuncVisitor, NonThoroughFunctionAnalysis) { - std::string Diags; - EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(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<addNonThoroughStatefulChecker>(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<addNonThoroughStatefulChecker>(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 diff erence 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<ErrorPrevented>() != - CallExitBeginN->getState()->get<ErrorPrevented>(); - } -}; - -void addThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer, - AnalyzerOptions &AnOpts) { - AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}}; - AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) { - Registry.addChecker<StatefulChecker<ThoroughErrorNotPreventedFuncVisitor>>( - "test.StatefulChecker", "Description", ""); - }); -} - -TEST(NoStateChangeFuncVisitor, ThoroughFunctionAnalysis) { - std::string Diags; - EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(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<addThoroughStatefulChecker>(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<addThoroughStatefulChecker>(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 index 85cce190a65e9..60b8aafa0d84e 100644 --- a/clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp +++ b/clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp @@ -51,7 +51,7 @@ void addCustomChecker(AnalysisASTConsumer &AnalysisConsumer, TEST(RegisterCustomCheckers, RegisterChecker) { std::string Diags; EXPECT_TRUE(runCheckerOnCode<addCustomChecker>("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 @@ void addDep(AnalysisASTConsumer &AnalysisConsumer, TEST(RegisterDeps, UnsatisfiedDependency) { std::string Diags; EXPECT_TRUE(runCheckerOnCode<addDep>("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 @@ TEST(RegisterDeps, SimpleWeakDependency) { std::string Diags; EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerBothEnabled>( "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,33 +280,31 @@ TEST(RegisterDeps, SimpleWeakDependency) { // but the dependencies are switched. EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerBothEnabledSwitched>( "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<addWeakDepCheckerDepDisabled>( "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<addWeakDepCheckerDepUnspecified>( "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<addWeakDepTransitivity>("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<addWeakDepHasWeakDep>("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(); } @@ -416,7 +414,7 @@ TEST(RegisterDeps, DependencyInteraction) { std::string Diags; EXPECT_TRUE( runCheckerOnCode<addWeakDepHasStrongDep>("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(); @@ -426,14 +424,14 @@ TEST(RegisterDeps, DependencyInteraction) { // established in between the modeling portion and the weak dependency. EXPECT_TRUE( runCheckerOnCode<addWeakDepAndStrongDep>("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<addDisabledWeakDepHasStrongDep>( "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(); @@ -441,22 +439,20 @@ TEST(RegisterDeps, DependencyInteraction) { // but it shouldn't enable a strong unspecified dependency. EXPECT_TRUE(runCheckerOnCode<addDisabledWeakDepHasUnspecifiedStrongDep>( "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<addWeakDepHasDisabledStrongDep>( "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<addWeakDepHasUnspecifiedButLaterEnabledStrongDep>( "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(); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits