<br><br><div class="gmail_quote">2009/3/17 Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="h5"><br>
On Mar 16, 2009, at 7:39 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;">
When we setup the subregion map, we should consider nested subregions. Consider the code:<br>
<br>
struct s1 {<br>
  struct s2 {<br>
    int d;<br>
  } e;<br>
};<br>
<br>
void f(double timeout) {<br>
  struct s1 a;<br>
  a.e.d = (long) timeout;<br>
  if (a.e.d == 10)<br>
    a.e.d = 4;<br>
}<br>
<br>
Current implementation only setup subregion map from 'e' to 'd', but no map from 'a' to 'e'. So we lost the binding for a.e.d. The attached patch uses a worklist to record<br>
intermediate region roots which are subregions themselves.<br>
</blockquote>
<br></div></div>
Hi Zhongxing,<br>
<br>
Looks good.  One comment.  Your patch expands to:<br>
<br>
  while (!IntermediateRoots.empty()) {<br>
    const MemRegion* R = IntermediateRoots.back();<br>
    IntermediateRoots.pop_back();<br>
<br>
    if (const VarRegion* VR = dyn_cast<VarRegion>(R)) {<br>
      if (SymReaper.isLive(Loc, VR->getDecl()))<br>
        RegionRoots.push_back(VR); // This is a live "root".<br>
    }<br>
    else {<br>
      // Get the super region for R.<br>
      const MemRegion* SuperR = cast<SubRegion>(R)->getSuperRegion();<br>
      // Get the current set of subregions for SuperR.<br>
      const SubRegionsTy* SRptr = SubRegMap.lookup(SuperR);<br>
      SubRegionsTy SR = SRptr ? *SRptr : SubRegF.GetEmptySet();<br>
      // Add R to the subregions of SuperR.<br>
      SubRegMap = SubRegMapF.Add(SubRegMap, SuperR, SubRegF.Add(SR, R));<br>
<br>
      // Finally, check if SuperR is a VarRegion.  We need to do this<br>
      // to also mark SuperR as a root (as it may not have a value directly<br>
      // bound to it in the store).<br>
      if (const VarRegion* VR = dyn_cast<VarRegion>(SuperR)) {<br>
        if (SymReaper.isLive(Loc, VR->getDecl()))<br>
          RegionRoots.push_back(VR); // This is a live "root".<br>
      } else if (isa<SubRegion>(SuperR)) {<br>
        // We need to setup the subregion map of the super region of the SuperR.<br>
        IntermediateRoots.push_back(SuperR);<br>
      }<br>
    }<br>
  }<br>
<br>
With the worklist you added it seems like this code can now be simplified:<br>
<br>
  while (!IntermediateRoots.empty()) {<br>
    const MemRegion* R = IntermediateRoots.back();<br>
    IntermediateRoots.pop_back();<br>
<br>
    if (const VarRegion* VR = dyn_cast<VarRegion>(R)) {<br>
      if (SymReaper.isLive(Loc, VR->getDecl()))<br>
        RegionRoots.push_back(VR); // This is a live "root".<br>
    }<br>
    else {<br>
      // Get the super region for R.<br>
      const MemRegion* SuperR = cast<SubRegion>(R)->getSuperRegion();<br>
      // Get the current set of subregions for SuperR.<br>
      const SubRegionsTy* SRptr = SubRegMap.lookup(SuperR);<br>
      SubRegionsTy SR = SRptr ? *SRptr : SubRegF.GetEmptySet();<br>
      // Add R to the subregions of SuperR.<br>
      SubRegMap = SubRegMapF.Add(SubRegMap, SuperR, SubRegF.Add(SR, R));<br>
      // Handle super region.<br>
      if (isa<SubRegion>(SuperR) || isa<VarRegion>(VR))<br>
        ImmediateRoots.push_back(SuperR);<br>
    }<br>
  }<br>
<br>
Here I have simplified the "recursive" case in the worklist by enqueuing both VarRegions and SubRegions to the worklist in the "else" block.  This is a little nicer since the logic for handling VarRegions (a base case) is now handled in one place.<br>

</blockquote></div><br>Nice! Applied. The logic can be further simplified since VarRegion isa SubRegion.<br>