Use same logic to check for pointer arithmetic in an assignment in array bounds inference and rewriter.

Author: john-h-kastner

clang/include/clang/3C/Utils.h

@@ -257,4 +257,9 @@ inline const clang::DiagnosticBuilder &operator<<(
   return DB;
 }
 
+// Return true if an assignment LHS=RHS would result in invalidating the bounds
+// of LHS by assigning to a pointer a value derived from the pointer, for
+// example, by using pointer arithmetic to compute a new pointer at some offset
+// from the original.
+bool isAssignmentPointerArithmetic(clang::Expr *LHS, clang::Expr *RHS);
 #endif

clang/lib/3C/AVarBoundsInfo.cpp

@@ -992,87 +992,10 @@ bool AVarBoundsInfo::addAssignment(BoundsKey L, BoundsKey R) {
   return true;
 }
 
-// Visitor to collect all the variables and structure member access that are
-// used during the life-time of the visitor.
-class CollectDeclsVisitor : public RecursiveASTVisitor<CollectDeclsVisitor> {
-public:
-  explicit CollectDeclsVisitor() : ObservedDecls(), ObservedStructAccesses() {}
-
-  virtual ~CollectDeclsVisitor() {}
-
-  bool VisitDeclRefExpr(DeclRefExpr *DRE) {
-    if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl()))
-      ObservedDecls.insert(VD);
-    return true;
-  }
-
-  // For a->b; We need to get `a->b` rather than just `b`. This way assignment
-  // from a field in one instance of a structure to the same field in another
-  // instance is not treated as pointer arithmetic.
-  bool VisitMemberExpr(MemberExpr *ME) {
-    // TODO: Is this cast legit? `getMemberDecl()` returns a `ValueDecl*`, but I
-    //       think it can only be a `FieldDecl` for structs in C.
-    auto *FD = cast<FieldDecl>(ME->getMemberDecl());
-
-    // TODO: Is recursively using a new visitor the right thing to do? It feels
-    //       odd.
-    CollectDeclsVisitor MEVis;
-    MEVis.TraverseStmt(ME->getBase());
-    // Field access through variable.
-    for (auto *D : MEVis.getObservedDecls()) {
-      std::vector<FieldDecl *> SingletonAccessList({FD});
-      ObservedStructAccesses.insert(std::make_pair(D, SingletonAccessList));
-    }
-    // Field access through other structure fields.
-    for (StructAccess SA : MEVis.getObservedStructAccesses()) {
-      SA.second.push_back(FD);
-      ObservedStructAccesses.insert(SA);
-    }
-    return false;
-  }
-
-  bool VisitCallExpr(CallExpr *CE) {
-    // Stop the visitor when we hit a CallExpr. This stops us from treating a
-    // function call like `a = foo(a);` the same as `a = a + 1`.
-    return false;
-  }
-
-  const std::set<VarDecl *> &getObservedDecls() { return ObservedDecls; }
-
-  typedef std::pair<VarDecl *, std::vector<FieldDecl *>> StructAccess;
-  const std::set<StructAccess> &getObservedStructAccesses() {
-    return ObservedStructAccesses;
-  }
-
-private:
-  // Contains all VarDecls seen by this visitor
-  std::set<VarDecl *> ObservedDecls;
-
-  // Contains the source representation of all record access (MemberExpression)
-  // seen by this visitor.
-  std::set<StructAccess> ObservedStructAccesses;
-};
-
 bool AVarBoundsInfo::handlePointerAssignment(clang::Expr *L, clang::Expr *R,
                                              ASTContext *C,
                                              ConstraintResolver *CR) {
-  CollectDeclsVisitor LVarVis;
-  LVarVis.TraverseStmt(L->getExprStmt());
-
-  CollectDeclsVisitor RVarVis;
-  RVarVis.TraverseStmt(R->getExprStmt());
-
-  std::set<VarDecl *> CommonVars;
-  std::set<CollectDeclsVisitor::StructAccess> CommonStVars;
-  findIntersection(LVarVis.getObservedDecls(), RVarVis.getObservedDecls(),
-                   CommonVars);
-  findIntersection(LVarVis.getObservedStructAccesses(),
-                   RVarVis.getObservedStructAccesses(), CommonStVars);
-
-  // If CommonVars is not empty, then the same pointer appears on the LHS and
-  // RHS of an assignment. We say that the pointer is assigned from a pointer
-  // arithmetic expression.
-  if (!CommonVars.empty() || !CommonStVars.empty())
+  if (isAssignmentPointerArithmetic(L ,R))
     recordArithmeticOperation(L, CR);
   return true;
 }

