Insert itypes correctly for constant sized arrays

clang/include/clang/3C/RewriteUtils.h

@@ -28,9 +28,7 @@ class DeclReplacement {
 
   std::string getReplacement() const { return Replacement; }
 
-  virtual SourceRange getSourceRange(SourceManager &SM) const {
-    return getDecl()->getSourceRange();
-  }
+  virtual SourceRange getSourceRange(SourceManager &SM) const;
 
   // Discriminator for LLVM-style RTTI (dyn_cast<> et al.).
   enum DRKind {

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

@@ -226,6 +226,8 @@ void getPrintfStringArgIndices(const clang::CallExpr *CE,
 int64_t getStmtIdWorkaround(const clang::Stmt *St,
                             const clang::ASTContext &Context);
 
+clang::SourceLocation getCheckedCAnnotationsEnd(const clang::Decl *D);
+
 // Shortcut for the getCustomDiagID + Report sequence to report a custom
 // diagnostic as we currently do in 3C.
 //

clang/lib/3C/ConstraintVariables.cpp

@@ -956,8 +956,16 @@ PointerVariableConstraint::mkString(Constraints &CS,
   }
 
   // No space after itype.
-  if (!EmittedName && !UseName.empty())
-    Ss << " " << UseName;
+  if (!EmittedName && !UseName.empty()) {
+    // Avoid emitting an extra space between the last star and the identifier
+    // when generating strings for unchecked pointers. This is used when
+    // emitting the unchecked part of typedef parameters with itypes. Ss.str()
+    // unfortunately copies contents of Ss. With C++20 there will be Ss.view()
+    // which I believe avoids the copy.
+    if (!Ss.str().empty() && Ss.str().back() != '*')
+      Ss << " ";
+    Ss << UseName;
+  }
 
   // Final array dropping.
   if (!ConstArrs.empty()) {
@@ -2234,7 +2242,11 @@ void FVComponentVariable::equateWithItype(ProgramInfo &I,
           ? "Internal constraint for generic function declaration, "
             "for which 3C currently does not support re-solving."
           : ReasonUnchangeable;
-  bool HasBounds = ExternalConstraint->srcHasBounds();
+  // If a pointer is an array pointer with declared bounds or is a constant size
+  // array, then it must solve to either ARR or NTARR. This avoids losing bounds
+  // on array pointers, and converting constant sized arrays into pointers.
+  bool MustBeArray =
+    ExternalConstraint->srcHasBounds() || ExternalConstraint->hasSomeSizedArr();
   bool HasItype = ExternalConstraint->srcHasItype();
   // If the type cannot change at all (ReasonUnchangeable2 is set), then we
   // constrain both the external and internal types to not change. Otherwise, if
@@ -2243,7 +2255,7 @@ void FVComponentVariable::equateWithItype(ProgramInfo &I,
   // allow the internal type to change so that the type can change from an itype
   // to fully checked.
   bool MustConstrainInternalType = !ReasonUnchangeable2.empty();
-  if (HasItype && (MustConstrainInternalType || HasBounds)) {
+  if (HasItype && (MustConstrainInternalType || MustBeArray)) {
     ExternalConstraint->equateWithItype(I, ReasonUnchangeable2, PSL);
     if (ExternalConstraint != InternalConstraint)
       linkInternalExternal(I, false);

clang/lib/3C/DeclRewriter.cpp

@@ -36,62 +36,58 @@ void DeclRewriter::buildItypeDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
                                   std::string &Type, std::string &IType,
                                   ProgramInfo &Info, ArrayBoundsRewriter &ABR) {
   const EnvironmentMap &Env = Info.getConstraints().getVariables();
-  bool IsTypedefVarUnchecked =
+  // True when the type of this variable is defined by a typedef, and the
+  // constraint variable representing the typedef solved to an unchecked type.
+  // In these cases, the typedef should be used in the unchecked part of the
+  // itype. The typedef is expanded using checked pointer types for the checked
+  // portion. In ItypesForExtern mode, typedefs are treated as unchecked because
+  // 3C will not rewrite the typedef to a checked type. Even if it solves to a
+  // checked type, it is not rewritten, so it remains unchecked in the converted
+  // code.
+  bool IsUncheckedTypedef =
     Defn->isTypedef() && (_3COpts.ItypesForExtern ||
                           !Defn->getTypedefVar()->isSolutionChecked(Env));
-  if (Defn->getFV()) {
-    // This declaration is for a function pointer. Writing itypes on function
-    // pointers is a little bit harder since the original type string will not
-    // work for the unchecked portion of the itype. We need to generate the
-    // unchecked type from the PVConstraint. The last argument of this call
-    // tells mkString to generate a string using unchecked types instead of
-    // checked types.
-    if (Defn->isTypedef() && !IsTypedefVarUnchecked)
-      Type = Defn->mkString(Info.getConstraints(),
-                            MKSTRING_OPTS(UnmaskTypedef = true,
-                                          ForItypeBase = true));
-    else
-      Type = Defn->mkString(Info.getConstraints(),
-                            MKSTRING_OPTS(ForItypeBase = true));
+  // True if this variable is defined by a typedef, and the constraint variable
+  // representing the typedef solves to a checked type. Notably not the negation
+  // of IsUncheckedTypedef because both require the variable type be defined
+  // by a typedef. The checked typedef is expanded using unchecked types in the
+  // unchecked portion of the itype. The typedef is used directly in the checked
+  // portion of the itype.
+  bool IsCheckedTypedef = Defn->isTypedef() && !IsUncheckedTypedef;
+  if (IsCheckedTypedef || Defn->getFV() || Defn->hasSomeSizedArr()) {
+    // Generate the type string from PVC if we need to unmask a typedef, this is
+    // a function pointer, or this is a constant size array. When unmasking a
+    // typedef, the expansion of the typedef does not exist in the original
+    // source, so it must be constructed. For function pointers, a function
+    // pointer appearing in the unchecked portion of an itype must contain an
+    // extra set of parenthesis (e.g. `void ((*f)())` instead of `void (f*)()`)
+    // for the declaration to parse correctly. For function pointers as well as
+    // constant size arrays, part of the type appears after the identifier
+    // (the parameter list and length respectively). This breaks the assumption
+    // in the next case that the declaration is type + identifier + itype.
+    Type = Defn->mkString(Info.getConstraints(),
+                          MKSTRING_OPTS(UnmaskTypedef = IsCheckedTypedef,
+                                        ForItypeBase = true));
   } else {
-    if (Defn->isTypedef() && !IsTypedefVarUnchecked)
-      Type = Defn->mkString(Info.getConstraints(),
-                            MKSTRING_OPTS(UnmaskTypedef = true,
-                                          ForItypeBase = true,
-                                          EmitName = false));
-    else
-      Type = Defn->getRewritableOriginalTy();
-
-    if (isa_and_nonnull<ParmVarDecl>(Decl)) {
-      if (Decl->getName().empty())
-        Type += Defn->getName();
-      else
-        Type += Decl->getNameAsString();
-    } else {
-      std::string Name = Defn->getName();
-      if (Name != RETVAR)
-        Type += Name;
-    }
+    // In the remaining cases, the unchecked portion of the itype is just the
+    // original type of the pointer.
+    // TODO: We could extract the original declaration string from the source
+    //       code instead of using the string stored in the constraint variable.
+    //       This would help preserve macros and potentially make this less
+    //       susceptible to other rewriting errors.
+    Type = Defn->getRewritableOriginalTy();
+
+    std::string Name = Defn->getName();
+    if (isa_and_nonnull<ParmVarDecl>(Decl) && !Decl->getName().empty())
+      Name = Decl->getNameAsString();
+    if (Name != RETVAR)
+     Type += Name;
   }
 
   IType = " : itype(";
-
-  if (IsTypedefVarUnchecked) {
-    // In -itypes-for-extern mode we do not rewrite typedefs to checked types.
-    // They are given a checked itype instead. The unchecked portion of the
-    // itype continues to use the original typedef, but the typedef in the
-    // checked portion is expanded and rewritten to use a checked type. This
-    // lets the typedef be used in unchecked code while still giving a checked
-    // type to the declaration so that it can be used in checked code.
-    // TODO: This could potentially be applied to typedef types even when the
-    //       flag is not passed to limit spread of wildness through typedefs.
-    IType += Defn->mkString(Info.getConstraints(),
-                            MKSTRING_OPTS(EmitName = false, ForItype = true,
-                                          UnmaskTypedef = true));
-  } else {
-    IType += Defn->mkString(Info.getConstraints(),
-                            MKSTRING_OPTS(EmitName = false, ForItype = true));
-  }
+  IType += Defn->mkString(Info.getConstraints(),
+                          MKSTRING_OPTS(EmitName = false, ForItype = true,
+                                        UnmaskTypedef = IsUncheckedTypedef));
   IType += ")" + ABR.getBoundsString(Defn, Decl, true);
 }
 
@@ -323,9 +319,8 @@ void DeclRewriter::rewriteSingleDecl(DeclReplacement *N, RSet &ToRewrite) {
       dyn_cast<TypedefDecl>(N->getDecl()) || isSingleDeclaration(N);
   assert("Declaration is not a single declaration." && IsSingleDecl);
   // This is the easy case, we can rewrite it locally, at the declaration.
-  // TODO why do we call getDecl() and getSourceRange() directly,
-  // TODO as opposed to getSourceRange()?
-  SourceRange TR = N->getDecl()->getSourceRange();
+  SourceManager &SM = N->getDecl()->getASTContext().getSourceManager();
+  SourceRange TR = N->getSourceRange(SM);
   doDeclRewrite(TR, N);
 }
 

clang/lib/3C/RewriteUtils.cpp

@@ -428,6 +428,17 @@ class TypeArgumentAdder : public clang::RecursiveASTVisitor<TypeArgumentAdder> {
   }
 };
 
+SourceRange DeclReplacement::getSourceRange(SourceManager &SM) const {
+  SourceRange SR = getDecl()->getSourceRange();
+  SourceLocation OldEnd = SR.getEnd();
+  SourceLocation NewEnd  = getCheckedCAnnotationsEnd(getDecl());
+  if (NewEnd.isValid() &&
+      (!OldEnd.isValid() || SM.isBeforeInTranslationUnit(OldEnd, NewEnd)))
+    SR.setEnd(NewEnd);
+
+  return SR;
+}
+
 SourceRange FunctionDeclReplacement::getSourceRange(SourceManager &SM) const {
   SourceLocation Begin = RewriteGeneric ? getDeclBegin(SM) :
                       (RewriteReturn ? getReturnBegin(SM) : getParamBegin(SM));
@@ -503,24 +514,12 @@ SourceLocation FunctionDeclReplacement::getDeclEnd(SourceManager &SM) const {
     }
   }
 
-  // If there's a bounds expression, this comes after the right paren of the
-  // function declaration parameter list.
-  if (auto *BoundsE = Decl->getBoundsExpr()) {
-    SourceLocation BoundsEnd = BoundsE->getEndLoc();
-    if (BoundsEnd.isValid() &&
-        (!End.isValid() || SM.isBeforeInTranslationUnit(End, BoundsEnd)))
-      End = BoundsEnd;
-  }
-
-  // If there's an itype, this also comes after the right paren. In the case
-  // that there is both a bounds expression and an itype, we need check
-  // which is later in the file and use that as the declaration end.
-  if (auto *InteropE = Decl->getInteropTypeExpr()) {
-    SourceLocation InteropEnd = InteropE->getEndLoc();
-    if (InteropEnd.isValid() &&
-        (!End.isValid() || SM.isBeforeInTranslationUnit(End, InteropEnd)))
-      End = InteropEnd;
-  }
+  // If there's a bounds or interop type expression, this will come after the
+  // right paren of the function declaration parameter list.
+  SourceLocation AnnotationsEnd = getCheckedCAnnotationsEnd(Decl);
+  if (AnnotationsEnd.isValid() &&
+      (!End.isValid() || SM.isBeforeInTranslationUnit(End, AnnotationsEnd)))
+    End = AnnotationsEnd;
 
   // SourceLocations are weird and turn up invalid for reasons I don't
   // understand. Fallback to extracting r paren location from source

clang/lib/3C/Utils.cpp

@@ -566,3 +566,28 @@ int64_t getStmtIdWorkaround(const Stmt *St, const ASTContext &Context) {
   // (alignof(Stmt) - 1) before dividing.
   return Context.getAllocator().identifyKnownObject(St);
 }
+
+// Get the SourceLocation for the end of any Checked C bounds or interop type
+// annotations on a declaration. Returns an invalid source location if no
+// Checked C annotations are present.
+SourceLocation getCheckedCAnnotationsEnd(const Decl *D) {
+  SourceManager &SM = D->getASTContext().getSourceManager();
+  SourceLocation End;
+
+  // Update the current end SourceLocation to the new SourceLocation if the new
+  // location is valid and comes after the current end location.
+  auto UpdateEnd = [&SM, &End](SourceLocation SL) {
+    if (SL.isValid() &&
+        (!End.isValid() || SM.isBeforeInTranslationUnit(End, SL)))
+      End = SL;
+  };
+
+  if (auto *DD = dyn_cast<DeclaratorDecl>(D)) {
+    if (auto *InteropE = DD->getInteropTypeExpr())
+      UpdateEnd(InteropE->getEndLoc());
+    if (auto *BoundsE = DD->getBoundsExpr())
+      UpdateEnd(BoundsE->getEndLoc());
+  }
+
+  return End;
+}

clang/test/3C/generalize.c

@@ -48,7 +48,7 @@ void nameless(void *a, char *b)
 {
   a = 1; // make it unsafe
 }
-// CHECK: void nameless(void * a, _Ptr<char> b);
+// CHECK: void nameless(void *a, _Ptr<char> b);
 // CHECK: void nameless(void *a, _Ptr<char> b)
 
 // Safe functions should be upgraded from "_Itype_for_any" to "_For_any"

clang/test/3C/itypes_for_extern.c

@@ -92,3 +92,45 @@ void has_itype0(int *a : itype(_Ptr<int>)) { a = 1; }
 
 void has_itype1(int *a : itype(_Ptr<int>)) { a = 0; }
 //CHECK: void has_itype1(int *a : itype(_Ptr<int>)) _Checked { a = 0; }
+
+// Test rewriting itypes for constant sized arrays. As with function pointers,
+// part of the type (the array size) occurs after the name of the variable
+// being declared. This complicates rewriting. These examples caused errors in
+// libjepg.
+
+int const_arr0[10];
+//CHECK_ALL: int const_arr0[10] : itype(int _Checked[10]);
+//CHECK_NOALL: int const_arr0[10];
+
+int *const_arr1[10];
+//CHECK_ALL: int * const_arr1[10] : itype(_Ptr<int> _Checked[10]) = {((void *)0)};
+//CHECK_NOALL: int *const_arr1[10];
+
+// Itypes for constants sized arrays when there a declaration with and without
+// a parameter list take slightly different paths that need to be tested. If
+// there is no parameter list, then the unchecked component of the itype can't
+// be copied from the declaration, and it instead must be generated from the
+// constraint variable.
+
+void const_arr_fn();
+void const_arr_fn(int a[10]) {}
+//CHECK_ALL: void const_arr_fn(int a[10] : itype(int _Checked[10]));
+//CHECK_ALL: void const_arr_fn(int a[10] : itype(int _Checked[10])) _Checked {}
+
+// Rewriting an existing itype or bounds expression on a global variable. Doing
+// this correctly requires replacing text until the end of the Checked C
+// annotation expression.
+int *a : itype(_Ptr<int>);
+int **b : itype(_Ptr<int *>);
+int *c : count(2);
+int **d : count(2);
+int **e : itype(_Array_ptr<int *>) count(2);
+int **f : count(2) itype(_Array_ptr<int *>);
+int **g : count(2) itype(_Array_ptr<int *>) = 0;
+//CHECK: int *a : itype(_Ptr<int>);
+//CHECK: int **b : itype(_Ptr<_Ptr<int>>) = ((void *)0);
+//CHECK: int *c : count(2);
+//CHECK: int **d : itype(_Array_ptr<_Ptr<int>>) count(2) = ((void *)0);
+//CHECK: int **e : itype(_Array_ptr<_Ptr<int>>) count(2) = ((void *)0);
+//CHECK: int **f : itype(_Array_ptr<_Ptr<int>>) count(2) = ((void *)0);
+//CHECK: int **g : itype(_Array_ptr<_Ptr<int>>) count(2) = 0;