Merge branch 'main' of github.com:correctcomputation/checkedc-clang into inline-struct-fixes

Author: mattmccutchen-cci

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

@@ -43,8 +43,15 @@ typedef std::set<ConstraintKey> CVars;
 // Holds Atoms, one for each of the pointer (*) declared in the program.
 typedef std::vector<Atom *> CAtoms;
 
+// Options for ConstraintVariable::mkString, using the code pattern described in
+// clang/include/clang/3C/OptionalParams.h. Use the MKSTRING_OPTS macro to
+// generate a MkStringOpts instance at a call site.
 struct MkStringOpts {
+  // True when the generated string should include
+  // the name of the variable, false for just the type.
   bool EmitName = true;
+  // True when the generated string is expected
+  // to be used inside an itype.
   bool ForItype = false;
   bool EmitPointee = false;
   bool UnmaskTypedef = false;
@@ -114,11 +121,12 @@ class ConstraintVariable {
                          qtyToStr(QT, N == RETVAR ? "" : N)) {}
 
 public:
-  // Create a "for-rewriting" representation of this ConstraintVariable.
-  // The 'emitName' parameter is true when the generated string should include
-  // the name of the variable, false for just the type.
-  // The 'forIType' parameter is true when the generated string is expected
-  // to be used inside an itype.
+  // Generate source code for the type and (in certain cases) the name of the
+  // variable or function represented by this ConstraintVariable that reflects
+  // any changes made by 3C and is suitable to insert during rewriting.
+  //
+  // This method is used in several contexts with special requirements, which
+  // are addressed by the options in MkStringOpts; see the comments there.
   virtual std::string mkString(Constraints &CS,
                                const MkStringOpts &Opts = {}) const = 0;
 

clang/lib/3C/CastPlacement.cpp

@@ -172,33 +172,30 @@ CastPlacementVisitor::getCastString(ConstraintVariable *Dst,
   case CAST_TO_WILD:
     return std::make_pair("((" + Dst->getRewritableOriginalTy() + ")", ")");
   case CAST_TO_CHECKED: {
+    // Needed as default to TypeVar branch below, reset otherwise.
+    std::string Type = "_Ptr<";
     std::string Suffix = ")";
     if (const auto *DstPVC = dyn_cast<PVConstraint>(Dst)) {
       assert("Checked cast not to a pointer" && !DstPVC->getCvars().empty());
       ConstAtom *CA =
           Info.getConstraints().getAssignment(DstPVC->getCvars().at(0));
 
-      // Writing an _Assume_bounds_cast to an array type requires inserting
-      // the bounds for destination array. These can come from the source
-      // code or the infered bounds. If neither source is available, use empty
-      // bounds.
-      if (isa<ArrAtom>(CA) || isa<NTArrAtom>(CA)) {
-        std::string Bounds = "";
-        if (DstPVC->srcHasBounds())
-          Bounds = DstPVC->getBoundsStr();
-        else if (DstPVC->hasBoundsKey())
-          Bounds = ABRewriter.getBoundsString(DstPVC, nullptr, true);
-        if (Bounds.empty())
-          Bounds = "byte_count(0)";
-
-        Suffix = ", " + Bounds + ")";
+      // TODO: Writing an _Assume_bounds_cast to an array type requires
+      // inserting the bounds for destination array. But the names used in src
+      // and dest may be different, so we need more sophisticated code to
+      // convert to local variable names. Use unknown bounds for now.
+      if (isa<ArrAtom>(CA)) {
+        Type = "_Array_ptr<";
+        Suffix = ", bounds(unknown))";
+      } else if (isa<NTArrAtom>(CA)) {
+        Type = "_Nt_array_ptr<";
+        Suffix = ", bounds(unknown))";
       }
     }
     // The destination's type may be generic, which would have an out-of-scope
     // type var, so use the already analysed local type var instead
-    std::string Type;
     if (TypeVar != nullptr) {
-      Type = "_Ptr<" +
+      Type +=
           TypeVar->mkString(Info.getConstraints(),
                             MKSTRING_OPTS(EmitName = false, EmitPointee = true)) +
           ">";

clang/lib/3C/ConstraintVariables.cpp

@@ -755,12 +755,19 @@ PointerVariableConstraint::mkString(Constraints &CS,
   UNPACK_OPTS(EmitName, ForItype, EmitPointee, UnmaskTypedef, UseName,
               ForItypeBase);
 
+  // This function has become pretty ad-hoc and has a number of known bugs: see
+  // https://github.com/correctcomputation/checkedc-clang/issues/703. We hope to
+  // overhaul it in the future.
+
   // The name field encodes if this variable is the return type for a function.
   // TODO: store this information in a separate field.
   bool IsReturn = getName() == RETVAR;
 
-  if (UseName.empty())
+  if (UseName.empty()) {
     UseName = getName();
+    if (UseName.empty())
+      EmitName = false;
+  }
 
   if (IsTypedef && !UnmaskTypedef) {
     std::string QualTypedef = gatherQualStrings() + TypedefString;
@@ -787,13 +794,9 @@ PointerVariableConstraint::mkString(Constraints &CS,
   bool EmittedBase = false;
   // Have we emitted the name of the variable yet?
   bool EmittedName = false;
-  // Was the last variable an Array?
-  bool PrevArr = false;
-  // Is the entire type so far an array?
-  bool AllArrays = true;
   if (!EmitName || IsReturn)
     EmittedName = true;
-  uint32_t TypeIdx = 0;
+  int TypeIdx = 0;
 
   // If we've set a GenericIndex for void, it means we're converting it into
   // a generic function so give it the default generic type name.
@@ -807,16 +810,15 @@ PointerVariableConstraint::mkString(Constraints &CS,
   }
 
   auto It = Vars.begin();
-  auto I = 0;
   // Skip over first pointer level if only emitting pointee string.
   // This is needed when inserting type arguments.
   if (EmitPointee)
     ++It;
   // Iterate through the vars(), but if we have an internal typedef, then stop
   // once you reach the typedef's level.
   for (; It != Vars.end() && IMPLIES(TypedefLevelInfo.HasTypedef,
-                                     I < TypedefLevelInfo.TypedefLevel);
-       ++It, I++) {
+                                     TypeIdx < TypedefLevelInfo.TypedefLevel);
+       ++It, TypeIdx++) {
     const auto &V = *It;
     ConstAtom *C = nullptr;
     if (ForItypeBase) {
@@ -838,21 +840,23 @@ PointerVariableConstraint::mkString(Constraints &CS,
     if (!ForItype && InferredGenericIndex == -1 && isVoidPtr())
       K = Atom::A_Wild;
 
-    if (PrevArr && ArrSizes.at(TypeIdx).first != O_SizedArray && !EmittedName) {
-      EmittedName = true;
+    // In a case like `_Ptr<TYPE> p[2]`, the ` p[2]` needs to end up _after_ the
+    // `>`, so we need to push the ` p[2]` onto EndStrs _before_ the code below
+    // pushes the `>`. In general, before we visit a checked pointer level (not
+    // a checked array level), we need to transfer any pending array levels and
+    // emit the name (if applicable).
+    if (K != Atom::A_Wild && ArrSizes.at(TypeIdx).first != O_SizedArray) {
       addArrayAnnotations(ConstArrs, EndStrs);
-      EndStrs.push_front(" " + UseName);
+      if (!EmittedName) {
+        EmittedName = true;
+        EndStrs.push_front(" " + UseName);
+      }
     }
-    PrevArr = ArrSizes.at(TypeIdx).first == O_SizedArray;
 
     switch (K) {
     case Atom::A_Ptr:
       getQualString(TypeIdx, Ss);
 
-      // We need to check and see if this level of variable
-      // is constrained by a bounds safe interface. If it is,
-      // then we shouldn't re-write it.
-      AllArrays = false;
       EmittedBase = false;
       Ss << "_Ptr<";
       EndStrs.push_front(">");
@@ -866,38 +870,28 @@ PointerVariableConstraint::mkString(Constraints &CS,
       // we should substitute [K].
       if (emitArraySize(ConstArrs, TypeIdx, K))
         break;
-      AllArrays = false;
-      // We need to check and see if this level of variable
-      // is constrained by a bounds safe interface. If it is,
-      // then we shouldn't re-write it.
       EmittedBase = false;
       Ss << "_Array_ptr<";
       EndStrs.push_front(">");
       break;
     case Atom::A_NTArr:
+      // If this is an NTArray.
+      getQualString(TypeIdx, Ss);
       if (emitArraySize(ConstArrs, TypeIdx, K))
         break;
-      AllArrays = false;
-      // This additional check is to prevent fall-through from the array.
-      if (K == Atom::A_NTArr) {
-        // If this is an NTArray.
-        getQualString(TypeIdx, Ss);
 
-        // We need to check and see if this level of variable
-        // is constrained by a bounds safe interface. If it is,
-        // then we shouldn't re-write it.
-        EmittedBase = false;
-        Ss << "_Nt_array_ptr<";
-        EndStrs.push_front(">");
-        break;
-      }
-      LLVM_FALLTHROUGH;
+      EmittedBase = false;
+      Ss << "_Nt_array_ptr<";
+      EndStrs.push_front(">");
+      break;
     // If there is no array in the original program, then we fall through to
     // the case where we write a pointer value.
     case Atom::A_Wild:
       if (emitArraySize(ConstArrs, TypeIdx, K))
         break;
-      AllArrays = false;
+      // FIXME: This code emits wild pointer levels with the outermost on the
+      // left. The outermost should be on the right
+      // (https://github.com/correctcomputation/checkedc-clang/issues/161).
       if (FV != nullptr) {
         FptrInner << "*";
         getQualString(TypeIdx, FptrInner);
@@ -917,35 +911,19 @@ PointerVariableConstraint::mkString(Constraints &CS,
       llvm_unreachable("impossible");
       break;
     }
-    TypeIdx++;
-  }
-
-  // If the previous variable was an array or
-  // if we are leaving an array run, we need to emit the
-  // annotation for a stack-array
-  if (PrevArr && !ConstArrs.empty())
-    addArrayAnnotations(ConstArrs, EndStrs);
-
-  // If the whole type is an array so far, and we haven't emitted
-  // a name yet, then emit the name so that it appears before
-  // the the stack array type.
-  if (PrevArr && !EmittedName && AllArrays) {
-    EmittedName = true;
-    EndStrs.push_front(" " + UseName);
   }
 
   if (!EmittedBase) {
     // If we have a FV pointer, then our "base" type is a function pointer type.
     if (FV) {
-      if (Ss.str().empty()) {
-        if (!EmittedName) {
-          FptrInner << UseName;
-          EmittedName = true;
-        }
-        for (std::string Str : EndStrs)
-          FptrInner << Str;
-        EndStrs.clear();
+      if (!EmittedName) {
+        FptrInner << UseName;
+        EmittedName = true;
       }
+      std::deque<std::string> FptrEndStrs;
+      addArrayAnnotations(ConstArrs, FptrEndStrs);
+      for (std::string Str : FptrEndStrs)
+        FptrInner << Str;
       bool EmitFVName = !FptrInner.str().empty();
       if (EmitFVName)
         Ss << FV->mkString(CS, MKSTRING_OPTS(UseName = FptrInner.str(),
@@ -963,27 +941,20 @@ PointerVariableConstraint::mkString(Constraints &CS,
     }
   }
 
-  // Add closing elements to type
-  for (std::string Str : EndStrs) {
-    Ss << Str;
-  }
-
   // No space after itype.
-  if (!EmittedName && !UseName.empty()) {
+  if (!EmittedName) {
     if (!StringRef(Ss.str()).endswith("*"))
       Ss << " ";
     Ss << UseName;
   }
 
-  // Final array dropping.
-  if (!ConstArrs.empty()) {
-    std::deque<std::string> ArrStrs;
-    addArrayAnnotations(ConstArrs, ArrStrs);
-    for (std::string Str : ArrStrs)
-      Ss << Str;
+  // Add closing elements to type
+  addArrayAnnotations(ConstArrs, EndStrs);
+  for (std::string Str : EndStrs) {
+    Ss << Str;
   }
 
-  if (IsReturn && !ForItype)
+  if (IsReturn && !ForItype && !StringRef(Ss.str()).endswith("*"))
     Ss << " ";
 
   return Ss.str();

clang/lib/3C/RewriteUtils.cpp

@@ -636,11 +636,10 @@ void RewriteConsumer::emitRootCauseDiagnostics(ASTContext &Context) {
       if (!File.getError()) {
         SourceLocation SL =
             SM.translateFileLineCol(*File, PSL.getLineNo(), PSL.getColSNo());
-        // Limit emitted root causes to those that effect more than one pointer
-        // or are in the main file of the TU. Alternatively, don't filter causes
-        // if -warn-all-root-cause is passed.
+        // Limit emitted root causes to those that effect at least one pointer.
+        // Alternatively, don't filter causes if -warn-all-root-cause is passed.
         int PtrCount = I.getNumPtrsAffected(WReason.first);
-        if (_3COpts.WarnAllRootCause || SM.isInMainFile(SL) || PtrCount > 1) {
+        if (_3COpts.WarnAllRootCause || PtrCount > 0) {
           // SL is invalid when the File is not in the current translation unit.
           if (SL.isValid()) {
             EmittedDiagnostics.insert(PSL);

clang/test/3C/b_tests/b1_allsafe.c

@@ -31,7 +31,7 @@ int *foo() {
   //CHECK: _Ptr<int> y = &sy;
   int *z = sus(x, y);
   //CHECK_NOALL: _Ptr<int> z = sus(x, y);
-  //CHECK_ALL: _Ptr<int> z = sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y);
+  //CHECK_ALL: _Ptr<int> z = sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y);
   *z = *z + 1;
   return z;
 }
@@ -47,6 +47,6 @@ int *bar() {
   //CHECK: _Ptr<int> y = &sy;
   int *z = (sus(x, y));
   //CHECK_NOALL: _Ptr<int> z = (sus(x, y));
-  //CHECK_ALL: _Ptr<int> z = (sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y));
+  //CHECK_ALL: _Ptr<int> z = (sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y));
   return z;
 }

clang/test/3C/b_tests/b23_explicitunsafecast.c

@@ -30,7 +30,7 @@ int *foo() {
   //CHECK: _Ptr<int> y = &sy;
   int *z = (int *)sus(x, y);
   //CHECK_NOALL: _Ptr<int> z = (_Ptr<int>)sus(x, y);
-  //CHECK_ALL: _Ptr<int> z = (_Ptr<int>)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y);
+  //CHECK_ALL: _Ptr<int> z = (_Ptr<int>)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y);
   *z = *z + 1;
   return z;
 }
@@ -45,6 +45,6 @@ char *bar() {
   //CHECK: _Ptr<int> y = &sy;
   char *z = (char *)(sus(x, y));
   //CHECK_NOALL: char *z = (char *)(((int *)sus(x, y)));
-  //CHECK_ALL: char *z = (char *)(((int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y)));
+  //CHECK_ALL: char *z = (char *)(((int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y)));
   return z;
 }

clang/test/3C/b_tests/b23_retswitchexplicit.c

@@ -30,7 +30,7 @@ char *foo() {
   //CHECK: _Ptr<int> y = &sy;
   char *z = (int *)sus(x, y);
   //CHECK_NOALL: char *z = (int *)sus(x, y);
-  //CHECK_ALL: char *z = (int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y);
+  //CHECK_ALL: char *z = (int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y);
   *z = *z + 1;
   return z;
 }
@@ -45,6 +45,6 @@ int *bar() {
   //CHECK: _Ptr<int> y = &sy;
   int *z = (char *)(sus(x, y));
   //CHECK_NOALL: int *z = (char *)(sus(x, y));
-  //CHECK_ALL: int *z = (char *)(sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y));
+  //CHECK_ALL: int *z = (char *)(sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y));
   return z;
 }

clang/test/3C/b_tests/b24_implicitunsafecast.c

@@ -30,7 +30,7 @@ int *foo() {
   //CHECK: _Ptr<int> y = &sy;
   int *z = (int *)sus(x, y);
   //CHECK_NOALL: _Ptr<int> z = (_Ptr<int>)sus(x, y);
-  //CHECK_ALL: _Ptr<int> z = (_Ptr<int>)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y);
+  //CHECK_ALL: _Ptr<int> z = (_Ptr<int>)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y);
   *z = *z + 1;
   return z;
 }
@@ -45,6 +45,6 @@ char *bar() {
   //CHECK: _Ptr<int> y = &sy;
   char *z = sus(x, y);
   //CHECK_NOALL: char *z = ((int *)sus(x, y));
-  //CHECK_ALL: char *z = ((int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y));
+  //CHECK_ALL: char *z = ((int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y));
   return z;
 }

clang/test/3C/b_tests/b24_retswitchimplicit.c

@@ -30,7 +30,7 @@ char *foo() {
   //CHECK: _Ptr<int> y = &sy;
   char *z = (int *)sus(x, y);
   //CHECK_NOALL: char *z = (int *)sus(x, y);
-  //CHECK_ALL: char *z = (int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y);
+  //CHECK_ALL: char *z = (int *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y);
   *z = *z + 1;
   return z;
 }
@@ -45,6 +45,6 @@ int *bar() {
   //CHECK: _Ptr<int> y = &sy;
   int *z = sus(x, y);
   //CHECK_NOALL: int *z = ((char *)sus(x, y));
-  //CHECK_ALL: int *z = ((char *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, byte_count(0)), y));
+  //CHECK_ALL: int *z = ((char *)sus(_Assume_bounds_cast<_Array_ptr<int>>(x, bounds(unknown)), y));
   return z;
 }