This does look fairly straightforward, and fairly good! But I'm concerned about
the FunctionDecl parameter: the analyzer also allows ObjCMethodDecls and
BlockDecls to be analyzed as top-level entry points. Perhaps a PointerUnion3
would be the best thing to do here, at least for now? Or alternately, we allow
people to specify which initial state they're checking, either by having
several callbacks or using a template parameter. What do you think?
Also, one nice thing about checkInitialState is that it can be non-const, since
it is guaranteed to run 0 or 1 times before the checker is used elsewhere. :-)
> - Handling NULL ProgramStateRef-s. I do not think it is reasonable to have
> NULL as the initial state. Probably assert when a checkInitialState call
> returns NULL?
I think a NULL initial state means a checker has decided not to analyze this
function. Weird and unlikely, but possible. (For example, it allows us to move
the -analyze-function option into a checker.)
> - Whether to call the checkInitialState callback in
> ExprEngine::getInitialState, or where getInitialState is used. I found one
> such method: CoreEngine::ExecuteWorkList. Logically - knowing nothing of the
> codebase -, I think it makes sense to do this in getInitialState. It does not
> appear to belong to CoreEngine, especially since I cannot find any references
> to getCheckerManager() in CoreEngine.
I agree. Can you explain why you needed to move the getInitialState call
earlier? It seems dangerous to run checkers before we have a block counter in
place, since conjured symbols use the block count.
Do we have any checkers that can use this? Perhaps as proof-of-concept you can
move the check for main's argc and argv into a checker.
> Once checkInitialState is working, I also plan to do a void
> checkBeginFunction(const FunctionDecl*, CheckerContext&) const, i.e. a
> callback that is called whenever a function is entered, either through a
> function call (this would be triggered right after checkPreCall) or when it
> is the entry point of the analysis.
I'm not sold on this one yet. I mean, I get the idea, but it again seems like
something where we want a nicer interface than just "FunctionDecl" (and at the
very least need to handle ObjCMethodDecl as well). But we can discuss this
later.
I think you'd implement this by calling checkBeginFunction right after
checkInitialState and also from ExprEngine::inlineCall (right after
"State->enterStackFrame(...)")
Finally, style notes (guessing this comes from bouncing between projects):
> + if(!State) return NULL; // exit early in case of unfeasible state
Space after if, consequent statement on a separate line, comments also on a
separate line (and in the form of a complete sentence, with capitalization and
a period).
Thank you for putting this together!
Jordan
On Dec 13, 2013, at 17:03 , Gabor Kozar <[email protected]> wrote:
> Hi Jordan,
>
> I had an issue recently with the Static Analyzer that I wouldn't be able to
> detect when a function was entered - either through a call or serving as the
> entry point of the analysis. (I ended up using checkPreStmt and figuring out
> if we've been magically transported inside a function body and check the
> program state whether the necessary info was already recorded by
> checkPreCall. Not something I'd call nice by any means.)
>
> So I wrote to cfg-dev and you mentioned that you had no such callbacks
> currently implemented, although they have been needed before, and that
> patches were welcome. Since this is a fairly trivial project, I was
> comfortable with doing it in my very limited free time (especially because I
> still intend to work on the Static Analyzer open projects at some point when
> I'm able).
>
> Find the diff file attached. Since you've pointed me to
> ExprEngine::getInitialState, I just modified it to call any checkers
> registered for check::InitialState at the end. So the signature for the
> callback is:
> ProgramStateRef checkInitialState(const FunctionDecl*, ProgramStateRef) const
>
> There are a few things I'm not sure about:
> - Handling NULL ProgramStateRef-s. I do not think it is reasonable to have
> NULL as the initial state. Probably assert when a checkInitialState call
> returns NULL?
> - Whether to call the checkInitialState callback in
> ExprEngine::getInitialState, or where getInitialState is used. I found one
> such method: CoreEngine::ExecuteWorkList. Logically - knowing nothing of the
> codebase -, I think it makes sense to do this in getInitialState. It does not
> appear to belong to CoreEngine, especially since I cannot find any references
> to getCheckerManager() in CoreEngine.
>
> Please let me know what you think!
>
> Once checkInitialState is working, I also plan to do a void
> checkBeginFunction(const FunctionDecl*, CheckerContext&) const, i.e. a
> callback that is called whenever a function is entered, either through a
> function call (this would be triggered right after checkPreCall) or when it
> is the entry point of the analysis.
>
> I'm not sure about where can I notify the checkers of this though. When is
> the point when we can say "okay so let's start analyzing this function"? If
> you could point me to the method, that would help a lot. (Ironically, we also
> have a code comprehension project running where I work [at Ericsson], but I
> do not have access to server where we have the Clang trunk parsed, since I'm
> doing this from home in my free time.)
>
> Thanks!
>
> --
> Gábor 'ShdNx' Kozár
> [email protected]
>
Index: lib/StaticAnalyzer/Core/CheckerManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CheckerManager.cpp (revision 197277)
+++ lib/StaticAnalyzer/Core/CheckerManager.cpp (working copy)
@@ -36,7 +36,8 @@
!DeadSymbolsCheckers.empty() ||
!RegionChangesCheckers.empty() ||
!EvalAssumeCheckers.empty() ||
- !EvalCallCheckers.empty();
+ !EvalCallCheckers.empty() ||
+ !InitialStateCheckers.empty();
}
void CheckerManager::finishedCheckerRegistration() {
@@ -584,6 +585,15 @@
I->second->printState(Out, State, NL, Sep);
}
+ProgramStateRef
+CheckerManager::runCheckersForInitialState(const FunctionDecl* F, ProgramStateRef State) {
+ for(unsigned i = 0, e = InitialStateCheckers.size(); i != e; ++i) {
+ if(!State) return NULL; // exit early in case of unfeasible state
+ State = InitialStateCheckers[i](F, State);
+ }
+ return State;
+}
+
//===----------------------------------------------------------------------===//
// Internal registration functions for AST traversing.
//===----------------------------------------------------------------------===//
@@ -684,6 +694,10 @@
EndOfTranslationUnitCheckers.push_back(checkfn);
}
+void CheckerManager::_registerForInitialState(CheckInitialStateFunc checkfn) {
+ InitialStateCheckers.push_back(checkfn);
+}
+
//===----------------------------------------------------------------------===//
// Implementation details.
//===----------------------------------------------------------------------===//
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp (revision 197277)
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp (working copy)
@@ -163,8 +163,8 @@
}
}
}
-
- return state;
+
+ return getCheckerManager().runCheckersForInitialState(dyn_cast<FunctionDecl>(D), state);
}
ProgramStateRef
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CoreEngine.cpp (revision 197277)
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp (working copy)
@@ -166,6 +166,8 @@
if (G->num_roots() == 0) { // Initialize the analysis by constructing
// the root if none exists.
+ if(!InitState) InitState = SubEng.getInitialState(L);
+ //InitState = getCheckerManager().runCheckersForInitialState(dyn_cast<FunctionDecl>(L->getDecl()), InitState);
const CFGBlock *Entry = &(L->getCFG()->getEntry());
@@ -190,11 +192,8 @@
// Set the current block counter to being empty.
WList->setBlockCounter(BCounterFactory.GetEmptyCounter());
- if (!InitState)
- // Generate the root.
- generateNode(StartLoc, SubEng.getInitialState(L), 0);
- else
- generateNode(StartLoc, InitState, 0);
+ // Generate the root.
+ generateNode(StartLoc, InitState, 0);
}
// Check if we have a steps limit
Index: include/clang/StaticAnalyzer/Core/CheckerManager.h
===================================================================
--- include/clang/StaticAnalyzer/Core/CheckerManager.h (revision 197277)
+++ include/clang/StaticAnalyzer/Core/CheckerManager.h (working copy)
@@ -403,6 +403,15 @@
void runCheckersForPrintState(raw_ostream &Out, ProgramStateRef State,
const char *NL, const char *Sep);
+ /// \brief Run checkers for manipulating the initial program state at the
+ /// start of the analysis.
+ ///
+ /// \param F The function decl serving as entry point
+ /// \param State The initial program state
+ /// \returns Checkers can modify the state by returning a new one.
+ ProgramStateRef runCheckersForInitialState(const FunctionDecl* F,
+ ProgramStateRef State);
+
//===----------------------------------------------------------------------===//
// Internal registration functions for AST traversing.
//===----------------------------------------------------------------------===//
@@ -480,6 +489,9 @@
AnalysisManager&, BugReporter &)>
CheckEndOfTranslationUnit;
+ typedef CheckerFn<ProgramStateRef (const FunctionDecl *, ProgramStateRef)>
+ CheckInitialStateFunc;
+
typedef bool (*HandlesStmtFunc)(const Stmt *D);
void _registerForPreStmt(CheckStmtFunc checkfn,
HandlesStmtFunc isForStmtFn);
@@ -519,6 +531,8 @@
void _registerForEndOfTranslationUnit(CheckEndOfTranslationUnit checkfn);
+ void _registerForInitialState(CheckInitialStateFunc checkfn);
+
//===----------------------------------------------------------------------===//
// Internal registration functions for events.
//===----------------------------------------------------------------------===//
@@ -623,6 +637,8 @@
std::vector<CheckEndOfTranslationUnit> EndOfTranslationUnitCheckers;
+ std::vector<CheckInitialStateFunc> InitialStateCheckers;
+
struct EventInfo {
SmallVector<CheckEventFunc, 4> Checkers;
bool HasDispatcher;
Index: include/clang/StaticAnalyzer/Core/Checker.h
===================================================================
--- include/clang/StaticAnalyzer/Core/Checker.h (revision 197277)
+++ include/clang/StaticAnalyzer/Core/Checker.h (working copy)
@@ -415,6 +415,18 @@
}
};
+class InitialState {
+ template <typename CHECKER>
+ static ProgramStateRef _checkInitialState(void *checker, const FunctionDecl* decl, ProgramStateRef state) {
+ return ((const CHECKER *)checker)->checkInitialState(decl, state);
+ }
+public:
+ template <typename CHECKER>
+ static void _register(CHECKER *checker, CheckerManager &mgr) {
+ mgr._registerForInitialState(CheckerManager::CheckInitialStateFunc(checker, _checkInitialState<CHECKER>));
+ }
+};
+
} // end check namespace
namespace eval {
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
