[cfe-commits] [PATCH] CompoundLiteral
Ted Kremenek
kremenek at apple.com
Tue Oct 28 10:44:50 PDT 2008
On Oct 27, 2008, at 11:15 PM, Zhongxing Xu wrote:
> Minor changes (forgot 'return store' in the last patch).
>
> On Tue, Oct 28, 2008 at 1:21 PM, Zhongxing Xu
> <xuzhongxing at gmail.com> wrote:
> I changed the BindCompoundLiteral to AddCompoundLiteral. Init SVals
> are not passed into Store explicitly. Instead a GRState is passed.
> Because we cannot assume the compound literal is array. It may be
> struct. Recursive processing is necessary.
>
> This patch is big enough. Later I will modify the AddDecl to follow
> the style of AddCompoundLiteral, so that we can process decl with
> init list.
Hi Zhongxing,
I think this patch is a good start, but I have some significant
concerns with where some of the code is going. In your
implementation, RegionStore actually walks the ASTs and interrogates
the Environment (via GRState). I strongly feel that such logic should
not be in an implementation of any Store; a Store should concern
itself with only mapping from regions to values (and possibly Decls to
regions); nothing more, nothing less.
The follow issues arise by having the AST processing logic in
RegionStore itself:
(a) This logic needs to be duplicated in every implementation of Store
that reasons about compound literals. This is an issue for concern
on its own, but if we change the representation of CompoundLiterals in
the AST only slightly we need to update every implementation of Store.
(b) Having RegionStore interrogate the Environment is very dangerous
because it makes assumptions that only GRExprEngine can make. First,
it assumes how GRExprEngine stores temporary values used in the
construction of compound literals. As you know, GRExprEngine removes
bindings from the environment when it feels that is safe to do so, and
we may change these optimizations at any time. A Store should not
have any knowledge of such optimizations.
(c) Ideally, a Store should have no knowledge of GRState. We should
be able to create a store object independently of a GRState. (on a
related topic, after looking at Store again, I don't think any of the
getLValueXXX methods should take a GRState* argument and should
instead take a Store argument).
I think keeping the clean separation of concerns between syntax and
semantics, while a little more effort at times, is completely worth it
in the long run. The design in the patch I made yesterday to add some
basic support for CompoundLiterals in GRExprEngine did this:
GRExprEngine interrogated the environment for the SVals of the
literals, and then passed them in bulk to the Store. In this way we
get a clean separation between syntax and semantics. Only
GRExprEngine and GRState (the environment aspect) should reason about
syntax. Store should never reason about syntax. Aside from a few
rare exceptions, we should pass AST nodes (Decl* or Stmt*) to Store
only to use them as identifiers. For example, the 'getLValueVar'
method maps from VarDecls to SVals.
Specific comments inline:
Index: include/clang/Analysis/PathSensitive/Store.h
===================================================================
--- include/clang/Analysis/PathSensitive/Store.h (revision 58306)
+++ include/clang/Analysis/PathSensitive/Store.h (working copy)
@@ -55,13 +55,25 @@
virtual Store BindCompoundLiteral(Store store, const
CompoundLiteralRegion* R,
const SVal* BegInit,
const SVal* EndInit) = 0;
-
+
+ /// Do things kind of like 'AddDecl()': Create region for compound
literal,
+ /// bind the init value to the region, create sub regions when
+ /// necessary. Assume all init list exprs are evaluated and has
Expr->SVal
+ /// bindings in the Environment. This interface is intended to
replace the
+ /// above BindCompoundLiteral. The AddDecl() will be changed to
follow the
+ /// style of this interface.
+ virtual Store AddCompoundLiteral(Store store, CompoundLiteralExpr*
CLE,
+ const GRState* state) = 0;
+
This method should take a Store instead of a GRState.
Index: include/clang/Analysis/PathSensitive/GRState.h
===================================================================
--- include/clang/Analysis/PathSensitive/GRState.h (revision 58306)
+++ include/clang/Analysis/PathSensitive/GRState.h (working copy)
@@ -338,6 +338,9 @@
-
+
+ // Get the lvalue for a CompoundLiteral.
+ SVal GetLValue(const GRState* St, const CompoundLiteralExpr* CL) {
+ return StoreMgr->getLValueCompoundLiteral(St, CL);
+ }
Looks good, except we should just pass St->getStore() to
getLValueCompoundLiteral.
Index: include/clang/Analysis/PathSensitive/MemRegion.h
===================================================================
--- include/clang/Analysis/PathSensitive/MemRegion.h (revision 58313)
+++ include/clang/Analysis/PathSensitive/MemRegion.h (working copy)
@@ -420,7 +420,7 @@
MemSpaceRegion* getUnknownRegion();
bool isGlobalsRegion(const MemRegion* R) {
- assert(R && globals);
+ assert(R);
return R == globals;
}
I think you pulled this into a separate patch already. Thanks for
keeping them separate.
Index: lib/Analysis/GRState.cpp
===================================================================
--- lib/Analysis/GRState.cpp (revision 58306)
+++ lib/Analysis/GRState.cpp (working copy)
@@ -112,6 +112,19 @@
return getPersistentState(newState);
}
+const GRState*
+GRStateManager::AddCompoundLiteral(const GRState* state,
+ CompoundLiteralExpr* CL) {
+ Store oldStore = state->getStore();
+ Store newStore = StoreMgr->AddCompoundLiteral(oldStore, CL, state);
+ if (newStore == oldStore)
+ return state;
+
+ GRState newState = *state;
+ newState.St = newStore;
+ return getPersistentState(newState);
+}
While this is simple, I'm not certain how we should marshall in the
SVals for the CompoundLiteral values. As I said before, we should not
pass a GRState* to StoreManager::AddCompoundLiteral, and the logic for
processing the syntax for CompoundLiterals should be hoisted into
GRExprEngine.
Index: lib/Analysis/BasicStore.cpp
===================================================================
+SVal BasicStoreManager::getLValueCompoundLiteral(const GRState* St,
+ const
CompoundLiteralExpr* CL){
+ return loc::MemRegionVal(MRMgr.getCompoundLiteralRegion(CL));
+}
+
It seems to me that this method will have the same implementation for
any Store. Perhaps we should just put it in StoreManager? (we can
allow it to be virtual to override in subclasses).
Index: lib/Analysis/RegionStore.cpp
===================================================================
Store InitializeArrayToUndefined(Store store, QualType T,
MemRegion* BaseR);
Store InitializeStructToUndefined(Store store, QualType T,
MemRegion* BaseR);
+
+ Store InitializeArrayWithInitList(Store store,
+ QualType T,
+ MemRegion* BaseR,
+ InitListExpr* ILE,
+ const GRState* state);
+
+ Store InitializeStructWithInitList(Store store,
+ QualType T,
+ MemRegion* BaseR,
+ InitListExpr* ILE,
+ const GRState* state);
};
The token "InitListExpr" should never appear in any implementation of
Store, as we already have CompoundLiteralExpr to identify a particular
compound literal. InitListExpr is syntactic packaging for the
syntactic representation of the values of a compound literal, and only
GRExprEngine should be aware of such details.
+Store RegionStoreManager::AddCompoundLiteral(Store store,
+ CompoundLiteralExpr* CLE,
+ const GRState* state) {
+
+ // A CompoundLiteral is an unnamed object, so we create a MemRegion
for it.
+ CompoundLiteralRegion* CLR = MRMgr.getCompoundLiteralRegion(CLE);
+
+ // Initialize the CompoundLiteral object according to its type.
+ QualType T = CLE->getType();
+
+ InitListExpr* ILE = cast<InitListExpr>(CLE->getInitializer());
+
+ if (Loc::IsLocType(T) || T->isIntegerType()) {
+ // Scalar type, simply fetch the sval of the init expr, bind the
region to
+ // it.
+ assert(++(ILE->child_begin()) == ILE->child_end());
+
+ Stmt::child_iterator CI = ILE->child_begin();
+
+ SVal V = StateMgr.GetSVal(state, cast<Expr>(*CI));
+
+ store = Bind(store, loc::MemRegionVal(CLR), V);
+
+ return store;
+ }
+
+ if (T->isArrayType()) {
+ store = InitializeArrayWithInitList(store, T, CLR, ILE, state);
+ return store;
+ }
+
+ if (T->isStructureType()) {
+ store = InitializeStructWithInitList(store, T, CLR, ILE, state);
+ return store;
+ }
+
+ assert(0 && "unprocessed CompoundLiteral type");
+ return store;
+}
+
All of this can easily get hoisted into GRExprEngine.
+SVal RegionStoreManager::getLValueCompoundLiteral(const GRState* St,
+ const
CompoundLiteralExpr* CL){
+ return loc::MemRegionVal(MRMgr.getCompoundLiteralRegion(CL));
+}
+
This method has the same implementation as the one in
BasicStoreManager. Let's just move it to StoreManager.
+Store RegionStoreManager::InitializeArrayWithInitList(Store store,
+ QualType T,
+ MemRegion* BaseR,
+ InitListExpr*
ILE,
+ const GRState*
state) {
+ assert(T->isArrayType());
+ assert(BaseR);
+ assert(ILE);
+
+ bool isGlobal=MRMgr.isGlobalsRegion(cast<SubRegion>(BaseR)-
>getSuperRegion());
+
+ BasicValueFactory& BasicVals = StateMgr.getBasicVals();
+
+ if (ConstantArrayType* CAT =
dyn_cast<ConstantArrayType>(T.getTypePtr())) {
+ llvm::APInt Size = CAT->getSize();
+ llvm::APInt i = llvm::APInt::getNullValue(Size.getBitWidth());
+
+ InitListExpr::child_iterator I = ILE->child_begin(), E = ILE-
>child_end();
+
+ for (; i != Size; ++i) {
+ // Get the unique SVal.
+ nonloc::ConcreteInt Idx(BasicVals.getValue(llvm::APSInt(i)));
+ ElementRegion* ER = MRMgr.getElementRegion(Idx, BaseR);
+
+ if (I != E) {
+ SVal V = StateMgr.GetSVal(state, cast<Expr>(*I));
+ store = Bind(store, loc::MemRegionVal(ER), V);
+ ++I;
+
+ } else {
+ if (isGlobal) {
+ llvm::APInt ZeroInt =
llvm::APInt::getNullValue(Size.getBitWidth());
+ // Get the unique SVal.
+ nonloc::ConcreteInt
ZeroVal(BasicVals.getValue(llvm::APSInt(ZeroInt)));
+ store = Bind(store, loc::MemRegionVal(ER), ZeroVal);
+ } else
+ store = Bind(store, loc::MemRegionVal(ER), UndefinedVal());
+ }
+
This is great how this distinguishes between behavior for globals and
local variables when default initializing values in the array that are
not provided by the compound literal. From what I can tell, however,
simply passing in an array of SVals (as my previous patch did) would
work just as well, and RegionStore (or any Store) wouldn't have to
reason about InitListExpr or the environment.
Store RegionStoreManager::InitializeStructToUndefined(Store store,
QualType T,
MemRegion*
BaseR) {
QualType CT = StateMgr.getContext().getCanonicalType(T);
@@ -412,3 +520,38 @@
return store;
}
+
+Store RegionStoreManager::InitializeStructWithInitList(Store store,
+ QualType T,
+ MemRegion*
BaseR,
+ InitListExpr*
ILE,
+ const GRState*
state) {
+ QualType CT = StateMgr.getContext().getCanonicalType(T);
+ const RecordType* RT = cast<RecordType>(CT.getTypePtr());
+ RecordDecl* RD = RT->getDecl();
+ assert(RD->isDefinition());
+
+ RecordDecl::field_iterator FI = RD->field_begin(), FE = RD-
>field_end();
+ InitListExpr::child_iterator EI = ILE->child_begin(), EE = ILE-
>child_end();
+
+ for (; FI != FE; ++FI, ++EI) {
+
+ QualType FTy = (*FI)->getType();
+ FieldRegion* FR = MRMgr.getFieldRegion(*FI, BaseR);
+
+ if (Loc::IsLocType(FTy) || FTy->isIntegerType()) {
+ SVal V = StateMgr.GetSVal(state, cast<Expr>(*EI));
+ store = Bind(store, loc::MemRegionVal(FR), V);
+
+ } else if (FTy->isArrayType()) {
+ store = InitializeArrayWithInitList(store, FTy, FR,
+ cast<InitListExpr>(*EI),
state);
+
+ } else if (FTy->isStructureType()) {
+ store = InitializeStructWithInitList(store, FTy, FR,
+ cast<InitListExpr>(*EI),
state);
+ }
+ }
I like the general design, but I think that most of this can be
hoisted into GRExprEngine and then marshaled in using a flattened
array of SVals. InitializeStructWithInitList would take that array,
and then recurse over the types for the CompoundLiteral, binding the
correct values as before. We would then just pass iterators (SVal*)
and QualTypes (for sub-arrays and sub-structs) down in recursive calls
to InitArrayWithinInitList and InitStructWithInitList (which would be
renamed) respectively. This gives us a clean separation between
syntax and semantics.
Index: lib/Analysis/GRExprEngine.cpp
===================================================================
--- lib/Analysis/GRExprEngine.cpp (revision 58306)
+++ lib/Analysis/GRExprEngine.cpp (working copy)
@@ -1564,23 +1564,13 @@
Visit(ILE, Pred, Tmp);
for (NodeSet::iterator I = Tmp.begin(), EI = Tmp.end(); I!=EI; +
+I) {
- // Retrieve the initializer values from the environment and store
them
- // into a vector that will then be handed off to the Store.
+
const GRState* St = GetState(*I);
- llvm::SmallVector<SVal, 10> IVals;
- IVals.reserve(ILE->getNumInits());
-
- for (Stmt::child_iterator J=ILE->child_begin(), EJ=ILE-
>child_end();
- J!=EJ; ++J)
- IVals.push_back(GetSVal(St, cast<Expr>(*J)));
- const CompoundLiteralRegion* R =
- StateMgr.getRegionManager().getCompoundLiteralRegion(CL);
-
- assert (!IVals.empty() && "Initializer cannot be empty.");
+ St = StateMgr.AddCompoundLiteral(St, CL);
- St = StateMgr.BindCompoundLiteral(St, R, &IVals[0],
&IVals[0]+IVals.size());
- MakeNode(Dst, CL, *I, SetSVal(St, CL, loc::MemRegionVal(R)));
+ // FIXME: CompoundLiteralExpr can be used in both lvalue and
rvalue context.
+ MakeNode(Dst, CL, *I, SetSVal(St, CL, StateMgr.GetLValue(St, CL)));
}
}
I really think that all of the syntax processing should go here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081028/308f0e07/attachment.html>
More information about the cfe-commits
mailing list