Implement current CWG direction for support of arrays of unknown bounds in

constant expressions.

We permit array-to-pointer decay on such arrays, but disallow pointer
arithmetic (since we do not know whether it will have defined behavior).

This is based on r311970 and r301822 (the former by me and the latter by Robert
Haberlach). Between then and now, two things have changed: we have committee
feedback indicating that this is indeed the right direction, and the code
broken by this change has been fixed.

This is necessary in C++17 to continue accepting certain forms of non-type
template argument involving arrays of unknown bound.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@316245 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/DiagnosticASTKinds.td

@@ -127,6 +127,10 @@ def note_constexpr_access_null : Note<
 def note_constexpr_access_past_end : Note<
   "%select{read of|assignment to|increment of|decrement of}0 "
   "dereferenced one-past-the-end pointer is not allowed in a constant expression">;
+def note_constexpr_access_unsized_array : Note<
+  "%select{read of|assignment to|increment of|decrement of}0 "
+  "pointer to element of array without known bound "
+  "is not allowed in a constant expression">;
 def note_constexpr_access_inactive_union_member : Note<
   "%select{read of|assignment to|increment of|decrement of}0 "
   "member %1 of union with %select{active member %3|no active member}2 "
@@ -154,6 +158,11 @@ def note_constexpr_baa_insufficient_alignment : Note<
 def note_constexpr_baa_value_insufficient_alignment : Note<
   "value of the aligned pointer (%0) is not a multiple of the asserted %1 "
   "%plural{1:byte|:bytes}1">;
+def note_constexpr_unsupported_unsized_array : Note<
+  "array-to-pointer decay of array member without known bound is not supported">;
+def note_constexpr_unsized_array_indexed : Note<
+  "indexing of array without known bound is not allowed "
+  "in a constant expression">;
 
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,

include/clang/Basic/DiagnosticIDs.h

