[cfe-dev] [PATCH] enable dynamic loading components for static analyzer
Ted Kremenek
kremenek at apple.com
Wed Nov 26 13:22:53 PST 2008
On Nov 26, 2008, at 2:56 AM, Zhongxing Xu wrote:
> This patch enables dynamically loaded components for static
> analyzer. It uses global objects of Register* classes. More comments
> in the patch.
Looks great! I'm really excited about this. My biggest meta point is
that I feel that ManagerRegister and RegisterConstraintManager should
go in libDriver, not libAnalysis (feel free to comment about this
opinion). Specific comments inline:
Index: include/clang/Analysis/PathSensitive/GRExprEngine.h
===================================================================
--- include/clang/Analysis/PathSensitive/GRExprEngine.h (revision 60079)
+++ include/clang/Analysis/PathSensitive/GRExprEngine.h (working copy)
@@ -183,7 +183,9 @@
public:
GRExprEngine(CFG& cfg, Decl& CD, ASTContext& Ctx, LiveVariables& L,
GRStateManager::StoreManagerCreator SMC =
- CreateBasicStoreManager);
+ CreateBasicStoreManager,
+ GRStateManager::ConstraintManagerCreator CMC =
+ CreateBasicConstraintManager);
Looks good.
Index: include/clang/Analysis/PathSensitive/ManagerRegister.h
===================================================================
--- include/clang/Analysis/PathSensitive/ManagerRegister.h (revision 0)
+++ include/clang/Analysis/PathSensitive/ManagerRegister.h (revision 0)
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_ANALYSIS_MANAGER_REGISTER_H
+#define LLVM_CLANG_ANALYSIS_MANAGER_REGISTER_H
+
+namespace clang {
+
+class StoreManager;
+class ConstraintManager;
+class GRStateManager;
+
+/// ManagerRegister - This class records manager creators registered at
+/// runtime. The information is communicated to AnalysisManager
through static
+/// members. Better design is expected.
+
+class ManagerRegister {
+public:
+ typedef ConstraintManager* (*ConstraintManagerCreator)
(GRStateManager&);
+ typedef StoreManager* (*StoreManagerCreator)(GRStateManager&);
Aren't these typedefs the same as in the ones in GRStateManager?
Maybe we should just move those typedefs into the clang namespace and
have them used by both GRStateManager and ManagerRegister (if not,
let's just keep the ones in GRStateManager). It seems silly to have
the same typedefs defined twice, which can lead to weird warnings if
we don't keep them in sync.
Also, consider naming "ManagerRegister" as "ManagerRegistrar".
Register is a verb, Registrar is the noun.
+
+ static StoreManagerCreator StoreMgrCreator;
+ static ConstraintManagerCreator ConstraintMgrCreator;
+};
I see. This assumes that in a single clang process we have one
registered StoreManagerCreator and ConstraintManagerCreator. This
seems fine as long as GRExprEngine and GRStateManager have no notion
of this registrar class. That way clients that wish to instantiate
different GRExprEngines within the same clang process with different
StoreManagers/ConstraintManagers can do so.
After looking at this, I think that ManagerRegistrar should go in the
Driver (or libDriver), not libAnalysis. It really has to do with
machinery of how to drive libAnalysis, but is not fundamentally a part
of it.
Index: include/clang/Analysis/PathSensitive/ConstraintManager.h
===================================================================
--- include/clang/Analysis/PathSensitive/ConstraintManager.h (revision
60079)
+++ include/clang/Analysis/PathSensitive/ConstraintManager.h (working
copy)
@@ -16,6 +16,7 @@
// FIXME: Typedef LiveSymbolsTy/DeadSymbolsTy at a more appropriate
place.
#include "clang/Analysis/PathSensitive/Store.h"
+#include "clang/Analysis/PathSensitive/ManagerRegister.h"
We shouldn't add a dependency here on ManagerRegister.h. I think that
"RegisterConstraintManager" should just go in "ManagerRegistrar.h",
and ManagerRegistrar.h could just include ConstraintManager.h. Again,
this is all driver logic.
namespace llvm {
class APSInt;
@@ -55,6 +56,25 @@
ConstraintManager* CreateBasicConstraintManager(GRStateManager&
statemgr);
+/// RegisterConstraintManager - This class is used to setup the
constraint
+/// manager of the static analyzer. The constructor takes a creator
function
+/// pointer for creating the constraint manager.
+///
+/// It is used like this:
+///
+/// class MyConstraintManager {};
+/// ConstraintManager* CreateMyConstraintManager(GRStateManager&
statemgr) {
+/// return new MyConstraintManager(statemgr);
+/// }
+/// RegisterConstraintManager X(CreateMyConstraintManager);
+
+class RegisterConstraintManager {
+public:
+ RegisterConstraintManager(ManagerRegister::ConstraintManagerCreator
CMC) {
+ ManagerRegister::ConstraintMgrCreator = CMC;
Let's add an assertion here:
assert(ManagerRegister::ConstraintMgrCreator == 0 &&
"ConstraintMgrCreator already set!");
+ }
+};
+
Looks great. This nicely mirrors the plugin logic in opt. Let's put
this in libDriver.
#endif
Index: Driver/AnalysisConsumer.cpp
===================================================================
--- Driver/AnalysisConsumer.cpp (revision 60079)
+++ Driver/AnalysisConsumer.cpp (working copy)
@@ -129,12 +129,20 @@
llvm::OwningPtr<LiveVariables> liveness;
llvm::OwningPtr<ParentMap> PM;
+ // Configurable components creators.
+ GRStateManager::StoreManagerCreator CreateStoreMgr;
+ GRStateManager::ConstraintManagerCreator CreateConstraintMgr;
+
Looks good.
public:
AnalysisManager(AnalysisConsumer& c, Decl* d, Stmt* b)
- : D(d), Body(b), TU(0), AScope(ScopeDecl), C(c),
DisplayedFunction(false) {}
+ : D(d), Body(b), TU(0), AScope(ScopeDecl), C(c),
DisplayedFunction(false) {
+ setManagerCreators();
+ }
AnalysisManager(AnalysisConsumer& c, TranslationUnit* tu)
- : D(0), Body(0), TU(tu), AScope(ScopeTU), C(c),
DisplayedFunction(false) {}
+ : D(0), Body(0), TU(tu), AScope(ScopeTU), C(c),
DisplayedFunction(false) {
+ setManagerCreators();
+ }
Decl* getCodeDecl() const {
assert (AScope == ScopeDecl);
@@ -152,13 +160,12 @@
}
GRStateManager::StoreManagerCreator getStoreManagerCreator() {
- switch (C.SM) {
- default:
-#define ANALYSIS_STORE(NAME, CMDFLAG, DESC)\
-case NAME##Model: return Create##NAME##Manager;
-#include "Analyses.def"
- }
+ return CreateStoreMgr;
};
+
+ GRStateManager::ConstraintManagerCreator
getConstraintManagerCreator() {
+ return CreateConstraintMgr;
+ }
virtual CFG* getCFG() {
if (!cfg) cfg.reset(CFG::buildCFG(getBody()));
@@ -215,7 +222,7 @@
bool shouldVisualizeGraphviz() const {
return C.VisGraphviz;
}
-
+
bool shouldVisualizeUbigraph() const {
return C.VisUbigraph;
}
@@ -249,8 +256,33 @@
<< MD->getSelector().getAsString() << "'\n";
}
}
+
+ private:
+ /// Set configurable analyzer components creators. First check if
there are
+ /// components registered at runtime. Otherwise fall back to
builtin
+ /// components.
+ void setManagerCreators() {
+ if (ManagerRegister::StoreMgrCreator != 0) {
+ CreateStoreMgr = ManagerRegister::StoreMgrCreator;
+ }
+ else {
+ switch (C.SM) {
+ default:
+ assert(0 && "Unknown store manager.");
+#define ANALYSIS_STORE(NAME, CMDFLAG, DESC) \
+ case NAME##Model: CreateStoreMgr = Create##NAME##Manager;
break;
+#include "Analyses.def"
+ }
+ }
+
+ if (ManagerRegister::ConstraintMgrCreator != 0)
+ CreateConstraintMgr = ManagerRegister::ConstraintMgrCreator;
+ else
+ CreateConstraintMgr = CreateBasicConstraintManager;
+ }
+
This is sweet. Nice work.
Index: lib/Analysis/ManagerRegister.cpp
===================================================================
--- lib/Analysis/ManagerRegister.cpp (revision 0)
+++ lib/Analysis/ManagerRegister.cpp (revision 0)
@@ -0,0 +1,9 @@
+#include "clang/Analysis/PathSensitive/ManagerRegister.h"
+
+using namespace clang;
+
+ManagerRegister::StoreManagerCreator
+ManagerRegister::StoreMgrCreator = 0;
+
+ManagerRegister::ConstraintManagerCreator
+ManagerRegister::ConstraintMgrCreator = 0;
Looks great. Move to lib/Driver.
Index: lib/Analysis/GRExprEngine.cpp
===================================================================
--- lib/Analysis/GRExprEngine.cpp (revision 60079)
+++ lib/Analysis/GRExprEngine.cpp (working copy)
@@ -116,13 +116,13 @@
GRExprEngine::GRExprEngine(CFG& cfg, Decl& CD, ASTContext& Ctx,
LiveVariables& L,
- GRStateManager::StoreManagerCreator SMC)
+ GRStateManager::StoreManagerCreator SMC,
+ GRStateManager::ConstraintManagerCreator
CMC)
: CoreEngine(cfg, CD, Ctx, *this),
G(CoreEngine.getGraph()),
Liveness(L),
Builder(NULL),
- StateMgr(G.getContext(), SMC,
- CreateBasicConstraintManager, G.getAllocator(), cfg, CD,
L),
+ StateMgr(G.getContext(), SMC, CMC, G.getAllocator(), cfg, CD, L),
SymMgr(StateMgr.getSymbolManager()),
CurrentStmt(NULL),
NSExceptionII(NULL), NSExceptionInstanceRaiseSelectors(NULL),
Looks great.
More information about the cfe-dev
mailing list