[cfe-commits] [patch] Subregion map

[Ted Kremenek]

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.