<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 27, 2008, at 11:15 PM, Zhongxing Xu wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Minor changes (forgot 'return store' in the last patch).<br><br><div class="gmail_quote">On Tue, Oct 28, 2008 at 1:21 PM, Zhongxing Xu <span dir="ltr"><<a href="mailto:xuzhongxing@gmail.com">xuzhongxing@gmail.com</a>></span> wrote:<br> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">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.<br> <br>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.<br> </blockquote></div></blockquote></div><br><div>Hi Zhongxing,</div><div><br></div><div>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.</div><div><br></div><div>The follow issues arise by having the AST processing logic in RegionStore itself:</div><div><br></div><div>(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.</div><div><br></div><div>(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.</div><div><br></div><div>(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).</div><div><br></div><div>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.</div><div><br></div><div>Specific comments inline:</div><div><br></div><div><div>Index: include/clang/Analysis/PathSensitive/Store.h</div><div>===================================================================</div><div>--- include/clang/Analysis/PathSensitive/Store.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 58306)</div><div>+++ include/clang/Analysis/PathSensitive/Store.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -55,13 +55,25 @@</div><div> virtual Store BindCompoundLiteral(Store store, const CompoundLiteralRegion* R,</div><div> const SVal* BegInit,</div><div> const SVal* EndInit) = 0;</div><div>- </div><div>+</div><div>+ /// Do things kind of like 'AddDecl()': Create region for compound literal,</div><div>+ /// bind the init value to the region, create sub regions when</div><div>+ /// necessary. Assume all init list exprs are evaluated and has Expr->SVal</div><div>+ /// bindings in the Environment. This interface is intended to replace the</div><div>+ /// above BindCompoundLiteral. The AddDecl() will be changed to follow the</div><div>+ /// style of this interface.</div><div>+ virtual Store AddCompoundLiteral(Store store, CompoundLiteralExpr* CLE,</div><div>+ const GRState* state) = 0;</div><div>+</div><div><br></div><div>This method should take a Store instead of a GRState.</div><div><br></div><div>Index: include/clang/Analysis/PathSensitive/GRState.h</div><div>===================================================================</div><div>--- include/clang/Analysis/PathSensitive/GRState.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 58306)</div><div>+++ include/clang/Analysis/PathSensitive/GRState.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -338,6 +338,9 @@</div><div><br></div><div>- </div><div>+</div><div>+ // Get the lvalue for a CompoundLiteral.</div><div>+ SVal GetLValue(const GRState* St, const CompoundLiteralExpr* CL) {</div><div>+ return StoreMgr->getLValueCompoundLiteral(St, CL);</div><div>+ }</div><div><br></div><div>Looks good, except we should just pass St->getStore() to getLValueCompoundLiteral.</div><div><br></div><div>Index: include/clang/Analysis/PathSensitive/MemRegion.h</div><div>===================================================================</div><div>--- include/clang/Analysis/PathSensitive/MemRegion.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 58313)</div><div>+++ include/clang/Analysis/PathSensitive/MemRegion.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -420,7 +420,7 @@</div><div> MemSpaceRegion* getUnknownRegion();</div><div> </div><div> bool isGlobalsRegion(const MemRegion* R) { </div><div>- assert(R && globals);</div><div>+ assert(R);</div><div> return R == globals; </div><div> }</div><div><br></div><div>I think you pulled this into a separate patch already. Thanks for keeping them separate.</div><div><br></div><div> </div><div>Index: lib/Analysis/GRState.cpp</div><div>===================================================================</div><div>--- lib/Analysis/GRState.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 58306)</div><div>+++ lib/Analysis/GRState.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -112,6 +112,19 @@</div><div> return getPersistentState(newState);</div><div> }</div><div> </div><div>+const GRState*</div><div>+GRStateManager::AddCompoundLiteral(const GRState* state,</div><div>+ CompoundLiteralExpr* CL) {</div><div>+ Store oldStore = state->getStore();</div><div>+ Store newStore = StoreMgr->AddCompoundLiteral(oldStore, CL, state);</div><div>+ if (newStore == oldStore)</div><div>+ return state;</div><div>+</div><div>+ GRState newState = *state;</div><div>+ newState.St = newStore;</div><div>+ return getPersistentState(newState);</div><div>+}</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>Index: lib/Analysis/BasicStore.cpp</div><div>===================================================================</div><div><br></div><div>+SVal BasicStoreManager::getLValueCompoundLiteral(const GRState* St,</div><div>+ const CompoundLiteralExpr* CL){</div><div>+ return loc::MemRegionVal(MRMgr.getCompoundLiteralRegion(CL));</div><div>+}</div><div>+</div><div><br></div><div>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).</div><div><br></div><div><br></div><div><br></div><div>Index: lib/Analysis/RegionStore.cpp</div><div>===================================================================</div><div> </div><div> Store InitializeArrayToUndefined(Store store, QualType T, MemRegion* BaseR);</div><div> Store InitializeStructToUndefined(Store store, QualType T, MemRegion* BaseR);</div><div>+</div><div>+ Store InitializeArrayWithInitList(Store store,</div><div>+ QualType T,</div><div>+ MemRegion* BaseR,</div><div>+ InitListExpr* ILE,</div><div>+ const GRState* state);</div><div>+</div><div>+ Store InitializeStructWithInitList(Store store,</div><div>+ QualType T,</div><div>+ MemRegion* BaseR,</div><div>+ InitListExpr* ILE,</div><div>+ const GRState* state);</div><div> };</div><div><br></div><div>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.</div><div><br></div><div>+Store RegionStoreManager::AddCompoundLiteral(Store store, </div><div>+ CompoundLiteralExpr* CLE,</div><div>+ const GRState* state) {</div><div>+</div><div>+ // A CompoundLiteral is an unnamed object, so we create a MemRegion for it.</div><div>+ CompoundLiteralRegion* CLR = MRMgr.getCompoundLiteralRegion(CLE);</div><div>+</div><div>+ // Initialize the CompoundLiteral object according to its type.</div><div>+ QualType T = CLE->getType();</div><div>+</div><div>+ InitListExpr* ILE = cast<InitListExpr>(CLE->getInitializer());</div><div>+</div><div>+ if (Loc::IsLocType(T) || T->isIntegerType()) {</div><div>+ // Scalar type, simply fetch the sval of the init expr, bind the region to</div><div>+ // it.</div><div>+ assert(++(ILE->child_begin()) == ILE->child_end());</div><div>+</div><div>+ Stmt::child_iterator CI = ILE->child_begin();</div><div>+</div><div>+ SVal V = StateMgr.GetSVal(state, cast<Expr>(*CI));</div><div>+</div><div>+ store = Bind(store, loc::MemRegionVal(CLR), V);</div><div>+</div><div>+ return store;</div><div>+ }</div><div>+</div><div>+ if (T->isArrayType()) {</div><div>+ store = InitializeArrayWithInitList(store, T, CLR, ILE, state);</div><div>+ return store;</div><div>+ }</div><div>+</div><div>+ if (T->isStructureType()) {</div><div>+ store = InitializeStructWithInitList(store, T, CLR, ILE, state);</div><div>+ return store;</div><div>+ }</div><div>+</div><div>+ assert(0 && "unprocessed CompoundLiteral type");</div><div>+ return store;</div><div>+}</div><div>+</div><div><br></div><div><div>All of this can easily get hoisted into GRExprEngine.</div><div> </div></div><div>+SVal RegionStoreManager::getLValueCompoundLiteral(const GRState* St,</div><div>+ const CompoundLiteralExpr* CL){</div><div>+ return loc::MemRegionVal(MRMgr.getCompoundLiteralRegion(CL));</div><div>+}</div><div>+</div><div><br></div><div>This method has the same implementation as the one in BasicStoreManager. Let's just move it to StoreManager.</div><div><br></div><div><br></div><div>+Store RegionStoreManager::InitializeArrayWithInitList(Store store, </div><div>+ QualType T,</div><div>+ MemRegion* BaseR,</div><div>+ InitListExpr* ILE,</div><div>+ const GRState* state) {</div><div>+ assert(T->isArrayType());</div><div>+ assert(BaseR);</div><div>+ assert(ILE);</div><div>+</div><div>+ bool isGlobal=MRMgr.isGlobalsRegion(cast<SubRegion>(BaseR)->getSuperRegion());</div><div>+</div><div>+ BasicValueFactory& BasicVals = StateMgr.getBasicVals();</div><div>+</div><div>+ if (ConstantArrayType* CAT = dyn_cast<ConstantArrayType>(T.getTypePtr())) {</div><div>+ llvm::APInt Size = CAT->getSize();</div><div>+ llvm::APInt i = llvm::APInt::getNullValue(Size.getBitWidth());</div><div>+</div><div>+ InitListExpr::child_iterator I = ILE->child_begin(), E = ILE->child_end();</div><div>+</div><div>+ for (; i != Size; ++i) {</div><div>+ // Get the unique SVal.</div><div>+ nonloc::ConcreteInt Idx(BasicVals.getValue(llvm::APSInt(i)));</div><div>+ ElementRegion* ER = MRMgr.getElementRegion(Idx, BaseR);</div><div>+</div><div>+ if (I != E) {</div><div>+ SVal V = StateMgr.GetSVal(state, cast<Expr>(*I));</div><div>+ store = Bind(store, loc::MemRegionVal(ER), V);</div><div>+ ++I;</div><div>+</div><div>+ } else {</div><div>+ if (isGlobal) {</div><div>+ llvm::APInt ZeroInt = llvm::APInt::getNullValue(Size.getBitWidth());</div><div>+ // Get the unique SVal.</div><div>+ nonloc::ConcreteInt ZeroVal(BasicVals.getValue(llvm::APSInt(ZeroInt)));</div><div>+ store = Bind(store, loc::MemRegionVal(ER), ZeroVal);</div><div>+ } else</div><div>+ store = Bind(store, loc::MemRegionVal(ER), UndefinedVal());</div><div>+ }</div><div>+</div><div><br></div><div>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.</div><div><br></div><div><br></div><div> Store RegionStoreManager::InitializeStructToUndefined(Store store, QualType T,</div><div> MemRegion* BaseR) {</div><div> QualType CT = StateMgr.getContext().getCanonicalType(T);</div><div>@@ -412,3 +520,38 @@</div><div> </div><div> return store;</div><div> }</div><div>+</div><div>+Store RegionStoreManager::InitializeStructWithInitList(Store store,</div><div>+ QualType T,</div><div>+ MemRegion* BaseR,</div><div>+ InitListExpr* ILE,</div><div>+ const GRState* state) {</div><div>+ QualType CT = StateMgr.getContext().getCanonicalType(T);</div><div>+ const RecordType* RT = cast<RecordType>(CT.getTypePtr());</div><div>+ RecordDecl* RD = RT->getDecl();</div><div>+ assert(RD->isDefinition());</div><div>+</div><div>+ RecordDecl::field_iterator FI = RD->field_begin(), FE = RD->field_end();</div><div>+ InitListExpr::child_iterator EI = ILE->child_begin(), EE = ILE->child_end();</div><div>+</div><div>+ for (; FI != FE; ++FI, ++EI) {</div><div>+</div><div>+ QualType FTy = (*FI)->getType();</div><div>+ FieldRegion* FR = MRMgr.getFieldRegion(*FI, BaseR);</div><div>+</div><div>+ if (Loc::IsLocType(FTy) || FTy->isIntegerType()) {</div><div>+ SVal V = StateMgr.GetSVal(state, cast<Expr>(*EI));</div><div>+ store = Bind(store, loc::MemRegionVal(FR), V);</div><div>+</div><div>+ } else if (FTy->isArrayType()) {</div><div>+ store = InitializeArrayWithInitList(store, FTy, FR, </div><div>+ cast<InitListExpr>(*EI), state);</div><div>+</div><div>+ } else if (FTy->isStructureType()) {</div><div>+ store = InitializeStructWithInitList(store, FTy, FR, </div><div>+ cast<InitListExpr>(*EI), state);</div><div>+ }</div><div>+ }</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>Index: lib/Analysis/GRExprEngine.cpp</div><div>===================================================================</div><div>--- lib/Analysis/GRExprEngine.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 58306)</div><div>+++ lib/Analysis/GRExprEngine.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -1564,23 +1564,13 @@</div><div> Visit(ILE, Pred, Tmp);</div><div> </div><div> for (NodeSet::iterator I = Tmp.begin(), EI = Tmp.end(); I!=EI; ++I) {</div><div>- // Retrieve the initializer values from the environment and store them</div><div>- // into a vector that will then be handed off to the Store. </div><div>+</div><div> const GRState* St = GetState(*I); </div><div>- llvm::SmallVector<SVal, 10> IVals;</div><div>- IVals.reserve(ILE->getNumInits());</div><div>- </div><div>- for (Stmt::child_iterator J=ILE->child_begin(), EJ=ILE->child_end();</div><div>- J!=EJ; ++J)</div><div>- IVals.push_back(GetSVal(St, cast<Expr>(*J)));</div><div> </div><div>- const CompoundLiteralRegion* R = </div><div>- StateMgr.getRegionManager().getCompoundLiteralRegion(CL);</div><div>- </div><div>- assert (!IVals.empty() && "Initializer cannot be empty.");</div><div>+ St = StateMgr.AddCompoundLiteral(St, CL);</div><div> </div><div>- St = StateMgr.BindCompoundLiteral(St, R, &IVals[0], &IVals[0]+IVals.size());</div><div>- MakeNode(Dst, CL, *I, SetSVal(St, CL, loc::MemRegionVal(R)));</div><div>+ // FIXME: CompoundLiteralExpr can be used in both lvalue and rvalue context.</div><div>+ MakeNode(Dst, CL, *I, SetSVal(St, CL, StateMgr.GetLValue(St, CL)));</div><div> }</div><div> }</div><div> </div><div>I really think that all of the syntax processing should go here.</div></div><div><br></div><div><br></div></body></html>