@@ -34,7 +34,7 @@ namespace clang {
       DIAG_SIZE_SERIALIZATION =  120,
       DIAG_SIZE_LEX           =  400,
       DIAG_SIZE_PARSE         =  500,
-      DIAG_SIZE_AST           =  110,
+      DIAG_SIZE_AST           =  150,
       DIAG_SIZE_COMMENT       =  100,
       DIAG_SIZE_CROSSTU       =  100,
       DIAG_SIZE_SEMA          = 3500,

lib/AST/ExprConstant.cpp

@@ -62,7 +62,13 @@ namespace {
   static QualType getType(APValue::LValueBase B) {
     if (!B) return QualType();
     if (const ValueDecl *D = B.dyn_cast<const ValueDecl*>())
-      return D->getType();
+      // FIXME: It's unclear where we're supposed to take the type from, and
+      // this actually matters for arrays of unknown bound. Using the type of
+      // the most recent declaration isn't clearly correct in general. Eg:
+      //
+      // extern int arr[]; void f() { extern int arr[3]; };
+      // constexpr int *p = &arr[1]; // valid?
+      return cast<ValueDecl>(D->getMostRecentDecl())->getType();
 
     const Expr *Base = B.get<const Expr*>();
 
@@ -141,6 +147,12 @@ namespace {
     return E && E->getType()->isPointerType() && tryUnwrapAllocSizeCall(E);
   }
 
+  /// The bound to claim that an array of unknown bound has.
+  /// The value in MostDerivedArraySize is undefined in this case. So, set it
+  /// to an arbitrary value that's likely to loudly break things if it's used.
+  static const uint64_t AssumedSizeForUnsizedArray =
+      std::numeric_limits<uint64_t>::max() / 2;
+
   /// Determines if an LValue with the given LValueBase will have an unsized
   /// array in its designator.
   /// Find the path length and type of the most-derived subobject in the given
@@ -148,7 +160,8 @@ namespace {
   static unsigned
   findMostDerivedSubobject(ASTContext &Ctx, APValue::LValueBase Base,
                            ArrayRef<APValue::LValuePathEntry> Path,
-                           uint64_t &ArraySize, QualType &Type, bool &IsArray) {
+                           uint64_t &ArraySize, QualType &Type, bool &IsArray,
+                           bool &FirstEntryIsUnsizedArray) {
     // This only accepts LValueBases from APValues, and APValues don't support
     // arrays that lack size info.
     assert(!isBaseAnAllocSizeCall(Base) &&
@@ -158,12 +171,18 @@ namespace {
 
     for (unsigned I = 0, N = Path.size(); I != N; ++I) {
       if (Type->isArrayType()) {
-        const ConstantArrayType *CAT =
+        const ArrayType *AT = Ctx.getAsArrayType(Type);
-            cast<ConstantArrayType>(Ctx.getAsArrayType(Type));
+        Type = AT->getElementType();
-        Type = CAT->getElementType();
-        ArraySize = CAT->getSize().getZExtValue();
         MostDerivedLength = I + 1;
         IsArray = true;
+
+        if (auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
+          ArraySize = CAT->getSize().getZExtValue();
+        } else {
+          assert(I == 0 && "unexpected unsized array designator");
+          FirstEntryIsUnsizedArray = true;
+          ArraySize = AssumedSizeForUnsizedArray;
+        }
       } else if (Type->isAnyComplexType()) {
         const ComplexType *CT = Type->castAs<ComplexType>();
         Type = CT->getElementType();
@@ -246,10 +265,12 @@ namespace {
         Entries.insert(Entries.end(), VEntries.begin(), VEntries.end());
         if (V.getLValueBase()) {
           bool IsArray = false;
+          bool FirstIsUnsizedArray = false;
           MostDerivedPathLength = findMostDerivedSubobject(
               Ctx, V.getLValueBase(), V.getLValuePath(), MostDerivedArraySize,
-              MostDerivedType, IsArray);
+              MostDerivedType, IsArray, FirstIsUnsizedArray);
           MostDerivedIsArrayElement = IsArray;
+          FirstEntryIsAnUnsizedArray = FirstIsUnsizedArray;
         }
       }
     }
@@ -318,7 +339,7 @@ namespace {
       // The value in MostDerivedArraySize is undefined in this case. So, set it
       // to an arbitrary value that's likely to loudly break things if it's
       // used.
-      MostDerivedArraySize = std::numeric_limits<uint64_t>::max() / 2;
+      MostDerivedArraySize = AssumedSizeForUnsizedArray;
       MostDerivedPathLength = Entries.size();
     }
     /// Update this designator to refer to the given base or member of this
@@ -350,6 +371,7 @@ namespace {
       MostDerivedArraySize = 2;
       MostDerivedPathLength = Entries.size();
     }
+    void diagnoseUnsizedArrayPointerArithmetic(EvalInfo &Info, const Expr *E);
     void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
                                    const APSInt &N);
     /// Add N to the address of this subobject.
@@ -357,6 +379,7 @@ namespace {
       if (Invalid || !N) return;
       uint64_t TruncatedN = N.extOrTrunc(64).getZExtValue();
       if (isMostDerivedAnUnsizedArray()) {
+        diagnoseUnsizedArrayPointerArithmetic(Info, E);
         // Can't verify -- trust that the user is doing the right thing (or if
         // not, trust that the caller will catch the bad behavior).
         // FIXME: Should we reject if this overflows, at least?
@@ -1094,9 +1117,19 @@ bool SubobjectDesignator::checkSubobject(EvalInfo &Info, const Expr *E,
     setInvalid();
     return false;
   }
+  // Note, we do not diagnose if isMostDerivedAnUnsizedArray(), because there
+  // must actually be at least one array element; even a VLA cannot have a
+  // bound of zero. And if our index is nonzero, we already had a CCEDiag.
   return true;
 }
 
+void SubobjectDesignator::diagnoseUnsizedArrayPointerArithmetic(EvalInfo &Info,
+                                                                const Expr *E) {
+  Info.CCEDiag(E, diag::note_constexpr_unsized_array_indexed);
+  // Do not set the designator as invalid: we can represent this situation,
+  // and correct handling of __builtin_object_size requires us to do so.
+}
+
 void SubobjectDesignator::diagnosePointerArithmetic(EvalInfo &Info,
                                                     const Expr *E,
                                                     const APSInt &N) {
@@ -1240,8 +1273,6 @@ namespace {
                     IsNullPtr);
       else {
         assert(!InvalidBase && "APValues can't handle invalid LValue bases");
-        assert(!Designator.FirstEntryIsAnUnsizedArray &&
-               "Unsized array with a valid base?");
         V = APValue(Base, Offset, Designator.Entries,
                     Designator.IsOnePastTheEnd, CallIndex, IsNullPtr);
       }
@@ -1314,10 +1345,14 @@ namespace {
       if (checkSubobject(Info, E, isa<FieldDecl>(D) ? CSK_Field : CSK_Base))
         Designator.addDeclUnchecked(D, Virtual);
     }
-    void addUnsizedArray(EvalInfo &Info, QualType ElemTy) {
+    void addUnsizedArray(EvalInfo &Info, const Expr *E, QualType ElemTy) {
-      assert(Designator.Entries.empty() && getType(Base)->isPointerType());
+      if (!Designator.Entries.empty()) {
-      assert(isBaseAnAllocSizeCall(Base) &&
+        Info.CCEDiag(E, diag::note_constexpr_unsupported_unsized_array);
-             "Only alloc_size bases can have unsized arrays");
+        Designator.setInvalid();
+        return;
+      }
+
+      assert(getType(Base)->isPointerType() || getType(Base)->isArrayType());
       Designator.FirstEntryIsAnUnsizedArray = true;
       Designator.addUnsizedArrayUnchecked(ElemTy);
     }
@@ -2624,9 +2659,11 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
   if (Sub.Invalid)
     // A diagnostic will have already been produced.
     return handler.failed();
-  if (Sub.isOnePastTheEnd()) {
+  if (Sub.isOnePastTheEnd() || Sub.isMostDerivedAnUnsizedArray()) {
     if (Info.getLangOpts().CPlusPlus11)
-      Info.FFDiag(E, diag::note_constexpr_access_past_end)
+      Info.FFDiag(E, Sub.isOnePastTheEnd()
+                         ? diag::note_constexpr_access_past_end
+                         : diag::note_constexpr_access_unsized_array)
           << handler.AccessKind;
     else
       Info.FFDiag(E);
@@ -5487,7 +5524,7 @@ static bool evaluateLValueAsAllocSize(EvalInfo &Info, APValue::LValueBase Base,
   Result.setInvalid(E);
 
   QualType Pointee = E->getType()->castAs<PointerType>()->getPointeeType();
-  Result.addUnsizedArray(Info, Pointee);
+  Result.addUnsizedArray(Info, E, Pointee);
   return true;
 }
 
@@ -5697,7 +5734,8 @@ bool PointerExprEvaluator::VisitCastExpr(const CastExpr* E) {
       return true;
     }
   }
-  case CK_ArrayToPointerDecay:
+
+  case CK_ArrayToPointerDecay: {
     if (SubExpr->isGLValue()) {
       if (!evaluateLValue(SubExpr, Result))
         return false;
@@ -5708,12 +5746,13 @@ bool PointerExprEvaluator::VisitCastExpr(const CastExpr* E) {
         return false;
     }
     // The result is a pointer to the first element of the array.
-    if (const ConstantArrayType *CAT
+    auto *AT = Info.Ctx.getAsArrayType(SubExpr->getType());
-          = Info.Ctx.getAsConstantArrayType(SubExpr->getType()))
+    if (auto *CAT = dyn_cast<ConstantArrayType>(AT))
       Result.addArray(Info, E, CAT);
     else
-      Result.Designator.setInvalid();
+      Result.addUnsizedArray(Info, E, AT->getElementType());
     return true;
+  }
 
   case CK_FunctionToPointerDecay:
     return evaluateLValue(SubExpr, Result);
@@ -5780,7 +5819,7 @@ bool PointerExprEvaluator::visitNonBuiltinCallExpr(const CallExpr *E) {
 
   Result.setInvalid(E);
   QualType PointeeTy = E->getType()->castAs<PointerType>()->getPointeeType();
-  Result.addUnsizedArray(Info, PointeeTy);
+  Result.addUnsizedArray(Info, E, PointeeTy);
   return true;
 }
 
@@ -7341,7 +7380,8 @@ static const Expr *ignorePointerCastsAndParens(const Expr *E) {
 /// Please note: this function is specialized for how __builtin_object_size
 /// views "objects".
 ///
-/// If this encounters an invalid RecordDecl, it will always return true.
+/// If this encounters an invalid RecordDecl or otherwise cannot determine the
+/// correct result, it will always return true.
 static bool isDesignatorAtObjectEnd(const ASTContext &Ctx, const LValue &LVal) {
   assert(!LVal.Designator.Invalid);
 
@@ -7372,9 +7412,8 @@ static bool isDesignatorAtObjectEnd(const ASTContext &Ctx, const LValue &LVal) {
   unsigned I = 0;
   QualType BaseType = getType(Base);
   if (LVal.Designator.FirstEntryIsAnUnsizedArray) {
-    assert(isBaseAnAllocSizeCall(Base) &&
+    // If we don't know the array bound, conservatively assume we're looking at
-           "Unsized array in non-alloc_size call?");
+    // the final array element.
-    // If this is an alloc_size base, we should ignore the initial array index
     ++I;
     BaseType = BaseType->castAs<PointerType>()->getPointeeType();
   }

test/SemaCXX/constant-expression-cxx11.cpp

@@ -604,20 +604,31 @@ static_assert(NATDCArray{}[1][1].n == 0, "");
 
 }
 
-// Tests for indexes into arrays of unknown bounds.
+// Per current CWG direction, we reject any cases where pointer arithmetic is
+// not statically known to be valid.
 namespace ArrayOfUnknownBound {
-  // This is a corner case of the language where it's not clear whether this
+  extern int arr[];
-  // should be an error: When we see the initializer for Z::a, the bounds of
+  constexpr int *a = arr;
-  // Z::b aren't known yet, but they will be known by the end of the translation
+  constexpr int *b = &arr[0];
-  // unit, so the compiler can in theory check the indexing into Z::b.
+  static_assert(a == b, "");
-  // For the time being, as long as this is unclear, we want to make sure that
+  constexpr int *c = &arr[1]; // expected-error {{constant}} expected-note {{indexing of array without known bound}}
-  // this compiles.
+  constexpr int *d = &a[1]; // expected-error {{constant}} expected-note {{indexing of array without known bound}}
-  struct Z {
+  constexpr int *e = a + 1; // expected-error {{constant}} expected-note {{indexing of array without known bound}}
-    static const void *const a[];
+
-    static const void *const b[];
+  struct X {
+    int a;
+    int b[]; // expected-warning {{C99}}
   };
-  constexpr const void *Z::a[] = {&b[0], &b[1]};
+  extern X x;
-  constexpr const void *Z::b[] = {&a[0], &a[1]};
+  constexpr int *xb = x.b; // expected-error {{constant}} expected-note {{not supported}}
+
+  struct Y { int a; };
+  extern Y yarr[];
+  constexpr Y *p = yarr;
+  constexpr int *q = &p->a;
+
+  extern const int carr[]; // expected-note {{here}}
+  constexpr int n = carr[0]; // expected-error {{constant}} expected-note {{non-constexpr variable}}
 }
 
 namespace DependentValues {

test/SemaCXX/constexpr-array-unknown-bound.cpp

@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -Wno-uninitialized -std=c++1z -fsyntax-only -verify
+
+const extern int arr[];
+constexpr auto p = arr; // ok
+constexpr int f(int i) {return p[i];} // expected-note {{read of dereferenced one-past-the-end pointer}}
+
+constexpr int arr[] {1, 2, 3};
+constexpr auto p2 = arr + 2; // ok
+constexpr int x = f(2); // ok
+constexpr int y = f(3); // expected-error {{constant expression}}
+// expected-note-re@-1 {{in call to 'f({{.*}})'}}
+
+// FIXME: consider permitting this case
+struct A {int m[];} a;
+constexpr auto p3 = a.m; // expected-error {{constant expression}} expected-note {{without known bound}}
+constexpr auto p4 = a.m + 1; // expected-error {{constant expression}} expected-note {{without known bound}}
+
+void g(int i) {
+  int arr[i];
+  constexpr auto *p = arr + 2; // expected-error {{constant expression}} expected-note {{without known bound}}
+
+  // FIXME: Give a better diagnostic here. The issue is that computing
+  // sizeof(*arr2) within the array indexing fails due to the VLA.
+  int arr2[2][i];
+  constexpr int m = ((void)arr2[2], 0); // expected-error {{constant expression}}
+}

test/SemaTemplate/temp_arg_nontype_cxx1z.cpp

@@ -23,6 +23,9 @@ namespace Array {
   A<const char*, &(&x)[1]> h; // expected-error {{refers to subobject '&x + 1'}}
   A<const char*, 0> i; // expected-error {{not allowed in a converted constant}}
   A<const char*, nullptr> j;
+
+  extern char aub[];
+  A<char[], aub> k;
 }
 
 namespace Function {