clang/lib/3C/RewriteUtils.cpp

@@ -431,35 +431,32 @@ class TypeArgumentAdder : public clang::RecursiveASTVisitor<TypeArgumentAdder> {
 class AssignmentUpdater : public RecursiveASTVisitor<AssignmentUpdater> {
 public:
   explicit AssignmentUpdater(ASTContext *C, ProgramInfo &I, Rewriter &R)
-    : Context(C), Info(I), CR(I, C), R(R) {}
+    : Info(I), CR(I, C), R(R) {}
 
   bool VisitBinaryOperator(BinaryOperator *O) {
-    // TODO: Use CollectDeclsVisitor
     if (O->getOpcode() == clang::BO_Assign) {
-      CollectDeclsVisitor LHSVis;
-      CVarSet LHSCVs = CR.getExprConstraintVarsSet(O->getLHS());
-      // FIXME: This isn't always a singleton: `int **a, **b; *(0 ? a : b) = 0;`
-      //        As long as inner pointers can't have bounds, we should be able
-      //        to differ a proper solution.
-      ConstraintVariable *CV = getOnly(LHSCVs);
-      CVarSet RHSCVs = CR.getExprConstraintVarsSet(O->getRHS());
-      bool VarOnRHS = RHSCVs.find(CV) != RHSCVs.end();
-      if (!VarOnRHS &&
-          CV->hasBoundsKey() &&
-          Info.getABoundsInfo().needsRangeBound(CV->getBoundsKey())) {
-        // FIXME: This is very likely wrong sometimes.
-        rewriteSourceRange(R, O->getLHS()->getSourceRange(),
-                           "__3c_tmp_" + CV->getName());
-        R.InsertTextAfterToken(O->getEndLoc(),
-                               ", " + CV->getName() + " = " + "__3c_tmp_" +
-                               CV->getName());
+      if (!isAssignmentPointerArithmetic(O->getLHS(), O->getRHS())) {
+        // TODO: I don't like calling getExprConstraintVars in the rewriting
+        //       because this has been a persistent source of errors in the cast
+        //       placement code.
+        CVarSet LHSCVs = CR.getExprConstraintVarsSet(O->getLHS());
+        // FIXME: This isn't always a singleton: `int **a, **b; *(0 ? a : b) = 0;`
+        //        As long as inner pointers can't have bounds, we should be able
+        //        to differ a proper solution.
+        ConstraintVariable *CV = getOnly(LHSCVs);
+        if (CV->hasBoundsKey() &&
+            Info.getABoundsInfo().needsRangeBound(CV->getBoundsKey())) {
+          std::string TmpVarName = "__3c_tmp_" + CV->getName();
+          rewriteSourceRange(R, O->getLHS()->getSourceRange(), TmpVarName);
+          R.InsertTextAfterToken(O->getEndLoc(),
+                                 ", " + CV->getName() + " = " + TmpVarName);
+        }
       }
     }
     return true;
   }
 
 private:
-  ASTContext *Context;
   ProgramInfo &Info;
   ConstraintResolver CR;
   Rewriter &R;
@@ -708,7 +705,6 @@ void RewriteConsumer::HandleTranslationUnit(ASTContext &Context) {
     CLV.TraverseDecl(D);
     ECPV.TraverseDecl(D);
     TPA.TraverseDecl(D);
-    //TODO: I don't believe the position of AU in the order should matter.
     AU.TraverseDecl(D);
   }
 

clang/lib/3C/Utils.cpp

@@ -15,6 +15,7 @@
 #include "clang/Sema/Sema.h"
 #include "llvm/Support/Path.h"
 #include <errno.h>
+#include <clang/3C/ConstraintResolver.h>
 
 using namespace llvm;
 using namespace clang;
@@ -598,3 +599,97 @@ SourceLocation getCheckedCAnnotationsEnd(const Decl *D) {
 
   return End;
 }
+
+// Visitor to collect all the variables and structure member access that are
+// used during the life-time of the visitor.
+class CollectDeclsVisitor : public RecursiveASTVisitor<CollectDeclsVisitor> {
+public:
+  explicit CollectDeclsVisitor() : ObservedDecls(), ObservedStructAccesses() {}
+
+  virtual ~CollectDeclsVisitor() {}
+
+  bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+    if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl()))
+      ObservedDecls.insert(VD);
+    return true;
+  }
+
+  // For a->b; We need to get `a->b` rather than just `b`. This way assignment
+  // from a field in one instance of a structure to the same field in another
+  // instance is not treated as pointer arithmetic.
+  bool VisitMemberExpr(MemberExpr *ME) {
+    // TODO: Is this cast legit? `getMemberDecl()` returns a `ValueDecl*`, but I
+    //       think it can only be a `FieldDecl` for structs in C.
+    auto *FD = cast<FieldDecl>(ME->getMemberDecl());
+
+    // TODO: Is recursively using a new visitor the right thing to do? It feels
+    //       odd.
+    CollectDeclsVisitor MEVis;
+    MEVis.TraverseStmt(ME->getBase());
+    // Field access through variable.
+    for (auto *D : MEVis.getObservedDecls()) {
+      std::vector<FieldDecl *> SingletonAccessList({FD});
+      ObservedStructAccesses.insert(std::make_pair(D, SingletonAccessList));
+    }
+    // Field access through other structure fields.
+    for (StructAccess SA : MEVis.getObservedStructAccesses()) {
+      SA.second.push_back(FD);
+      ObservedStructAccesses.insert(SA);
+    }
+    return false;
+  }
+
+  bool VisitCallExpr(CallExpr *CE) {
+    // Stop the visitor when we hit a CallExpr. This stops us from treating a
+    // function call like `a = foo(a);` the same as `a = a + 1`.
+    return false;
+  }
+
+  const std::set<VarDecl *> &getObservedDecls() { return ObservedDecls; }
+
+  typedef std::pair<VarDecl *, std::vector<FieldDecl *>> StructAccess;
+  const std::set<StructAccess> &getObservedStructAccesses() {
+    return ObservedStructAccesses;
+  }
+
+private:
+  // Contains all VarDecls seen by this visitor
+  std::set<VarDecl *> ObservedDecls;
+
+  // Contains the source representation of all record access (MemberExpression)
+  // seen by this visitor.
+  std::set<StructAccess> ObservedStructAccesses;
+};
+
+bool isAssignmentPointerArithmetic(Expr *LHS, Expr *RHS) {
+  CollectDeclsVisitor LVarVis;
+  LVarVis.TraverseStmt(LHS);
+
+  CollectDeclsVisitor RVarVis;
+  RVarVis.TraverseStmt(RHS);
+
+  std::set<VarDecl *> CommonVars;
+  std::set<CollectDeclsVisitor::StructAccess> CommonStVars;
+  findIntersection(LVarVis.getObservedDecls(), RVarVis.getObservedDecls(),
+                   CommonVars);
+  findIntersection(LVarVis.getObservedStructAccesses(),
+                   RVarVis.getObservedStructAccesses(), CommonStVars);
+
+  // If CommonVars is not empty, then the same pointer appears on the LHS and
+  // RHS of an assignment. We say that the pointer is assigned from a pointer
+  // arithmetic expression.
+  return !CommonVars.empty() || !CommonStVars.empty();
+
+  // TODO: Consider alternative implementation. The above code is based on an
+  //       implementation that was already used in the array bounds inference
+  //       code, so I have continued using it here. When I first needed this for
+  //       my own code, I reimplemented using constraint variable sets as shown
+  //       below. I have not consider exactly how these differ yet.
+#if 0
+  CVarSet LHSCVs = CR.getExprConstraintVarsSet(LHS);
+  CVarSet RHSCVs = CR.getExprConstraintVarsSet(RHS);
+  CVarSet CommonCVars;
+  findIntersection(LHSCVs, RHSCVs, CommonCVars);
+  return !CommonCVars.empty();
+#endif
+}