[clang] a375bfb - [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
Fri Sep 3 04:50:47 PDT 2021
Author: Kristóf Umann
Date: 2021-09-03T13:50:18+02:00
New Revision: a375bfb5b729e0f3ca8d5e001f423fa89e74de87
URL: https://github.com/llvm/llvm-project/commit/a375bfb5b729e0f3ca8d5e001f423fa89e74de87
DIFF: https://github.com/llvm/llvm-project/commit/a375bfb5b729e0f3ca8d5e001f423fa89e74de87.diff
LOG: [analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it
D105553 added NoStateChangeFuncVisitor, an abstract class to aid in creating
notes such as "Returning without writing to 'x'", or "Returning without changing
the ownership status of allocated memory". Its clients need to define, among
other things, what a change of state is.
For code like this:
f() {
g();
}
foo() {
f();
h();
}
We'd have a path in the ExplodedGraph that looks like this:
-- <g> -->
/ \
--- <f> --------> --- <h> --->
/ \ / \
-------- <foo> ------ <foo> -->
When we're interested in whether f neglected to change some property,
NoStateChangeFuncVisitor asks these questions:
÷×~
-- <g> -->
ß / \$ @&#*
--- <f> --------> --- <h> --->
/ \ / \
-------- <foo> ------ <foo> -->
Has anything changed in between # and *?
Has anything changed in between & and *?
Has anything changed in between @ and *?
...
Has anything changed in between $ and *?
Has anything changed in between × and ~?
Has anything changed in between ÷ and ~?
...
Has anything changed in between ß and *?
...
This is a rather thorough line of questioning, which is why in D105819, I was
only interested in whether state *right before* and *right after* a function
call changed, and early returned to the CallEnter location:
if (!CurrN->getLocationAs<CallEnter>())
return;
Except that I made a typo, and forgot to negate the condition. So, in this
patch, I'm fixing that, and under the same hood allow all clients to decide to
do this whole-function check instead of the thorough one.
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..7e93ff0bd4dff
--- /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->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