[clang] 0213d7e - [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
Mon Sep 13 04:51:03 PDT 2021
Author: Kristóf Umann
Date: 2021-09-13T13:50:01+02:00
New Revision: 0213d7ec0c501414d12020737fdc47e47e4392d9
URL: https://github.com/llvm/llvm-project/commit/0213d7ec0c501414d12020737fdc47e47e4392d9
DIFF: https://github.com/llvm/llvm-project/commit/0213d7ec0c501414d12020737fdc47e47e4392d9.diff
LOG: [analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it
Fix a compilation error due to a missing 'template' keyword.
Differential Revision: https://reviews.llvm.org/D108695
Added:
clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
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:
################################################################################
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index 139b0dcd51704..c42521376af92 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -633,6 +633,9 @@ 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.
@@ -643,6 +646,8 @@ 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;
@@ -651,6 +656,8 @@ 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);
@@ -658,11 +665,37 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor {
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 @@ 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 7db4066653cbd..d97e8c33ce254 100644
--- 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 <climits>
#include <functional>
#include <utility>
@@ -755,6 +758,17 @@ 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:
@@ -768,31 +782,24 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
return Ret;
}
- static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
- while (N && !N->getLocationAs<CallExitEnd>())
- N = N->getFirstSucc();
- return N;
+ 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 "";
}
virtual bool
- 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))
+ wasModifiedInFunction(const ExplodedNode *CallEnterN,
+ const ExplodedNode *CallExitEndN) override {
+ if (CallEnterN->getState()->get<RegionState>(Sym) !=
+ CallExitEndN->getState()->get<RegionState>(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
index ecd9b649c4f46..ab3059bf7b0fd 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -355,43 +355,82 @@ bool NoStateChangeFuncVisitor::isModifiedInFrame(const ExplodedNode *N) {
return FramesModifying.count(SCtx);
}
+void NoStateChangeFuncVisitor::markFrameAsModifying(
+ const StackFrameContext *SCtx) {
+ while (!SCtx->inTopFrame()) {
+ 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 *CurrN = CallExitBeginN;
-
- do {
- ProgramStateRef State = CurrN->getState();
- auto CallExitLoc = CurrN->getLocationAs<CallExitBegin>();
- 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<CallExitBegin>()) {
+ 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<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;
}
}
- // 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);
+ if (wasModifiedBeforeCallExit(CurrN, CurrCallExitBeginN))
+ markFrameAsModifying(CurrentSCtx);
+ }
}
PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index d131e03057b5d..985edf4db3408 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -9,6 +9,7 @@ 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 43ee7320721b3..b1ae591eaf7ec 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 e02b856e0e9c4..de804236229ad 100644
--- 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 @@ 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() << ":" << 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
index 32bd3950d526c..beaaebdd36cf9 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
new file mode 100644
index 0000000000000..a553aae039cdc
--- /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 <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->template 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 60b8aafa0d84e..85cce190a65e9 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,31 +280,33 @@ 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();
}
@@ -414,7 +416,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();
@@ -424,14 +426,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();
@@ -439,20 +441,22 @@ 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