Treat the bounds of _Nt_checked as one less than the written length (#653)

The Checked C bounds for constant sized `_Nt_checked` arrays is one less
than the written size because the written size counts the null
terminator while the Checked C bounds do not.

To make this change, the declared bounds of constant size arrays must be
added to the array bounds inference information after pointer type
constraint solving. This lets `_Checked` and `_Nt_checked` array be treated
differently.

This also fixes an error in `canBeNtArray` that caused  `ensureNtCorrect `to add
incorrect constraints. The function checked if a type was a `PointerType` or
`ArrayType`, but it could be a wrapper type around one of these types. For
example, `ParenType` and `DecayedType` instances were both treated as
not pointers or arrays even if the type they wrapped was a pointer or array type.

Fixes #396

Author: john-h-kastner

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

@@ -197,6 +197,8 @@ class AVarBoundsInfo {
   bool isValidBoundVariable(clang::Decl *D);
 
   void insertDeclaredBounds(clang::Decl *D, ABounds *B);
+  void insertDeclaredBounds(BoundsKey BK, ABounds *B);
+
   bool mergeBounds(BoundsKey L, BoundsPriority P, ABounds *B);
   bool removeBounds(BoundsKey L, BoundsPriority P = Invalid);
   bool replaceBounds(BoundsKey L, BoundsPriority P, ABounds *B);
@@ -290,6 +292,8 @@ class AVarBoundsInfo {
   // If yes, provide the index of the parameter.
   bool isFuncParamBoundsKey(BoundsKey BK, unsigned &PIdx);
 
+  void addConstantArrayBounds(ProgramInfo &I);
+
 private:
   friend class AvarBoundsInference;
   friend class CtxSensitiveBoundsKeyHandler;

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

@@ -410,7 +410,6 @@ class PointerVariableConstraint : public ConstraintVariable {
 
 public:
   std::string getTy() const { return BaseType; }
-  bool getArrPresent() const;
   // Check if the outermost pointer is an unsized array.
   bool isTopAtomUnsizedArr() const;
   // Check if any of the pointers is either a sized or unsized arr.
@@ -532,6 +531,10 @@ class PointerVariableConstraint : public ConstraintVariable {
   bool hasArr(const EnvironmentMap &E, int AIdx = -1) const override;
   bool hasNtArr(const EnvironmentMap &E, int AIdx = -1) const override;
 
+  bool isNtConstantArr(const EnvironmentMap &E) const;
+  bool isConstantArr() const;
+  unsigned long getConstantArrSize() const;
+
   void equateArgumentConstraints(ProgramInfo &I) override;
 
   bool isPartOfFunctionPrototype() const { return PartOfFuncPrototype; }

clang/lib/3C/3C.cpp

@@ -580,6 +580,12 @@ bool _3CInterface::solveConstraints() {
     dumpConstraintOutputJson(FINAL_OUTPUT_SUFFIX, GlobalProgramInfo);
 
   if (_3COpts.AllTypes) {
+    // Add declared bounds for all constant sized arrays. This needs to happen
+    // after constraint solving because the bound added depends on whether the
+    // array is NTARR or ARR.
+    GlobalProgramInfo.getABoundsInfo().addConstantArrayBounds(
+      GlobalProgramInfo);
+
     if (DebugArrSolver)
       GlobalProgramInfo.getABoundsInfo().dumpAVarGraph(
           "arr_bounds_initial.dot");

clang/lib/3C/AVarBoundsInfo.cpp

@@ -640,10 +640,7 @@ bool AVarBoundsInfo::isValidBoundVariable(clang::Decl *D) {
   return false;
 }
 
-void AVarBoundsInfo::insertDeclaredBounds(clang::Decl *D, ABounds *B) {
-  assert(isValidBoundVariable(D) && "Declaration not a valid bounds variable");
-  BoundsKey BK;
-  tryGetVariable(D, BK);
+void AVarBoundsInfo::insertDeclaredBounds(BoundsKey BK, ABounds *B) {
   if (B != nullptr) {
     // If there is already bounds information, release it.
     removeBounds(BK);
@@ -656,6 +653,13 @@ void AVarBoundsInfo::insertDeclaredBounds(clang::Decl *D, ABounds *B) {
   }
 }
 
+void AVarBoundsInfo::insertDeclaredBounds(clang::Decl *D, ABounds *B) {
+  assert(isValidBoundVariable(D) && "Declaration not a valid bounds variable");
+  BoundsKey BK;
+  tryGetVariable(D, BK);
+  insertDeclaredBounds(BK, B);
+}
+
 bool AVarBoundsInfo::tryGetVariable(clang::Decl *D, BoundsKey &R) {
   bool RetVal = false;
   if (isValidBoundVariable(D)) {
@@ -1471,3 +1475,35 @@ std::set<BoundsKey> AVarBoundsInfo::getCtxSensFieldBoundsKey(Expr *E,
   }
   return Ret;
 }
+
+// Adds declared bounds for all constant sized arrays. This needs to happen
+// after constraint solving because the bounds for a _Nt_checked array and a
+// _Checked array are different even if they are written with the same length.
+// The Checked C bounds for a _Nt_checked array do not include the null
+// terminator, but the length as written in the source code does.
+void AVarBoundsInfo::addConstantArrayBounds(ProgramInfo &I) {
+  for (auto VarEntry : I.getVarMap()) {
+    if (auto *VarPCV = dyn_cast<PVConstraint>(VarEntry.second)) {
+      if (VarPCV->hasBoundsKey() && VarPCV->isConstantArr()) {
+        // Lookup the declared size of the array. This is known because it is
+        // written in the source and was stored during constraint generation.
+        unsigned int ConstantCount = VarPCV->getConstantArrSize();
+
+        // Check if this array solved to NTARR. If it did, subtract one from the
+        // length to account for the null terminator.
+        const EnvironmentMap &Env = I.getConstraints().getVariables();
+        if (VarPCV->isNtConstantArr(Env)) {
+          assert("Size zero constant array should not solve to NTARR" &&
+                 ConstantCount != 0);
+          ConstantCount--;
+        }
+
+        // Insert this as a declared constant count bound for the constraint
+        // variable.
+        BoundsKey CBKey = getConstKey(ConstantCount);
+        ABounds *NB = new CountBound(CBKey);
+        insertDeclaredBounds(VarPCV->getBoundsKey(), NB);
+      }
+    }
+  }
+}

clang/lib/3C/ConstraintResolver.cpp

@@ -382,7 +382,7 @@ CSetBkeyPair ConstraintResolver::getExprConstraintVars(Expr *E) {
             if (auto *PCV = dyn_cast<PVConstraint>(CV))
               // On the other hand, CheckedC does let you take the address of
               // constant sized arrays.
-              if (!PCV->getArrPresent())
+              if (!PCV->isConstantArr())
                 PCV->constrainOuterTo(CS, CS.getPtr(), true);
           // Add a VarAtom to UOExpr's PVConstraint, for &.
           Ret = std::make_pair(addAtomAll(T.first, CS.getPtr(), CS), T.second);

clang/lib/3C/ConstraintVariables.cpp

@@ -330,10 +330,6 @@ PointerVariableConstraint::PointerVariableConstraint(
   }
   InferredGenericIndex = SourceGenericIndex;
 
-  bool VarCreated = false;
-  bool IsArr = false;
-  bool IsIncompleteArr = false;
-  bool IsTopMost = true;
   uint32_t TypeIdx = 0;
   std::string Npre = InFunc ? ((*InFunc) + ":") : "";
   VarAtom::VarKind VK =
@@ -355,6 +351,8 @@ PointerVariableConstraint::PointerVariableConstraint(
     TLoc = D->getTypeSourceInfo()->getTypeLoc();
 
   while (Ty->isPointerType() || Ty->isArrayType()) {
+    bool VarCreated = false;
+
     // Is this a VarArg type?
     std::string TyName = tyToStr(Ty);
     if (isVarArgType(TyName)) {
@@ -404,9 +402,6 @@ PointerVariableConstraint::PointerVariableConstraint(
     }
 
     if (Ty->isArrayType() || Ty->isIncompleteArrayType()) {
-      IsArr = true;
-      IsIncompleteArr = Ty->isIncompleteArrayType();
-
       // Boil off the typedefs in the array case.
       // TODO this will need to change to properly account for typedefs
       bool Boiling = true;
@@ -445,13 +440,6 @@ PointerVariableConstraint::PointerVariableConstraint(
               ArrSizeStrs[TypeIdx] = SizeStr;
           }
         }
-
-        // If this is the top-most pointer variable?
-        if (hasBoundsKey() && IsTopMost) {
-          BoundsKey CBKey = ABInfo.getConstKey(CAT->getSize().getZExtValue());
-          ABounds *NB = new CountBound(CBKey);
-          ABInfo.insertDeclaredBounds(D, NB);
-        }
       } else {
         ArrSizes[TypeIdx] =
             std::pair<OriginalArrType, uint64_t>(O_UnSizedArray, 0);
@@ -479,28 +467,29 @@ PointerVariableConstraint::PointerVariableConstraint(
     }
 
     // This type is not a constant atom. We need to create a VarAtom for this.
-
     if (!VarCreated) {
       VarAtom *VA = CS.getFreshVar(Npre + N, VK);
       Vars.push_back(VA);
       SrcVars.push_back(CS.getWild());
 
-      // Incomplete arrays are lower bounded to ARR because the transformation
-      // int[] -> _Ptr<int> is permitted while int[1] -> _Ptr<int> is not.
-      if (IsIncompleteArr)
-        CS.addConstraint(CS.createGeq(VA, CS.getArr(), false));
-      else if (IsArr)
+      // Incomplete arrays are not given ARR as an upper bound because the
+      // transformation int[] -> _Ptr<int> is permitted but int[1] -> _Ptr<int>
+      // is not.
+      if (ArrSizes[TypeIdx].first == O_SizedArray) {
         CS.addConstraint(CS.createGeq(CS.getArr(), VA, false));
+        // A constant array declared with size 0 cannot be _Nt_checked. Checked
+        // C requires that _Nt_checked arrays are not empty since the declared
+        // size of the array includes the null terminator.
+        if (ArrSizes[TypeIdx].second == 0)
+          CS.addConstraint(CS.createGeq(VA, CS.getArr(), false));
+      }
     }
 
     // Prepare for next level of pointer
-    VarCreated = false;
-    IsArr = false;
     TypeIdx++;
     Npre = Npre + "*";
-    VK = VarAtom::
-        V_Other; // only the outermost pointer considered a param/return
-    IsTopMost = false;
+    // Only the outermost pointer considered a param/return
+    VK = VarAtom::V_Other;
     if (!TLoc.isNull())
       TLoc = TLoc.getNextTypeLoc();
   }
@@ -1483,9 +1472,28 @@ bool PointerVariableConstraint::hasNtArr(const EnvironmentMap &E,
   return false;
 }
 
-bool PointerVariableConstraint::getArrPresent() const {
-  return llvm::any_of(ArrSizes,
-                      [](auto E) { return E.second.first != O_Pointer; });
+bool PointerVariableConstraint::isNtConstantArr(const EnvironmentMap &E) const {
+  // First check that this pointer is a constant sized array. This function is
+  // only looking for constant sized arrays, so other array pointers should
+  // return false.
+  if (isConstantArr()) {
+    assert("Atoms vector for array type can't be empty." && !Vars.empty());
+    // This is a constant sized array. It might have solved to WILD, ARR, or
+    // NTARR, but we should only return true if we know it's NTARR.
+    const ConstAtom *PtrType = getSolution(Vars[0], E);
+    return isa<NTArrAtom>(PtrType);
+  }
+  return  false;
+}
+
+bool PointerVariableConstraint::isConstantArr() const {
+  return ArrSizes.find(0) != ArrSizes.end() &&
+         ArrSizes.at(0).first == O_SizedArray;
+}
+
+unsigned long PointerVariableConstraint::getConstantArrSize() const {
+  assert("Pointer must be a constant array to get size." && isConstantArr());
+  return ArrSizes.at(0).second;
 }
 
 bool PointerVariableConstraint::isTopAtomUnsizedArr() const {

clang/lib/3C/Utils.cpp

@@ -209,9 +209,12 @@ bool isNullableType(const clang::QualType &QT) {
 }
 
 bool canBeNtArray(const clang::QualType &QT) {
-  if (const auto &Ptr = dyn_cast<clang::PointerType>(QT))
+  // First get the canonical type so that the following checks will not have to
+  // account for ParenType, DecayedType, or other variants used by clang.
+  QualType Canon = QT.getCanonicalType();
+  if (const auto &Ptr = dyn_cast<clang::PointerType>(Canon))
     return isNullableType(Ptr->getPointeeType());
-  if (const auto &Arr = dyn_cast<clang::ArrayType>(QT))
+  if (const auto &Arr = dyn_cast<clang::ArrayType>(Canon))
     return isNullableType(Arr->getElementType());
   return false;
 }

clang/test/3C/canonical_type_cast.c

@@ -14,11 +14,9 @@ void f(int *p) {
 }
 
 void g(int p[]) {
-  //CHECK_NOALL: void g(int p[]) {
-  //CHECK_ALL: void g(_Ptr<int> p) {
+  //CHECK: void g(_Ptr<int> p) {
   int *x = (int *)p;
-  //CHECK_NOALL: int *x = (int *)p;
-  //CHECK_ALL: _Ptr<int> x = (_Ptr<int>)p;
+  //CHECK: _Ptr<int> x = (_Ptr<int>)p;
 }
 
 /* A very similar issue with function pointers */

clang/test/3C/cant_be_nt.c

@@ -46,3 +46,27 @@ float* dar(struct foobar f) {
   //CHECK: _Ptr<float> dar(struct foobar f) {
   return f.ptr;
 }
+
+// Test a bug I found in canBeNtArray triggered by some specific types.
+// The P >= ARR constraint was incorrectly added to these variables, causing
+// them to solve to WILD instead of NTARR.
+
+void force_nt_arr(char *s : itype(_Nt_array_ptr<char>));
+
+void decayed_type(char s[10]) {
+//CHECK_NOALL: void decayed_type(char s[10]) {
+//CHECK_ALL: void decayed_type(char s _Nt_checked[10]) {
+  force_nt_arr(s);
+}
+
+void paren_type(char *(s)) {
+//CHECK_NOALL: void paren_type(char *(s) : itype(_Ptr<char>)) {
+//CHECK_ALL: void paren_type(_Nt_array_ptr<char> s) {
+  force_nt_arr(s);
+}
+
+void decayed_paren_type(char (s)[10]) {
+//CHECK_NOALL: void decayed_paren_type(char (s)[10]) {
+//CHECK_ALL: void decayed_paren_type(char s _Nt_checked[10]) {
+  force_nt_arr(s);
+}

clang/test/3C/nt_const_arr.c

@@ -0,0 +1,61 @@
+// RUN: rm -rf %t*
+// RUN: 3c -base-dir=%S -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK,CHECK_NOALL" %s
+// RUN: 3c -base-dir=%S -alltypes -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK,CHECK_ALL" %s
+// RUN: 3c -base-dir=%S -alltypes -addcr %s -- | %clang -c -x c -o /dev/null -
+// RUN: 3c -base-dir=%S -alltypes -output-dir=%t.checked %s --
+// RUN: 3c -base-dir=%t.checked -alltypes %t.checked/nt_const_arr.c -- | diff %t.checked/nt_const_arr.c -
+
+// A _Nt_checked constant sized array declared with size `n` should be treated
+// as having bounds `count(n - 1)` durring bounds inference because the null
+// terminator is included in the declared length but not in the Checked C
+// bounds.
+
+unsigned long strlen(const char *s : itype(_Nt_array_ptr<const char>));
+
+void foo() {
+  char foo[5] = "test";
+  //CHECK_ALL: char foo _Nt_checked[5] = "test";
+  //CHECK_NOALL: char foo[5] = "test";
+  char *bar = foo;
+  //CHECK_ALL: _Nt_array_ptr<char> bar : count(4) = foo;
+  //CHECK_NOALL: char *bar = foo;
+  strlen(bar);
+
+  char *baz = foo;
+  //CHECK_ALL: _Array_ptr<char> baz : count(4) = foo;
+  //CHECK_NOALL: char *baz = foo;
+  (void) baz[0];
+}
+
+void arr_param(char foo[10]) {
+//CHECK_ALL: void arr_param(char foo _Nt_checked[10]) _Checked {
+//CHECK_NOALL: void arr_param(char foo[10]) {
+  char *bar = foo;
+  //CHECK_ALL:_Nt_array_ptr<char> bar : count(9) = foo;
+  //CHECK_NOALL: char *bar = foo;
+  strlen(bar);
+}
+
+void empty() {
+  // _Nt_checked can't be empty, so this should solve to WILD
+  char empty_nt[0];
+  char *bar = empty_nt;
+  //CHECK: char empty_nt[0];
+  //CHECK: char *bar = empty_nt;
+  strlen(bar);
+
+
+  // We can have an empty string by declaring arr with size 1
+  char empty_str[1] = "";
+  char *baz = empty_str;
+  //CHECK_ALL: char empty_str _Nt_checked[1] = "";
+  //CHECK_NOALL: char empty_str[1] = "";
+  //CHECK_ALL: _Nt_array_ptr<char> baz : count(0) = empty_str;
+  //CHECK_NOALL: char *baz = empty_str;
+  strlen(baz);
+
+  // _Checked can also be empty
+  char empty_checked[0];
+  //CHECK_ALL: char empty_checked _Checked[0];
+  //CHECK_NOALL: char empty_checked[0];
+}