Insert itypes correctly for constant sized arrays (#702)

Given the declaration of a global variable `int foo[10]` converted with
`3c -itypes-for-extern -alltypes` the resulting declaration incorrectly moved
the length of the array before identifier. This changes fixes the rewriting to
correctly be `int foo[10] : itype(int _Checked[10])`. This also fixes itype
insertion for pointers to constant size arrays.

Another change is included to pass idempotence checks for the new tests added.
This change expands the source range replaced for global variable declarations
to include the itype or bounds declarations if present.

Author: john-h-kastner

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

@@ -52,6 +52,9 @@ struct MkStringOpts {
 };
 #define MKSTRING_OPTS(...) PACK_OPTS(MkStringOpts, __VA_ARGS__)
 
+// Name for function return, for debugging
+#define RETVAR "$ret"
+
 // Base class for ConstraintVariables. A ConstraintVariable can either be a
 // PointerVariableConstraint or a FunctionVariableConstraint. The difference
 // is that FunctionVariableConstraints have constraints on the return value
@@ -66,9 +69,23 @@ class ConstraintVariable {
   ConstraintVariableKind Kind;
 
 protected:
+  // A string representation for the type of this variable. Note that for
+  // complex types (e.g., function pointer, constant sized arrays), you cannot
+  // concatenate the type string with an identifier and expect to obtain a valid
+  // variable declaration.
   std::string OriginalType;
-  // Underlying name of the C variable this ConstraintVariable represents.
+  // Underlying name of the C variable this ConstraintVariable represents. This
+  // is not always a valid C identifier. It will be empty if no name was given
+  // (e.g., some parameter declarations). It will be the predefined string
+  // "$ret" when the ConstraintVariable represents a function return. It may
+  // take other values if the ConstraintVariable does not represent a C
+  // variable (e.g., explict casts and compound literals) .
   std::string Name;
+  // The combination of the type and name of the represented C variable. The
+  // combination is handled by clang library routines, so complex types
+  // like function pointers and constant size are handled correctly. See
+  // comments on Name for when name should be a valid identifier.
+  std::string OriginalTypeWithName;
   // Set of constraint variables that have been constrained due to a
   // bounds-safe interface (itype). They are remembered as being constrained
   // so that later on we do not introduce a spurious constraint
@@ -85,9 +102,15 @@ class ConstraintVariable {
   bool IsForDecl;
 
   // Only subclasses should call this
-  ConstraintVariable(ConstraintVariableKind K, std::string T, std::string N)
-      : Kind(K), OriginalType(T), Name(N), HasEqArgumentConstraints(false),
-        ValidBoundsKey(false), IsForDecl(false) {}
+  ConstraintVariable(ConstraintVariableKind K, std::string T, std::string N,
+                     std::string TN)
+    : Kind(K), OriginalType(T), Name(N), OriginalTypeWithName(TN),
+      HasEqArgumentConstraints(false), ValidBoundsKey(false),
+      IsForDecl(false) {}
+
+  ConstraintVariable(ConstraintVariableKind K, QualType QT, std::string N)
+    : ConstraintVariable(K, qtyToStr(QT), N,
+                         qtyToStr(QT, N == RETVAR ? "" : N)) {}
 
 public:
   // Create a "for-rewriting" representation of this ConstraintVariable.
@@ -166,6 +189,7 @@ class ConstraintVariable {
   // Get the original type string that can be directly
   // used for rewriting.
   std::string getRewritableOriginalTy() const;
+  std::string getOriginalTypeWithName() const;
   std::string getName() const { return Name; }
 
   void setValidDecl() { IsForDecl = true; }
@@ -378,7 +402,7 @@ class PointerVariableConstraint : public ConstraintVariable {
   // other fields are initialized to default values. This is used to construct
   // variables for non-pointer expressions.
   PointerVariableConstraint(std::string Name) :
-    ConstraintVariable(PointerVariable, "", Name), FV(nullptr),
+    ConstraintVariable(PointerVariable, "", Name, ""), FV(nullptr),
     SrcHasItype(false), PartOfFuncPrototype(false), Parent(nullptr),
     SourceGenericIndex(-1), InferredGenericIndex(-1),
     IsZeroWidthArray(false), IsTypedef(false),
@@ -530,8 +554,6 @@ class PointerVariableConstraint : public ConstraintVariable {
 };
 
 typedef PointerVariableConstraint PVConstraint;
-// Name for function return, for debugging
-#define RETVAR "$ret"
 
 // This class contains a pair of PVConstraints that represent an internal and
 // external view of a variable for use as the parameter and return constraints
@@ -622,9 +644,10 @@ class FunctionVariableConstraint : public ConstraintVariable {
                              const clang::ASTContext &C);
   FunctionVariableConstraint(clang::TypedefDecl *D, ProgramInfo &I,
                              const clang::ASTContext &C);
-  FunctionVariableConstraint(const clang::Type *Ty, clang::DeclaratorDecl *D,
-                             std::string N, ProgramInfo &I,
-                             const clang::ASTContext &C,
+
+  FunctionVariableConstraint(const clang::QualType Ty,
+                             clang::DeclaratorDecl *D, std::string N,
+                             ProgramInfo &I, const clang::ASTContext &C,
                              TypeSourceInfo *TSI = nullptr);
 
   PVConstraint *getExternalReturn() const {

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

@@ -228,6 +228,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

@@ -46,6 +46,12 @@ std::string ConstraintVariable::getRewritableOriginalTy() const {
   return OrigTyString;
 }
 
+std::string ConstraintVariable::getOriginalTypeWithName() const {
+  if (Name == RETVAR)
+    return getRewritableOriginalTy();
+  return OriginalTypeWithName;
+}
+
 PointerVariableConstraint *PointerVariableConstraint::getWildPVConstraint(
     Constraints &CS, const std::string &Rsn, PersistentSourceLoc *PSL) {
   auto *WildPVC = new PointerVariableConstraint("wildvar");
@@ -121,9 +127,9 @@ PointerVariableConstraint *PointerVariableConstraint::addAtomPVConstraint(
 PointerVariableConstraint::PointerVariableConstraint(
     PointerVariableConstraint *Ot)
   : ConstraintVariable(ConstraintVariable::PointerVariable, Ot->OriginalType,
-                       Ot->Name), BaseType(Ot->BaseType), Vars(Ot->Vars),
-    SrcVars(Ot->SrcVars), FV(Ot->FV), QualMap(Ot->QualMap),
-    ArrSizes(Ot->ArrSizes), ArrSizeStrs(Ot->ArrSizeStrs),
+                       Ot->Name, Ot->OriginalTypeWithName),
+    BaseType(Ot->BaseType), Vars(Ot->Vars), SrcVars(Ot->SrcVars), FV(Ot->FV),
+    QualMap(Ot->QualMap), ArrSizes(Ot->ArrSizes), ArrSizeStrs(Ot->ArrSizeStrs),
     SrcHasItype(Ot->SrcHasItype), ItypeStr(Ot->ItypeStr),
     PartOfFuncPrototype(Ot->PartOfFuncPrototype), Parent(Ot),
     BoundsAnnotationStr(Ot->BoundsAnnotationStr),
@@ -206,7 +212,7 @@ PointerVariableConstraint::PointerVariableConstraint(
     const ASTContext &C, std::string *InFunc, int ForceGenericIndex,
     bool PotentialGeneric,
     bool VarAtomForChecked, TypeSourceInfo *TSInfo, const QualType &ITypeT)
-    : ConstraintVariable(ConstraintVariable::PointerVariable, qtyToStr(QT), N),
+    : ConstraintVariable(ConstraintVariable::PointerVariable, QT, N),
       FV(nullptr), SrcHasItype(false), PartOfFuncPrototype(InFunc != nullptr),
       Parent(nullptr) {
   QualType QTy = QT;
@@ -512,7 +518,7 @@ PointerVariableConstraint::PointerVariableConstraint(
     //    tn fname = ...,
     // where tn is the typedef'ed type name.
     // There is possibly something more elegant to do in the code here.
-    FV = new FVConstraint(Ty, IsDeclTy ? D : nullptr, IsTypedef ? "" : N, I, C,
+    FV = new FVConstraint(QTy, IsDeclTy ? D : nullptr, IsTypedef ? "" : N, I, C,
                           TSInfo);
 
   // Get a string representing the type without pointer and array indirection.
@@ -965,8 +971,11 @@ PointerVariableConstraint::mkString(Constraints &CS,
   }
 
   // No space after itype.
-  if (!EmittedName && !UseName.empty())
-    Ss << " " << UseName;
+  if (!EmittedName && !UseName.empty()) {
+    if (!StringRef(Ss.str()).endswith("*"))
+      Ss << " ";
+    Ss << UseName;
+  }
 
   // Final array dropping.
   if (!ConstArrs.empty()) {
@@ -1004,10 +1013,10 @@ const CVarSet &PVConstraint::getArgumentConstraints() const {
 
 FunctionVariableConstraint::FunctionVariableConstraint(FVConstraint *Ot)
   : ConstraintVariable(ConstraintVariable::FunctionVariable, Ot->OriginalType,
-                       Ot->getName()), ReturnVar(Ot->ReturnVar),
-    ParamVars(Ot->ParamVars), FileName(Ot->FileName), Hasproto(Ot->Hasproto),
-    Hasbody(Ot->Hasbody), IsStatic(Ot->IsStatic), Parent(Ot),
-    IsFunctionPtr(Ot->IsFunctionPtr), TypeParams(Ot->TypeParams) {
+                       Ot->getName(), Ot->OriginalTypeWithName),
+    ReturnVar(Ot->ReturnVar), ParamVars(Ot->ParamVars), FileName(Ot->FileName),
+    Hasproto(Ot->Hasproto), Hasbody(Ot->Hasbody), IsStatic(Ot->IsStatic),
+    Parent(Ot), IsFunctionPtr(Ot->IsFunctionPtr), TypeParams(Ot->TypeParams) {
   this->HasEqArgumentConstraints = Ot->HasEqArgumentConstraints;
 }
 
@@ -1019,22 +1028,23 @@ FunctionVariableConstraint::FunctionVariableConstraint(DeclaratorDecl *D,
                                                        ProgramInfo &I,
                                                        const ASTContext &C)
     : FunctionVariableConstraint(
-          D->getType().getTypePtr(), D,
+          D->getType(), D,
           D->getDeclName().isIdentifier() ? std::string(D->getName()) : "", I,
           C, D->getTypeSourceInfo()) {}
 
 FunctionVariableConstraint::FunctionVariableConstraint(TypedefDecl *D,
                                                        ProgramInfo &I,
                                                        const ASTContext &C)
-    : FunctionVariableConstraint(D->getUnderlyingType().getTypePtr(), nullptr,
+    : FunctionVariableConstraint(D->getUnderlyingType(), nullptr,
                                  D->getNameAsString(), I, C,
                                  D->getTypeSourceInfo()) {}
 
 FunctionVariableConstraint::FunctionVariableConstraint(
-    const Type *Ty, DeclaratorDecl *D, std::string N, ProgramInfo &I,
+    const QualType QT, DeclaratorDecl *D, std::string N, ProgramInfo &I,
     const ASTContext &Ctx, TypeSourceInfo *TSInfo)
-    : ConstraintVariable(ConstraintVariable::FunctionVariable, tyToStr(Ty), N),
+    : ConstraintVariable(ConstraintVariable::FunctionVariable, QT, N),
       Parent(nullptr) {
+  const Type *Ty = QT.getTypePtr();
   QualType RT, RTIType;
   Hasproto = false;
   Hasbody = false;
@@ -2004,8 +2014,10 @@ void PointerVariableConstraint::mergeDeclaration(ConstraintVariable *FromCV,
          "Merging error, pointer depth change");
   Vars = NewVAtoms;
   SrcVars = NewSrcAtoms;
-  if (Name.empty())
+  if (Name.empty()) {
     Name = From->Name;
+    OriginalTypeWithName = From->OriginalTypeWithName;
+  }
   SrcHasItype = SrcHasItype || From->SrcHasItype;
   if (!From->ItypeStr.empty())
     ItypeStr = From->ItypeStr;
@@ -2265,16 +2277,20 @@ 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();
   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
-  // the variable has bounds, then we don't want the checked (external) portion
-  // of the type to change because that could blow away the bounds, but we still
-  // allow the internal type to change so that the type can change from an itype
-  // to fully checked.
+  // constrain both the external and internal types to not change.
   bool MustConstrainInternalType = !ReasonUnchangeable2.empty();
-  if (HasItype && (MustConstrainInternalType || HasBounds)) {
+  // Otherwise, if a pointer is an array pointer with declared bounds or is a
+  // constant size array, then we want to ensure the external type continues to
+  // solve to ARR or NTARR; see the comment on
+  // ConstraintVariable::equateWithItype re how this is achieved. This avoids
+  // losing bounds on array pointers, and converting constant sized arrays into
+  // pointers. We still allow the internal type to change so that the type can
+  // change from an itype to fully checked.
+  bool MustBeArray =
+    ExternalConstraint->srcHasBounds() || ExternalConstraint->hasSomeSizedArr();
+  if (HasItype && (MustConstrainInternalType || MustBeArray)) {
     ExternalConstraint->equateWithItype(I, ReasonUnchangeable2, PSL);
     if (ExternalConstraint != InternalConstraint)
       linkInternalExternal(I, false);

clang/lib/3C/DeclRewriter.cpp

@@ -36,62 +36,59 @@ 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;
+
+  // It should in principle be possible to always generate the unchecked portion
+  // of the itype by going through mkString. For practical reason, this doesn't
+  // always work, so we instead use the original type string as generated by
+  // clang so that we emit valid syntax in more cases. For examples and
+  // discussion refer to issue correctcomputation/checkedc-clang#703.
+  if (IsCheckedTypedef || Defn->getFV()) {
+    // 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. qtyToStr (which is used by
+    // getOriginalTypeWithName) does not support adding these parentheses.
+    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));
+    // In the remaining cases, the unchecked portion of the itype is just the
+    // original type of the pointer. The first branch tries to generate the type
+    // using the type and name for this specific declaration. This is important
+    // because it avoids changing parameter names, particularly in cases where
+    // multiple functions sharing the same name are defined in different
+    // translation units.
+    if (isa_and_nonnull<ParmVarDecl>(Decl) && !Decl->getName().empty())
+      Type = qtyToStr(Decl->getType(), Decl->getNameAsString());
     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;
-    }
+      Type = Defn->getOriginalTypeWithName();
   }
 
   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 +320,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);
 }