<br><br><div class="gmail_quote">On Fri, Jan 9, 2009 at 9:02 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.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;">
<div class="Ih2E3d"><br>
On Jan 8, 2009, at 9:21 PM, Zhongxing Xu wrote:<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
This patch adds KillStruct() to region store. When we assign UnknownVal to a struct, we set the region's default value to Unknown, and remove bindings for all subregions of the struct region.<br>
</blockquote>
<br></div>
Hi Zhongxing,<br>
<br>
This is nice. Could you include a test case that would trigger KillStruct? More comments below:<br>
<br>
<br>
Index: lib/Analysis/RegionStore.cpp<br>
===================================================================<br>
--- lib/Analysis/RegionStore.cpp （版本 61900）<br>
+++ lib/Analysis/RegionStore.cpp （工作副本）<br>
@@ -234,6 +234,9 @@<br>
<br>
const GRState* BindStruct(const GRState* St, const TypedRegion* R, SVal V);<br>
<br>
+ /// KillStruct - Set the entire struct to unknown.<br>
+ const GRState* KillStruct(const GRState* St, const TypedRegion* R);<br>
+<br>
// Utility methods.<br>
BasicValueFactory& getBasicVals() { return StateMgr.getBasicVals(); }<br>
ASTContext& getContext() { return StateMgr.getContext(); }<br>
@@ -910,6 +913,9 @@<br>
RecordDecl* RD = RT->getDecl();<br>
assert(RD->isDefinition());<br>
<br>
+ if (V.isUnknown())<br>
+ return KillStruct(St, R);<br>
+<br>
<br>
When precisely can V.isUnknown() be true when the value is a struct? (test case please).</blockquote><div><br>We already have that test case. It is the f5() in test/Analysis/array-struct.c.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
<br>
+const GRState* RegionStoreManager::KillStruct(const GRState* St,<br>
+ const TypedRegion* R){<br>
+ GRStateRef state(St, StateMgr);<br>
+ // Set the default value of the struct region to UnknownVal.<br>
+ St = state.set<RegionDefaultValue>(R, UnknownVal());<br>
<br>
Interesting. Do we need the killset anymore, or are we going to model "killed values" in this way?</blockquote><div><br>I think we still need killset. The regions in killset are ones we query its binding directly. The regions in RegionDefaultValue are ones whose subregions we query bindings for. I haven't had a way to combine them.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
+<br>
+ Store store = St->getStore();<br>
+ RegionBindingsTy B = GetRegionBindings(store);<br>
+<br>
+ std::vector<const MemRegion*> ToBeKilled;<br>
<br>
In general, please use a llvm::SmallVector<> so that we can optimize for the case when the number of items fits on the stack. Using std::vector forces us to malloc() memory every time this method is called (which is slow). My comment below, however, illustrates why we don't need to use either in this case.</blockquote>
<div><br>OK.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
+<br>
+ // Remove all bindings for the subregions of the struct.<br>
+ for (RegionBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {<br>
+ const MemRegion* r = I.getKey();<br>
+ if (const SubRegion* sr = dyn_cast<SubRegion>(r))<br>
+ if (sr->isSubRegionOf(R))<br>
+ ToBeKilled.push_back(R);<br>
<br>
I think the region to be killed is 'r', not 'R' (since we just bound a value to it up above).</blockquote><div><br>Yes, my mistake (typo).<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
+ }<br>
+<br>
+ for (std::vector<const MemRegion*>::iterator I = ToBeKilled.begin(),<br>
+ E = ToBeKilled.end(); I != E; ++I)<br>
+ store = Remove(store, Loc::MakeVal(*I));<br>
<br>
You don't need to build a vector of killed regions and then iterate through vector because 'B' is a purely functional data structure. That is, removing entries from store just creates a new store, and 'B' will continue have the same values and the iterator won't be invalidated. The following will do just fine:<br>
<br>
for (RegionBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {<br>
const MemRegion* r = I.getKey();<br>
if (const SubRegion* sr = dyn_cast<SubRegion>(r))<br>
if (sr->isSubRegionOf(R))<br>
store = Remove(store, Loc::MakeVal(r));<br>
<br>
+ return StateMgr.MakeStateWithStore(St, store);</blockquote><div><br>Ah, I forgot that point. Is this also known as persistent data structure?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
Do we also need to remove region views for the regions that are killed, or should we dispense with that GDM entry entirely?</blockquote><div><br>I think region views are orthogonal to this one. Region views are used to cast region back and forth between typed and untyped ones. Here we only handle the region bindings.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
+}<br>
+<br>
<br>
</blockquote></div><br>