Skip to content

Commit

Permalink
Revert "[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to…
Browse files Browse the repository at this point in the history 
… check entire function calls, rather than each ExplodedNode in it"

This reverts commit 7d0e62b.
  • Loading branch information
@Szelethus
Szelethus committed Sep 2, 2021
1 parent e962718 commit 3891b45
Show file tree
Hide file tree
Showing 9 changed files with 77 additions and 477 deletions.
44 changes: 6 additions & 38 deletions clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
View file
Expand Up @@ -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.
Expand All @@ -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 difficult to read.
llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying;
llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;

Expand All @@ -656,46 +651,18 @@ 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);

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.
Expand Down Expand Up @@ -735,6 +702,7 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor {
};

} // namespace ento

} // namespace clang

#endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
47 changes: 20 additions & 27 deletions clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
View file
Expand Up @@ -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"
Expand All @@ -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>
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down
88 changes: 25 additions & 63 deletions clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
View file
Expand Up @@ -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(
Expand Down
1 change: 0 additions & 1 deletion clang/unittests/StaticAnalyzer/CMakeLists.txt
View file
Expand Up @@ -9,7 +9,6 @@ add_clang_unittest(StaticAnalysisTests
CallDescriptionTest.cpp
CallEventTest.cpp
FalsePositiveRefutationBRVisitorTest.cpp
NoStateChangeFuncVisitorTest.cpp
ParamRegionTest.cpp
RangeSetTest.cpp
RegisterCustomCheckersTest.cpp
Expand Down
2 changes: 1 addition & 1 deletion clang/unittests/StaticAnalyzer/CallEventTest.cpp
View file
Expand Up @@ -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
Expand Down
20 changes: 2 additions & 18 deletions clang/unittests/StaticAnalyzer/CheckerRegistration.h
View file
Expand Up @@ -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"
Expand All @@ -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"; }
Expand Down
18 changes: 9 additions & 9 deletions clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
View file
Expand Up @@ -128,16 +128,16 @@ 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.
std::string Diags2;
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,
Expand All @@ -159,16 +159,16 @@ 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.
std::string Diags2;
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,
Expand Down Expand Up @@ -206,16 +206,16 @@ 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.
std::string Diags2;
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
Expand Down

0 comments on commit 3891b45

Please sign in to comment.