mkString fixes (mostly moved from the multi-decl overhaul) (#714)

- Remove extra spaces after wild pointer `*` symbols in some cases.

- Generate valid output in some more complex cases involving checked
  pointers, fixed-size arrays, and/or function pointers.

- Remove now-obsolete code from mkString.

Author: mattmccutchen-cci

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

@@ -42,8 +42,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;
@@ -113,11 +120,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/ConstraintVariables.cpp

@@ -750,12 +750,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;
@@ -782,13 +789,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.
@@ -802,16 +805,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) {
@@ -833,21 +835,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(">");
@@ -861,38 +865,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);
@@ -912,35 +906,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(),
@@ -958,27 +936,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/test/3C/compound_literal.c

@@ -67,7 +67,7 @@ void lists() {
 
   int *d[2] = (int *[2]){&x, (int *)1};
   //CHECK_NOALL: int *d[2] = (int *[2]){&x, (int *)1};
-  //CHECK_ALL: int * d _Checked[2] = (int * _Checked[2]){&x, (int *)1};
+  //CHECK_ALL: int *d _Checked[2] = (int * _Checked[2]){&x, (int *)1};
   int *d0 = d[0];
   //CHECK: int *d0 = d[0];
 

clang/test/3C/definedType.c

@@ -57,6 +57,8 @@ baz **f[1];
 // CHECK_ALL: _Ptr<_Ptr<baz>> f _Checked[1] = {((void *)0)};
 baz (*g)[1];
 // CHECK_ALL: _Ptr<baz _Checked[1]> g = ((void *)0);
+baz *(*g2)[1];
+// CHECK_ALL: _Ptr<_Ptr<baz> _Checked[1]> g2 = ((void *)0);
 baz h[1][1];
 // CHECK_ALL: baz h _Checked[1] _Checked[1];
 

clang/test/3C/fptr_array.c

@@ -9,15 +9,15 @@
 
 void (*fs[2])(int *);
 void (*f)(int *);
-//CHECK_NOALL: void (* fs[2])(_Ptr<int>) = {((void *)0)};
+//CHECK_NOALL: void (*fs[2])(_Ptr<int>) = {((void *)0)};
 //CHECK_NOALL: void (*f)(_Ptr<int>) = ((void *)0);
 //CHECK_ALL: _Ptr<void (_Ptr<int>)> fs _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<void (_Ptr<int>)> f = ((void *)0);
 
 void (*gs[2])(int *);
 void g_impl(int *x) { x = 1; }
 void (*g)(int *) = g_impl;
-//CHECK_NOALL: void (* gs[2])(int * : itype(_Ptr<int>)) = {((void *)0)};
+//CHECK_NOALL: void (*gs[2])(int * : itype(_Ptr<int>)) = {((void *)0)};
 //CHECK_NOALL: void g_impl(int *x : itype(_Ptr<int>)) { x = 1; }
 //CHECK_NOALL: void (*g)(int * : itype(_Ptr<int>)) = g_impl;
 
@@ -30,21 +30,21 @@ void (*h)(void *);
 
 int *(*is[2])(void);
 int *(*i)(void);
-//CHECK_NOALL: _Ptr<int> (* is[2])(void) = {((void *)0)};
+//CHECK_NOALL: _Ptr<int> (*is[2])(void) = {((void *)0)};
 //CHECK_NOALL: _Ptr<int> (*i)(void) = ((void *)0);
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> is _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> i = ((void *)0);
 
 int *(**js[2])(void);
 int *(**j)(void);
-//CHECK_NOALL: _Ptr<int> (** js[2])(void) = {((void *)0)};
+//CHECK_NOALL: _Ptr<int> (**js[2])(void) = {((void *)0)};
 //CHECK_NOALL: _Ptr<int> (**j)(void) = ((void *)0);
 //CHECK_ALL: _Ptr<_Ptr<_Ptr<int> (void)>> js _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<_Ptr<_Ptr<int> (void)>> j = ((void *)0);
 
 int *(*ks[2][2])(void);
 int *(*k)(void);
-//CHECK_NOALL: _Ptr<int> (* ks[2][2])(void) = {((void *)0)};
+//CHECK_NOALL: _Ptr<int> (*ks[2][2])(void) = {((void *)0)};
 //CHECK_NOALL: _Ptr<int> (*k)(void) = ((void *)0);
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> ks _Checked[2] _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> k = ((void *)0);
@@ -53,6 +53,9 @@ void (* const l)(int *);
 //CHECK_NOALL: void (*const l)(_Ptr<int>) = ((void *)0);
 //CHECK_ALL: const _Ptr<void (_Ptr<int>)> l = ((void *)0);
 
+#define UNWRITABLE_MS_DECL void (*ms[2])(int *)
+UNWRITABLE_MS_DECL;
+
 void test(void){
   fs[1] = f;
   gs[1] = g;
@@ -63,4 +66,11 @@ void test(void){
   void (* const ls[1])(int *) = {l};
   //CHECK_NOALL: void (*const ls[1])(_Ptr<int>) = {l};
   //CHECK_ALL: const _Ptr<void (_Ptr<int>)> ls _Checked[1] = {l};
+
+  void (*(*pms)[2])(int *) = &ms;
+  //CHECK: _Ptr<void (*[2])(int *)> pms = &ms;
+
+  void (*(*apms[1])[2])(int *) = {&ms};
+  //CHECK_NOALL: void (*(*apms[1])[2])(int *) = {&ms};
+  //CHECK_ALL: _Ptr<void (*[2])(int *)> apms _Checked[1] = {&ms};
 }

clang/test/3C/itype_typedef.c

@@ -34,7 +34,7 @@ void test2(td2 a) {
 typedef int *td3;
 td3 test3() {
 //CHECK: typedef _Ptr<int> td3;
-//CHECK: int * test3(void) : itype(td3) {
+//CHECK: int *test3(void) : itype(td3) {
   return (int*) 1;
 }
 

clang/test/3C/itypes_for_extern.c

@@ -30,14 +30,8 @@ int *baz(int *a, int len, int *b) {
   return a;
 }
 
-// FIXME: The space between after the first star is needed for the idempotence
-//        test to pass. If there isn't a space there, 3c will add one when
-//        re-run on its original output. This should be fixed ideally in two
-//        ways.  First, the space shouldn't be added when not present in the
-//        original source, and second, the second conversion should not rewrite
-//        the declaration.
-void buz(int * (*f)(int *, int *)) {}
-//CHECK: void buz(int * ((*f)(int *, int *)) : itype(_Ptr<_Ptr<int> (_Ptr<int>, _Ptr<int>)>)) _Checked {}
+void buz(int *(*f)(int *, int *)) {}
+//CHECK: void buz(int *((*f)(int *, int *)) : itype(_Ptr<_Ptr<int> (_Ptr<int>, _Ptr<int>)>)) _Checked {}
 
 typedef int * int_star;
 void typedef_test(int_star p) {}

clang/test/3C/ptr_array.c

@@ -28,7 +28,7 @@ void test1(int *a) {
 
   int *b[1] = {a};
   //CHECK_NOALL: int *b[1] = {a};
-  //CHECK_ALL: int * b _Checked[1] = {a};
+  //CHECK_ALL: int *b _Checked[1] = {a};
 }
 
 /* Example from from the issue */
@@ -40,7 +40,7 @@ int *foo() {
   int z = 3;
   int *ptrs[4] = {&x, &y, &z, (int *)5};
   //CHECK_NOALL: int *ptrs[4] = {&x, &y, &z, (int *)5};
-  //CHECK_ALL: int * ptrs _Checked[4] = {&x, &y, &z, (int *)5};
+  //CHECK_ALL: int *ptrs _Checked[4] = {&x, &y, &z, (int *)5};
   int *ret;
   //CHECK: int *ret;
   for (int i = 0; i < 4; i++) {