Insert itypes correctly for constant sized arrays

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 D)
+    : Kind(K), OriginalType(T), Name(N), OriginalTypeWithName(D),
+      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,18 @@ 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 it must solve to either ARR or NTARR. This avoids
+  // losing bounds on array pointers, and converting constant sized arrays into
+  // pointers. but 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);
 }