Collection of code cleanup changes made to bounds inference (#650)

* Cleanup ScopeVisitor:
  - Better naming for fields
  - Make references to AVarBoundsInfo fields constant
  - Expose result of visitor through constant set reference instead of by
    mutating sets passed as arguments.
  - Remove loop of singleton set.

* Cleanup getReachableBoundKeys
  - Rename SBVar -> FromProgramVar
  - Get rid of unnecessary set in constant finding loop

* Cleanup StructAccessVisitor
  - Make two fields private. One of these was never used publicly, the
    other is exposed through a getter.
  - Slightly simpler logic for handling variable declarations.
    `isLocalVarDecl` returns false for ParmVarDecls, so it's OK to update
    IsGlobal with the result of this function for ParmVarDecl and
    VarDecl.
  - Added note about a bug on structure access such as
    `( 0 ? global_struct_var : local_struct_var).array`.

* Cleanup CollectDeclsVisitor
  - Make set fields private and expose through const reference.
  - Update comments

* Make some methods of ConstraintsGraph const
  - getNeighbors, getSuccessors, getPredecessors, and findNode are
    trivially const.
  - visitBreadthFirst mutates the BFSCache map, but this acceptable
    because it is cache for the actual graph data, and the underlying
    data is not mutated. To let this method be checked `BFSCache` is
    explicitly qualified with mutable.

* Cleanup performFlowAnalysis
  - Avoid re-using AvarBoundsInference instance.
  - Avoid re-using set in inner most inference loop
  - Move some code into performWorkListInference since it was always
    called before/after this function.

* Cleanup performWorkListInference
  - Remove `Changed` loop variable and instead terminate outer loop when
    `WorkList` is empty.
  - Replace inner while loop with a for loop to make it clearer that it
    the loop is just iterating over each element of `WorkList`.

* Cleanup predictBounds
  - Move variable declarations closer to use.
  - Declare variables with explicit types where this helps me understand
    the code.
  - Add some comments.

* Cleanup inferFromPotentialBounds
  - rename Handled to HasInferredBound
  - Use llvm::any_of instead of loop

* Cleanup computeArrPointers
  - Avoid duplicating code in main for loop
  - Add comment expressing concern about special-casing on return-type
    null terminated arrays.
  - Delete some intermediate sets that didn't need to exist.

* Delete commented out method tryGetBoundsKeyVar
  - A nearly identical un-commented version of this method still exists. The
    only difference is that getVariable is used to get a ConstraintVariable
    instead of getExprConstraintVars.

* Cleanup handleAllocatorCall
  - remove unused Fname parameter from isAllocatorCall
  - getCalledFunctionName can only be called on CallExprs, so the type of
    its parameter can be changed from Expr to CallExpr.
  - Introduce function hasValidBoundsKey that can be used instead of
    calling tryGetValidBoundsKey with unused BoundsKey reference.

* Cleanup LocalVarABVisitor
  - Rename addUsedParmVarDecl -> addNonLengthParameter
  - Delete some unused local variables
  - Add a comment about some odd code

* Cleanup ComparisonVisitor
  - Fix typo in class name
  - Remove unused field
  - Expose result through const reference to field instead of mutating
    constructor argument.

* Cleanup LengthVarInference
  - Remove unused fields
  - Avoid needing to `new` and `delete` some fields.

* Add some array bounds debugging utilities
  - Dump context sensitive and reverse context sensitive graphs in
    addition to standard graph.
  - Add dumpBounds method to write current bounds solution state to
    standard error. This can be called from the debugger while stepping
    through execution.

* ProgramVar cleanup
  - Store constant value of variable explicitly
  - Assertions to check that non-constant variable is not used as
    constant

Author: john-h-kastner

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

@@ -106,31 +106,31 @@ class AvarBoundsInference {
 
   // Infer bounds for the given key from the set of given ARR atoms.
   // The flag FromPB requests the inference to use potential length variables.
-  bool inferBounds(BoundsKey K, AVarGraph &BKGraph, bool FromPB = false);
+  bool inferBounds(BoundsKey K, const AVarGraph &BKGraph, bool FromPB = false);
 
-  // Get a consistent bound for all the arrays whose bounds have been
-  // inferred.
-  bool convergeInferredBounds();
+  // Get a consistent bound for all the arrays whose bounds have been inferred.
+  void convergeInferredBounds();
 
 private:
   // Find all the reachable variables form FromVarK that are visible
   // in DstScope
   bool getReachableBoundKeys(const ProgramVarScope *DstScope,
                              BoundsKey FromVarK, std::set<BoundsKey> &PotK,
-                             AVarGraph &BKGraph, bool CheckImmediate = false);
+                             const AVarGraph &BKGraph,
+                             bool CheckImmediate = false);
 
   // Check if bounds specified by Bnds are declared bounds of K.
   bool areDeclaredBounds(
       BoundsKey K,
       const std::pair<ABounds::BoundsKind, std::set<BoundsKey>> &Bnds);
 
   // Get all the bounds of the given array i.e., BK
-  bool getRelevantBounds(BoundsKey BK, BndsKindMap &ResBounds);
+  void getRelevantBounds(BoundsKey BK, BndsKindMap &ResBounds);
 
   // Predict possible bounds for DstArrK from the bounds of  Neighbours.
   // Return true if there is any change in the captured bounds information.
-  bool predictBounds(BoundsKey DstArrK, std::set<BoundsKey> &Neighbours,
-                     AVarGraph &BKGraph);
+  bool predictBounds(BoundsKey DstArrK, const std::set<BoundsKey> &Neighbours,
+                     const AVarGraph &BKGraph);
 
   void mergeReachableProgramVars(BoundsKey TarBK, std::set<BoundsKey> &AllVars);
 
@@ -139,14 +139,16 @@ class AvarBoundsInference {
   // Set the given pointer to have impossible bounds.
   void setImpossibleBounds(BoundsKey BK);
   // Infer bounds of the given pointer key from potential bounds.
-  bool inferFromPotentialBounds(BoundsKey BK, AVarGraph &BKGraph);
+  bool inferFromPotentialBounds(BoundsKey BK, const AVarGraph &BKGraph);
 
   AVarBoundsInfo *BI;
 
   // Potential Bounds for each bounds key inferred for the current iteration.
   std::map<BoundsKey, BndsKindMap> CurrIterInferBounds;
   // BoundsKey that failed the flow inference.
   std::set<BoundsKey> BKsFailedFlowInference;
+
+  static ABounds *getPreferredBound(const BndsKindMap &BKindMap);
 };
 
 // Class that maintains information about potential bounds for
@@ -255,7 +257,7 @@ class AVarBoundsInfo {
   ProgramVar *getProgramVar(BoundsKey VK);
 
   // Propagate the array bounds information for all array ptrs.
-  bool performFlowAnalysis(ProgramInfo *PI);
+  void performFlowAnalysis(ProgramInfo *PI);
 
   // Get the context sensitive BoundsKey for the given key at CallSite
   // located at PSL.
@@ -366,23 +368,23 @@ class AVarBoundsInfo {
   bool isFunctionReturn(BoundsKey BK);
 
   // Of all the pointer bounds key, find arr pointers.
-  void computerArrPointers(ProgramInfo *PI, std::set<BoundsKey> &Ret);
+  void computeArrPointers(const ProgramInfo *PI);
 
   // Get all the array pointers that need bounds.
-  void getBoundsNeededArrPointers(const std::set<BoundsKey> &ArrPtrs,
-                                  std::set<BoundsKey> &AB);
+  void getBoundsNeededArrPointers(std::set<BoundsKey> &AB) const;
 
   // Keep only highest priority bounds for all the provided BoundsKeys
   // returns true if any thing changed, else false.
-  bool keepHighestPriorityBounds(std::set<BoundsKey> &ArrPtrs);
+  bool keepHighestPriorityBounds();
 
   // Perform worklist based inference on the requested array variables using
   // the provided graph and potential length variables.
-  bool performWorkListInference(const std::set<BoundsKey> &ArrNeededBounds,
-                                AVarGraph &BKGraph, AvarBoundsInference &BI,
-                                bool FromPB);
+  void performWorkListInference(const AVarGraph &BKGraph,
+                                AvarBoundsInference &BI, bool FromPB);
 
   void insertParamKey(ParamDeclType ParamDecl, BoundsKey NK);
+
+  void dumpBounds();
 };
 
 #endif // LLVM_CLANG_3C_AVARBOUNDSINFO_H

clang/include/clang/3C/ArrayBoundsInferenceConsumer.h

@@ -69,11 +69,11 @@ class LocalVarABVisitor : public clang::RecursiveASTVisitor<LocalVarABVisitor> {
   bool VisitSwitchStmt(SwitchStmt *S);
   bool VisitBinaryOperator(BinaryOperator *O);
   bool VisitArraySubscriptExpr(ArraySubscriptExpr *E);
-  bool isNonLengthParameter(ParmVarDecl *PVD);
+  bool isNonLengthParameter(ParmVarDecl *PVD) const;
 
 private:
   void handleAssignment(BoundsKey LK, QualType LHSType, Expr *RHS);
-  void addUsedParmVarDecl(Expr *CE);
+  void addNonLengthParameter(Expr *CE);
   std::set<ParmVarDecl *> NonLengthParameters;
   ASTContext *Context;
   ProgramInfo &Info;
@@ -88,8 +88,6 @@ class LengthVarInference : public StmtVisitor<LengthVarInference> {
 public:
   LengthVarInference(ProgramInfo &In, ASTContext *AC, FunctionDecl *F);
 
-  virtual ~LengthVarInference();
-
   void VisitStmt(Stmt *St);
 
   void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE);
@@ -98,11 +96,9 @@ class LengthVarInference : public StmtVisitor<LengthVarInference> {
   std::map<const Stmt *, CFGBlock *> StMap;
   ProgramInfo &I;
   ASTContext *C;
-  FunctionDecl *FD;
   CFGBlock *CurBB;
-  ControlDependencyCalculator *CDG;
-  ConstraintResolver *CR;
   std::unique_ptr<CFG> Cfg;
+  ControlDependencyCalculator CDG;
 };
 
 void handleArrayVariablesBoundsDetection(ASTContext *C, ProgramInfo &I,

clang/include/clang/3C/ConstraintVariables.h

@@ -412,7 +412,7 @@ class PointerVariableConstraint : public ConstraintVariable {
   std::string getTy() const { return BaseType; }
   bool getArrPresent() const;
   // Check if the outermost pointer is an unsized array.
-  bool isTopCvarUnsizedArr() const;
+  bool isTopAtomUnsizedArr() const;
   // Check if any of the pointers is either a sized or unsized arr.
   bool hasSomeSizedArr() const;
 

clang/include/clang/3C/Constraints.h

@@ -488,8 +488,8 @@ class Constraints {
   NTArrAtom *getNTArr() const;
   WildAtom *getWild() const;
   ConstAtom *getAssignment(Atom *A);
-  ConstraintsGraph &getChkCG();
-  ConstraintsGraph &getPtrTypCG();
+  const ConstraintsGraph &getChkCG() const;
+  const ConstraintsGraph &getPtrTypCG() const;
 
   void resetEnvironment();
   bool checkInitialEnvSanity();

clang/include/clang/3C/ConstraintsGraph.h

@@ -153,7 +153,7 @@ class DataGraph
   }
 
   bool getNeighbors(Data D, std::set<Data> &DataSet, bool Succ,
-                    bool Append = false, bool IgnoreSoftEdges = false) {
+                    bool Append = false, bool IgnoreSoftEdges = false) const {
     NodeType *N = this->findNode(D);
     if (N == nullptr)
       return false;
@@ -170,21 +170,23 @@ class DataGraph
     return !DataSet.empty();
   }
 
-  bool getSuccessors(Data D, std::set<Data> &DataSet, bool Append = false) {
+  bool
+  getSuccessors(Data D, std::set<Data> &DataSet, bool Append = false) const {
     return getNeighbors(D, DataSet, true, Append);
   }
 
-  bool getPredecessors(Data D, std::set<Data> &DataSet, bool Append = false) {
+  bool
+  getPredecessors(Data D, std::set<Data> &DataSet, bool Append = false) const {
     return getNeighbors(D, DataSet, false, Append);
   }
 
-  NodeType *findNode(Data D) {
+  NodeType *findNode(Data D) const {
     if (NodeSet.find(D) != NodeSet.end())
-      return NodeSet[D];
+      return NodeSet.at(D);
     return nullptr;
   }
 
-  void visitBreadthFirst(Data Start, llvm::function_ref<void(Data)> Fn) {
+  void visitBreadthFirst(Data Start, llvm::function_ref<void(Data)> Fn) const {
     NodeType *N = this->findNode(Start);
     if (N == nullptr)
       return;
@@ -217,7 +219,7 @@ class DataGraph
 private:
   template <typename G> friend struct llvm::GraphTraits;
   friend class GraphVizOutputGraph;
-  std::map<Data, std::set<Data>> BFSCache;
+  mutable std::map<Data, std::set<Data>> BFSCache;
   std::map<Data, NodeType *> NodeSet;
 
   void invalidateBFSCache() { BFSCache.clear(); }

clang/include/clang/3C/ProgramInfo.h

@@ -134,6 +134,7 @@ class ProgramInfo : public ProgramVariableAdder {
 
   const VariableMap &getVarMap() const { return Variables; }
   Constraints &getConstraints() { return CS; }
+  const Constraints &getConstraints() const { return CS; }
   AVarBoundsInfo &getABoundsInfo() override { return ArrBInfo; }
 
   PerformanceStats &getPerfStats() { return PerfS; }

clang/include/clang/3C/ProgramVar.h

@@ -406,35 +406,55 @@ class FunctionScope : public ProgramVarScope {
 // Class that represents a program variable along with its scope.
 class ProgramVar {
 public:
-  const ProgramVarScope *getScope() { return VScope; }
+  const ProgramVarScope *getScope() const { return VScope; }
   void setScope(const ProgramVarScope *PVS) { this->VScope = PVS; }
-  BoundsKey getKey() { return K; }
-  bool isNumConstant() { return IsConstant; }
-  std::string mkString(bool GetKey = false);
-  std::string getVarName() { return VarName; }
-  std::string verboseStr();
-  ProgramVar *makeCopy(BoundsKey NK);
-  virtual ~ProgramVar() {}
+  BoundsKey getKey() const { return K; }
+  const std::string &getVarName() const { return VarName; }
+  std::string verboseStr() const;
+  ProgramVar *makeCopy(BoundsKey NK) const;
+
+  bool isNumConstant() const {return IsConstant; }
+  uint64_t getConstantVal() const {
+    assert("Can't get constant value for non-constant var." && IsConstant);
+    return ConstantVal;
+  }
 
   static ProgramVar *createNewProgramVar(BoundsKey VK, std::string VName,
-                                         const ProgramVarScope *PVS,
-                                         bool IsCons = false);
+                                         const ProgramVarScope *PVS);
+
+  static ProgramVar *createNewConstantVar(BoundsKey VK, uint64_t Value);
 
 private:
   BoundsKey K;
   std::string VarName;
   const ProgramVarScope *VScope;
-  bool IsConstant; // is a literal integer, not a variable
+
+  // Is a literal integer, not a variable.
+  bool IsConstant;
+  uint64_t ConstantVal;
+
   // TODO: All the ProgramVars may not be used. We should try to figure out
   //  a way to free unused program vars.
-  static std::set<ProgramVar *> AllProgramVars;
-
-  ProgramVar(BoundsKey VK, std::string VName, const ProgramVarScope *PVS,
-             bool IsCons)
-      : K(VK), VarName(VName), VScope(PVS), IsConstant(IsCons) {}
+  static std::set<const ProgramVar *> AllProgramVars;
+
+  ProgramVar(BoundsKey K, const std::string &VarName,
+             const ProgramVarScope *VScope, bool IsConstant,
+             uint32_t ConstantVal)
+    : K(K), VarName(VarName), VScope(VScope), IsConstant(IsConstant),
+      ConstantVal(ConstantVal) {
+    // Constant variables should be a subclass of ProgramVariable. Until that
+    // change happens this should sanity check how ProgramVars are constructed.
+    assert("Constant value should not be set for non-constant variables." &&
+           (IsConstant || ConstantVal == 0));
+    AllProgramVars.insert(this);
+  }
 
   ProgramVar(BoundsKey VK, std::string VName, const ProgramVarScope *PVS)
-      : ProgramVar(VK, VName, PVS, false) {}
+    : ProgramVar(VK, VName, PVS, false, 0) {}
+
+  ProgramVar(BoundsKey VK, uint32_t CVal)
+    : ProgramVar(VK, std::to_string(CVal), GlobalScope::getGlobalScope(), true,
+                 CVal) {}
 };
 
 #endif // LLVM_CLANG_3C_PROGRAMVAR_H

clang/lib/3C/ABounds.cpp

@@ -30,7 +30,7 @@ ABounds *ABounds::getBoundsInfo(AVarBoundsInfo *ABInfo, BoundsExpr *BExpr,
     if (ABInfo->tryGetVariable(CBE->getCountExpr()->IgnoreParenCasts(), C,
                                VK)) {
       ProgramVar *PV = ABInfo->getProgramVar(VK);
-      if (PV->isNumConstant() && PV->getVarName() == "0") {
+      if (PV->isNumConstant() && PV->getConstantVal() == 0) {
         // Invalid bounds. This is for functions like free.
         // Where the bounds is 0.
         Ret = nullptr;
@@ -60,7 +60,7 @@ std::string ABounds::getBoundsKeyStr(BoundsKey BK, AVarBoundsInfo *ABI,
                                      Decl *D) {
   ProgramVar *PV = ABI->getProgramVar(BK);
   assert(PV != nullptr && "No Valid program var");
-  std::string BKStr = PV->mkString();
+  std::string BKStr = PV->getVarName();
   unsigned PIdx = 0;
   auto *PVD = dyn_cast_or_null<ParmVarDecl>(D);
   // Does this belong to a function parameter?
@@ -74,7 +74,7 @@ std::string ABounds::getBoundsKeyStr(BoundsKey BK, AVarBoundsInfo *ABI,
       // If the parameter in the new declaration does not have a name?
       // then use the old name.
       if (BKStr.empty())
-        BKStr = PV->mkString();
+        BKStr = PV->getVarName();
     }
   }
   return BKStr;