Cleanup

Author: john-h-kastner

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

@@ -262,7 +262,7 @@ class AVarBoundsInfo {
   // Check if the bounds keys will need to be rewritten with range bounds. This
   // is true for bounds keys that are subject to pointer arithmetic but
   // otherwise have inferred bounds.
-  bool needsRangeBound(BoundsKey BK);
+  bool needsRangeBound(ConstraintVariable *CV);
 
   // Get the ProgramVar for the provided VarKey.
   ProgramVar *getProgramVar(BoundsKey VK);

clang/lib/3C/AVarBoundsInfo.cpp

@@ -813,6 +813,10 @@ BoundsKey AVarBoundsInfo::getVariable(clang::VarDecl *VD) {
     insertProgramVar(NK, PVar);
     if (isPtrOrArrayType(VD->getType())) {
       PointerBoundsKey.insert(NK);
+      // Global variables cannot be given range bounds because it is not
+      // possible to initialize a duplicated pointer variable with the same
+      // value as the original.
+      // TODO: Followup issue.
       if (!VD->isLocalVarDeclOrParm())
         IneligibleForRangeBounds.insert(NK);
     }
@@ -880,6 +884,10 @@ BoundsKey AVarBoundsInfo::getVariable(clang::FieldDecl *FD) {
     insertProgramVar(NK, PVar);
     if (isPtrOrArrayType(FD->getType())) {
       PointerBoundsKey.insert(NK);
+      // Fields are not rewritten with range bounds because we would need to
+      // duplicate the field and update all structure initializations to
+      // properly set the new field.
+      // TODO: Followup issue.
       IneligibleForRangeBounds.insert(NK);
     }
   }
@@ -951,16 +959,17 @@ bool AVarBoundsInfo::handleAssignment(clang::Decl *L, CVarOption LCVars,
 }
 
 bool AVarBoundsInfo::addAssignment(BoundsKey L, BoundsKey R) {
-  // Before adding an edge from L to R or R to L, first verify that the source
-  // BoundsKey for the edge is not computed by pointer arithmetic. The pointer
-  // arithmetic invalidates the bounds on the pointer, so bounds should not
-  // propagate through it. The bounds are allowed to exist on the pointer
-  // because they will emitted as range bounds based on a different base
-  // pointer. For future work, all bounds "down stream" of pointer arithmetic
-  // could also use range bounds using the same base pointer.
   auto AddEdgeUnlessPointerArithmetic = [this](BoundsKey From, BoundsKey To) {
-    if (!hasPointerArithmetic(From) &&
-        (!hasPointerArithmetic(To) || canInferRangeBounds(To)))
+    // Verify that the source BoundsKey for the edge is not computed by pointer
+    // arithmetic. Pointer arithmetic invalidates the bounds on the pointer, so
+    // bounds should not propagate through it.
+    // TODO: Followup issue
+    bool FromValid = !hasPointerArithmetic(From);
+    // The destination BoundsKey may be computed by pointer arithmetic as long
+    // 3C can emit range bounds the pointer. If 3C cannot emit range bounds,
+    // then the incoming edge is not added so that no bounds will be inferred.
+    bool ToValid = !hasPointerArithmetic(To) || canInferRangeBounds(To);
+    if (FromValid && ToValid)
       ProgVarGraph.addUniqueEdge(From, To);
   };
 
@@ -1016,7 +1025,10 @@ bool AVarBoundsInfo::canInferRangeBounds(BoundsKey BK) {
   return IneligibleForRangeBounds.find(BK) == IneligibleForRangeBounds.end();
 }
 
-bool AVarBoundsInfo::needsRangeBound(BoundsKey BK) {
+bool AVarBoundsInfo::needsRangeBound(ConstraintVariable *CV) {
+  if (!CV->hasBoundsKey())
+    return false;
+  BoundsKey BK = CV->getBoundsKey();
   // A pointer should get range bounds if it is computed by pointer arithmetic
   // and would otherwise need bounds. Some pointers (global variables and struct
   // fields) can't be rewritten to use range bounds (by 3C; Checked C does

clang/lib/3C/DeclRewriter.cpp

@@ -37,9 +37,7 @@ void DeclRewriter::buildItypeDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
                                   ProgramInfo &Info, ArrayBoundsRewriter &ABR,
                                   std::vector<std::string> *SDecls) {
   bool NeedsRangeBound =
-    SDecls != nullptr &&
-    Defn->hasBoundsKey() &&
-    Info.getABoundsInfo().needsRangeBound(Defn->getBoundsKey());
+    SDecls != nullptr && Info.getABoundsInfo().needsRangeBound(Defn);
   assert("Adding range bounds on return, global variable, or field!" &&
          (!NeedsRangeBound || (isa_and_nonnull<VarDecl>(Decl) &&
                                cast<VarDecl>(Decl)->isLocalVarDeclOrParm())));
@@ -130,9 +128,7 @@ void DeclRewriter::buildCheckedDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
                                     std::vector<std::string> *SDecls) {
   // I don't like using SDecls != nullptr as a nullptr as a sentinel like this.
   bool NeedsRangeBound =
-    SDecls != nullptr &&
-    Defn->hasBoundsKey() &&
-    Info.getABoundsInfo().needsRangeBound(Defn->getBoundsKey());
+    SDecls != nullptr && Info.getABoundsInfo().needsRangeBound(Defn);
   assert("Adding range bounds on return, global variable, or field!" &&
          (!NeedsRangeBound || (isa_and_nonnull<VarDecl>(Decl) &&
                                cast<VarDecl>(Decl)->isLocalVarDeclOrParm())));

clang/lib/3C/RewriteUtils.cpp

@@ -431,31 +431,34 @@ class TypeArgumentAdder : public clang::RecursiveASTVisitor<TypeArgumentAdder> {
 class AssignmentUpdater : public RecursiveASTVisitor<AssignmentUpdater> {
 public:
   explicit AssignmentUpdater(ASTContext *C, ProgramInfo &I, Rewriter &R)
-    : Info(I), CR(I, C), R(R) {}
+    : ABInfo(I.getABoundsInfo()), CR(I, C), R(R) {}
 
   bool VisitBinaryOperator(BinaryOperator *O) {
     if (O->getOpcode() == clang::BO_Assign) {
       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);
-          bool InsertFail = R.InsertTextAfterToken(O->getEndLoc(),
-                                                        ", " + CV->getName() +
-                                                        " = " + TmpVarName);
-          if (InsertFail) {
-            // FIXME: Use rewriteSourceRange so that it handle emitting error messages.
-            llvm::errs()
-              << "Rewriting failed while updating assignment!\n";
-            O->getEndLoc().print(llvm::errs(), R.getSourceMgr());
+        // It is possible for multiple ConstraintVariables to exist on the LHS
+        // of an assignment expression; e.g., `*(0 ? a : b) = 0`. If this
+        // happens, and one of those variables needed range bounds, then the
+        // following rewriting is not correct. I believe that it can only happen
+        // when the LHS is a pointer dereference or struct field access.
+        // Structure fields and inner pointer levels can never have range bounds
+        // so this case currently is not possible.
+        assert(LHSCVs.size() == 1 || llvm::count_if(LHSCVs, [this](
+          ConstraintVariable *CV) { return ABInfo.needsRangeBound(CV); }) == 0);
+        for (ConstraintVariable *CV: LHSCVs) {
+          if (ABInfo.needsRangeBound(CV)) {
+            std::string TmpVarName = "__3c_tmp_" + CV->getName();
+            rewriteSourceRange(R, O->getLHS()->getSourceRange(), TmpVarName);
+            bool InsertFail = R.InsertTextAfterToken(O->getEndLoc(),
+                                                          ", " + CV->getName() +
+                                                          " = " + TmpVarName);
+            if (InsertFail) {
+              // FIXME: Use rewriteSourceRange so that it handle emitting error messages.
+              llvm::errs()
+                << "Rewriting failed while updating assignment!\n";
+              O->getEndLoc().print(llvm::errs(), R.getSourceMgr());
+            }
           }
         }
       }
@@ -464,7 +467,7 @@ class AssignmentUpdater : public RecursiveASTVisitor<AssignmentUpdater> {
   }
 
 private:
-  ProgramInfo &Info;
+  AVarBoundsInfo &ABInfo;
   ConstraintResolver CR;
   Rewriter &R;
 };

clang/lib/3C/Utils.cpp

@@ -614,16 +614,14 @@ class CollectDeclsVisitor : public RecursiveASTVisitor<CollectDeclsVisitor> {
     return true;
   }
 
-  // For a->b; We need to get `a->b` rather than just `b`. This way assignment
+  // 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
+    // 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.