[cfe-commits] [patch] Subregion map

[Zhongxing Xu]

2009/3/17 Ted Kremenek <kremenek at apple.com>

>
> On Mar 16, 2009, at 7:39 PM, Zhongxing Xu wrote:
>
>  When we setup the subregion map, we should consider nested subregions.
>> Consider the code:
>>
>> struct s1 {
>>  struct s2 {
>>    int d;
>>  } e;
>> };
>>
>> void f(double timeout) {
>>  struct s1 a;
>>  a.e.d = (long) timeout;
>>  if (a.e.d == 10)
>>    a.e.d = 4;
>> }
>>
>> 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
>> intermediate region roots which are subregions themselves.
>>
>
> Hi Zhongxing,
>
> Looks good.  One comment.  Your patch expands to:
>
>  while (!IntermediateRoots.empty()) {
>    const MemRegion* R = IntermediateRoots.back();
>    IntermediateRoots.pop_back();
>
>    if (const VarRegion* VR = dyn_cast<VarRegion>(R)) {
>      if (SymReaper.isLive(Loc, VR->getDecl()))
>        RegionRoots.push_back(VR); // This is a live "root".
>    }
>    else {
>      // Get the super region for R.
>      const MemRegion* SuperR = cast<SubRegion>(R)->getSuperRegion();
>      // Get the current set of subregions for SuperR.
>      const SubRegionsTy* SRptr = SubRegMap.lookup(SuperR);
>      SubRegionsTy SR = SRptr ? *SRptr : SubRegF.GetEmptySet();
>      // Add R to the subregions of SuperR.
>      SubRegMap = SubRegMapF.Add(SubRegMap, SuperR, SubRegF.Add(SR, R));
>
>      // Finally, check if SuperR is a VarRegion.  We need to do this
>      // to also mark SuperR as a root (as it may not have a value directly
>      // bound to it in the store).
>      if (const VarRegion* VR = dyn_cast<VarRegion>(SuperR)) {
>        if (SymReaper.isLive(Loc, VR->getDecl()))
>          RegionRoots.push_back(VR); // This is a live "root".
>      } else if (isa<SubRegion>(SuperR)) {
>        // We need to setup the subregion map of the super region of the
> SuperR.
>        IntermediateRoots.push_back(SuperR);
>      }
>    }
>  }
>
> With the worklist you added it seems like this code can now be simplified:
>
>  while (!IntermediateRoots.empty()) {
>    const MemRegion* R = IntermediateRoots.back();
>    IntermediateRoots.pop_back();
>
>    if (const VarRegion* VR = dyn_cast<VarRegion>(R)) {
>      if (SymReaper.isLive(Loc, VR->getDecl()))
>        RegionRoots.push_back(VR); // This is a live "root".
>    }
>    else {
>      // Get the super region for R.
>      const MemRegion* SuperR = cast<SubRegion>(R)->getSuperRegion();
>      // Get the current set of subregions for SuperR.
>      const SubRegionsTy* SRptr = SubRegMap.lookup(SuperR);
>      SubRegionsTy SR = SRptr ? *SRptr : SubRegF.GetEmptySet();
>      // Add R to the subregions of SuperR.
>      SubRegMap = SubRegMapF.Add(SubRegMap, SuperR, SubRegF.Add(SR, R));
>      // Handle super region.
>      if (isa<SubRegion>(SuperR) || isa<VarRegion>(VR))
>        ImmediateRoots.push_back(SuperR);
>    }
>  }
>
> 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.
>

Nice! Applied. The logic can be further simplified since VarRegion isa
SubRegion.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090318/d67c3d7d/attachment.html>