comments

Author: john-h-kastner

clang/include/clang/3C/AVarBoundsInfo.h

@@ -395,7 +395,7 @@ class AVarBoundsInfo {
   CtxSensitiveBoundsKeyHandler CSBKeyHandler;
 
   AVarGraph LowerBoundGraph;
-  static const BoundsKey InvalidLowerBoundKey = 0;
+  const BoundsKey InvalidLowerBoundKey = 0;
 
   // BoundsKeys that that cannot be used as a lower bound. These are used in an
   // update such as `a = a + 1`, or are transitively assigned from such a

clang/lib/3C/AVarBoundsInfo.cpp

@@ -522,84 +522,173 @@ bool AvarBoundsInference::inferBounds(BoundsKey K, const AVarGraph &BKGraph,
   return IsChanged;
 }
 
+
+void AVarBoundsInfo::computeInvalidLowerBounds() {
+  // This will compute a breadth first search starting from the constant
+  // InvalidLowerBoundKey. Any reachable keys are also invalid lower bounds.
+  // This is essentially the same algorithm as is used for solving the checked
+  // type constraint graph.
+  assert(InvalidLowerBounds.empty());
+  std::queue<BoundsKey> WorkList;
+  WorkList.push(InvalidLowerBoundKey);
+
+  // To check if a bounds key is already invalidated we check if it is either in
+  // the set of invalidated keys, or is the constant InvalidLowerBoundKey.
+  auto IsInvalidated = [this](BoundsKey BK) {
+    return BK == InvalidLowerBoundKey ||
+           InvalidLowerBounds.find(BK) != InvalidLowerBounds.end();
+  };
+
+  while (!WorkList.empty()) {
+    BoundsKey Curr = WorkList.front();
+    WorkList.pop();
+    assert(IsInvalidated(Curr));
+
+    std::set<BoundsKey> Neighbors;
+    LowerBoundGraph.getSuccessors(Curr, Neighbors);
+    for (BoundsKey NK : Neighbors) {
+      // The neighbors of an invalid lower bound are also invalid, with the
+      // exception that if there is a bound in the source code, then we assume
+      // the bound is correct, and so the pointer is a valid lower bound for
+      // itself.
+      bool HasDeclaredBounds =
+        getBounds(NK, BoundsPriority::Declared) != nullptr;
+      if (!HasDeclaredBounds && !IsInvalidated(NK)) {
+        InvalidLowerBounds.insert(NK);
+        WorkList.push(NK);
+      }
+    }
+  }
+}
+
 void
 AVarBoundsInfo::inferLowerBounds(ProgramInfo *PI) {
   computeInvalidLowerBounds();
 
+  // This maps array pointers to a single consistent lower bound pointer, or
+  // possible the constant InvalidLowerBoundKey if no lower bound could be found
+  // or generated. Note that there can only be a single valid lower bound.
+  // Future work can extend this map to track sets if possible lower bounds.
   std::map<BoundsKey, BoundsKey> InfLBs;
-  std::deque<BoundsKey> WorkList;
+
+  // Lower bound inference will proceed as a traversal of the LowerBoundGraph.
+  // The traversal starts at the direct predecessors of the pointers that need
+  // an inferred lower bound.
+  std::queue<BoundsKey> WorkList;
   for (BoundsKey BK : InvalidLowerBounds) {
     std::set<BoundsKey> Pred;
     LowerBoundGraph.getPredecessors(BK, Pred);
     for (BoundsKey Seed : Pred) {
-      if (Seed != 0 &&
+      if (Seed != InvalidLowerBoundKey &&
           InvalidLowerBounds.find(Seed) == InvalidLowerBounds.end()) {
+        // This pointer is a valid lower bound for itself, so add it to the
+        // worklist and initialize it in the map of inferred lower bounds.
         InfLBs[Seed] = Seed;
-        WorkList.push_back(Seed);
+        WorkList.push(Seed);
       }
     }
   }
 
-  // These must come after the valid Lower Bounds in the worklist, so this loop
-  // needs to be separate.
+  // It's possible for there to be invalid lower bounds that are not reachable
+  // from any valid lower bounds, so we also initialize the worklist with all
+  // invalid lower bounds. These come after the valid lower bounds so that is
+  // less likely a fresh lower bound will be generated but later thrown out, as
+  // that process is inefficient at least in the current implementation.
   for (BoundsKey InvLB : InvalidLowerBounds)
-    WorkList.push_back(InvLB);
+    WorkList.push(InvLB);
 
+  // This set tracks the pointers for which we will need to generate a fresh
+  // lower bound pointer. These pointers do not have a single consistent lower
+  // bound in the source code, but 3C is able to insert a duplicate declaration
+  // to act as the lower bound.
   std::set<BoundsKey> NeedFreshLB;
+
+  // This set track the pointers that have multiple inconsistent lower bounds.
+  // This is used to differentiate pointers that need a fresh lower bound
+  // because no lower bounds could be found from pointers that need a fresh
+  // lower bound because there were multiple inconsistent lower bounds. Note
+  // that is not a subset of NeedFreshLB because a pointer may have conflicting
+  // bounds but be ineligible for a fresh lower bound.
   std::set<BoundsKey> HasConflictingBounds;
+
   while (!WorkList.empty()) {
     BoundsKey BK = WorkList.front();
-    WorkList.pop_front();
+    WorkList.pop();
 
     if (isEligibleForFreshLowerBound(BK) &&
-        (InfLBs.find(BK) == InfLBs.end() || InfLBs[BK] == 0)) {
-      // We've reached an array pointer in the work list that either has not been assigned a lower bound, or
-      assert(InvalidLowerBounds.find(BK) != InvalidLowerBounds.end());
+        (InfLBs.find(BK) == InfLBs.end() || InfLBs[BK] == InvalidLowerBoundKey)) {
+      // We've reached an array pointer in the work list that either has not
+      // been assigned a lower bound, or has multiple conflicting lower bounds.
+      // We will generate a fresh lower bound.
+      assert(
+        "Generating fresh bound for pointer that can be its own lower bound." &&
+        InvalidLowerBounds.find(BK) != InvalidLowerBounds.end());
       InfLBs[BK] = getFreshLowerBound(BK);
       NeedFreshLB.insert(BK);
     }
 
     std::set<BoundsKey> Succ;
     LowerBoundGraph.getSuccessors(BK, Succ);
     for (BoundsKey S : Succ) {
-      if (InvalidLowerBounds.find(S) != InvalidLowerBounds.end()) {
-        if (InfLBs.find(S) == InfLBs.end()) {
-          // No prior lower bound known for `S`. Initialize it to use the same
-          // lower bound as `BK`.
-          if (isInAccessibleScope(S, InfLBs[BK])) {
-            InfLBs[S] = InfLBs[BK];
-          } else {
-            InfLBs[S] = 0;
-          }
-          WorkList.push_front(S);
-        } else if (InfLBs[BK] != InfLBs[S] &&
-                   HasConflictingBounds.find(S) == HasConflictingBounds.end()) {
-          HasConflictingBounds.insert(S);
-          if (NeedFreshLB.find(S) != NeedFreshLB.end() &&
-              isInAccessibleScope(S, InfLBs[BK])) {
-            NeedFreshLB.erase(S);
-            BoundsKey SLB = InfLBs[S];
-            LowerBoundGraph.visitBreadthFirst(S, [this, SLB, &InfLBs, &WorkList](BoundsKey BK) {
-              if (InfLBs.find(BK) != InfLBs.end() && InfLBs[BK] == SLB) {
-                InfLBs.erase(BK);
-                std::set<BoundsKey> Pred;
-                LowerBoundGraph.getPredecessors(BK, Pred);
-                for (BoundsKey P : Pred) {
-                  if (P != 0 && InfLBs.find(P) != InfLBs.end())
-                    WorkList.push_back(P);
-                }
+
+      // Do not process any array pointers that are valid lower bounds. They
+      // should just serve as their own lower bound.
+      if (InvalidLowerBounds.find(S) == InvalidLowerBounds.end())
+        continue;
+
+      if (InfLBs.find(S) == InfLBs.end()) {
+        // No prior lower bound known for `S`. Initialize it to use the same
+        // lower bound as `BK`, if this is possible given their scopes.
+        if (isInAccessibleScope(S, InfLBs[BK])) {
+          InfLBs[S] = InfLBs[BK];
+        } else {
+          InfLBs[S] = InvalidLowerBoundKey;
+        }
+        WorkList.push(S);
+      } else if (InfLBs[BK] != InfLBs[S] &&
+                 HasConflictingBounds.find(S) == HasConflictingBounds.end()) {
+        // The lower bound of `BK` is not the same as the current inferred lower
+        // bounds of `S`. This is a problem, so we need to mark `S` as having
+        // conflicting lower bounds. We only do this invalidation step once. If
+        // the BoundsKey is already known to have conflicting bounds, then do
+        // not reset it again. Doing so can cause an infinite loop when there is
+        // a cycle of BoundsKeys needing a lower bound, where each BoundsKey in
+        // the cycle is eligible for a fresh lower bound.
+        HasConflictingBounds.insert(S);
+
+        if (NeedFreshLB.find(S) != NeedFreshLB.end() ) {
+          // This case handles when we a fresh lower bounds was created for `S`
+          // before any conflict was detected. It is possible that the conflict
+          // we detect here only exists between the fresh lower bound and the
+          // lower bound of `BK`. In this case, we can drop the fresh lower
+          // bounds to use the inferred lower bound. In order to fully drop the
+          // fresh bound, we must also drop it from all BoundsKey reachable from
+          // `S`, as these may have already had a lower bound inferred based on
+          // the fresh lower bound.
+          NeedFreshLB.erase(S);
+          BoundsKey SLB = InfLBs[S];
+          // TODO: This is inefficient.
+          LowerBoundGraph.visitBreadthFirst(S, [this, SLB, &InfLBs, &WorkList](BoundsKey BK) {
+            if (InfLBs.find(BK) != InfLBs.end() && InfLBs[BK] == SLB) {
+              InfLBs.erase(BK);
+              std::set<BoundsKey> Pred;
+              LowerBoundGraph.getPredecessors(BK, Pred);
+              for (BoundsKey P : Pred) {
+                if (P != InvalidLowerBoundKey &&
+                    InfLBs.find(P) != InfLBs.end())
+                  WorkList.push(P);
               }
-            });
-          } else if (InfLBs[S] != 0) {
-            // If there is and existing lower bounds, it must be the same lower
-            // bound as `BK`. If it's not, invalidate the lower bound for `S`.
-            InfLBs[S] = 0;
-            WorkList.push_back(S);
-          }
+            }
+          });
+        } else if (InfLBs[S] != InvalidLowerBoundKey) {
+          // If no fresh lower bound was generated, then things are much
+          // simpler. Just invalidate the lower bound of `S` and enqueue it.
+          InfLBs[S] = InvalidLowerBoundKey;
+          WorkList.push(S);
         }
-        // Otherwise, the a lower bound exists for `S`, and it's the same lower
-        // bounds as `BK`. Nothing changes, so don't enqueue `S`.
       }
+      // Otherwise, the a lower bound exists for `S`, and it's the same lower
+      // bounds as `BK`. Nothing changes, so don't enqueue `S`.
     }
   }
 
@@ -619,7 +708,8 @@ BoundsKey AVarBoundsInfo::getFreshLowerBound(BoundsKey Arr) {
 
 bool AVarBoundsInfo::hasLowerBound(BoundsKey K)  {
   return InvalidLowerBounds.find(K) == InvalidLowerBounds.end() ||
-         (LowerBounds.find(K) != LowerBounds.end() && LowerBounds[K] != 0);
+         (LowerBounds.find(K) != LowerBounds.end() &&
+          LowerBounds[K] != InvalidLowerBoundKey);
 }
 
 bool AvarBoundsInference::inferFromPotentialBounds(BoundsKey BK,
@@ -802,7 +892,8 @@ bool AVarBoundsInfo::tryGetVariable(clang::Expr *E, const ASTContext &C,
 // Merging bounds B with the present bounds of key L at the same priority P
 // Returns true if we update the bounds for L (with B)
 bool AVarBoundsInfo::mergeBounds(BoundsKey L, BoundsPriority P, ABounds *B) {
-  if (B->getLowerBoundKey() == 0 && LowerBounds.find(L) != LowerBounds.end())
+  if (B->getLowerBoundKey() == InvalidLowerBoundKey &&
+      LowerBounds.find(L) != LowerBounds.end())
     B->setLowerBoundKey(LowerBounds[L]);
   bool RetVal = false;
   if (BInfo.find(L) != BInfo.end() && BInfo[L].find(P) != BInfo[L].end()) {
@@ -1468,32 +1559,6 @@ void AVarBoundsInfo::performFlowAnalysis(ProgramInfo *PI) {
   PStats.endArrayBoundsInferenceTime();
 }
 
-void AVarBoundsInfo::computeInvalidLowerBounds() {
-  assert(InvalidLowerBounds.empty());
-  std::queue<BoundsKey> WorkList;
-  WorkList.push(0);
-
-  auto IsInvalidated = [this](BoundsKey BK) {
-    return BK == 0 || InvalidLowerBounds.find(BK) != InvalidLowerBounds.end();
-  };
-
-  while (!WorkList.empty()) {
-    BoundsKey Curr = WorkList.front();
-    WorkList.pop();
-    assert(IsInvalidated(Curr));
-
-    std::set<BoundsKey> Neighbors;
-    LowerBoundGraph.getSuccessors(Curr, Neighbors);
-    for (BoundsKey NK : Neighbors) {
-      bool HasDeclaredBounds =
-        getBounds(NK, BoundsPriority::Declared) != nullptr;
-      if (!HasDeclaredBounds && !IsInvalidated(NK)) {
-        InvalidLowerBounds.insert(NK);
-        WorkList.push(NK);
-      }
-    }
-  }
-}
 
 bool AVarBoundsInfo::keepHighestPriorityBounds() {
   bool HasChanged = false;