[clang] 3891b45 - Revert "[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it"
Kristóf Umann via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 2 08:20:17 PDT 2021
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();
}
More information about the cfe-commits
mailing list