Szelethus created this revision.
Szelethus added reviewers: NoQ, martong, balazske, vsavchenko, steakhal,
ASDenysPetrov.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun,
whisperity.
Szelethus requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
D105553 <https://reviews.llvm.org/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
<https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D108695
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -355,43 +355,70 @@
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 *getCallExitEnd(const ExplodedNode *N) {
+ while (N && !N->getLocationAs<CallExitEnd>())
+ 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;
+
+ while (CurrN) {
+ // 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.
+ CurrN = CurrN->getFirstPred();
+ 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 = getCallExitEnd(CurrCallExitBeginN))
+ 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;
+ if (wasModifiedBeforeCallExit(CurrN, CurrCallExitBeginN))
+ markFrameAsModifying(CurrentSCtx);
CurrN = CurrN->getFirstPred();
- } while (CurrN);
+ }
}
PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ 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 @@
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 @@
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;
-
+ wasModifiedInFunction(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitEndN) override {
if (CurrN->getState()->get<RegionState>(Sym) !=
- CallExitN->getState()->get<RegionState>(Sym))
+ CallExitEndN->getState()->get<RegionState>(Sym))
return true;
OwnerSet CurrOwners = getOwnersAtNode(CurrN);
- OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+ 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
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -651,6 +651,8 @@
/// 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 +660,23 @@
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.
+ /// \return Whether the state was modified from the current node, \p CurrN, to
+ /// the end of the stack fram, at \p CallExitBeginN. \p CurrN and
+ /// \p CallExitBeginN are always in the same stack frame.
virtual bool
wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
- const ExplodedNode *CallExitBeginN) = 0;
+ const ExplodedNode *CallExitBeginN) {
+ return false;
+ }
+
+ /// \return Whether the state was modified in the inlined function call in
+ /// between \p CallEnterN and \p CallExitBeginN. Mind that the stack frame
+ /// retrieved from a CallEnter is the *caller's* stack frame! The inlined
+ /// function's stack should be retrieved from \p CallExitBeginN.
+ 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
