Flesh out the -Warray-bounds detection of C89 tail-padded one-element

arrays. This now suppresses the warning only in the case of
a one-element array as the last field in a struct where the array size
is a literal '1' rather than any macro expansion or template parameter.

This doesn't distinguish between the language standard in use to allow
code which dates from C89 era to compile without the warning even in C99
and C++ builds. We could add a separate warning (under a different flag)
with fixit hints to switch to a flexible array, but its not clear that
this would be desirable. Much of the code using this idiom is striving
for maximum portability.

Tests were also fleshed out a bit, and the diagnostic itself tweaked to
be more pretty w.r.t. single elment arrays. This is more ugly than
I would like due to APInt's not being supported by the diagnostic
rendering engine.

A pseudo-patch for this was proposed by Nicola Gigante, but I reworked
it both for several correctness issues and for code style.

Sorry this was so long in coming.

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

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index ce76b605d77a1ce79a1ed00798010e7069ea5418..1bb6c7125f1a16cd7e814b96762029259ed47216 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4089,7 +4089,8 @@ def warn_array_index_precedes_bounds : Warning<
   "array index of '%0' indexes before the beginning of the array">,
   InGroup<DiagGroup<"array-bounds">>;
 def warn_array_index_exceeds_bounds : Warning<
-  "array index of '%0' indexes past the end of an array (that contains %1 elements)">,
+  "array index of '%0' indexes past the end of an array (that contains %1 "
+  "%select{element|elements}2)">,
   InGroup<DiagGroup<"array-bounds">>;
 def note_array_index_out_of_bounds : Note<
   "array %0 declared here">;

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 3065a5f64b3d4763d5483463024ae28963dddfda..c54d250b042898b3efffce3198b035596563c9a3 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -3474,6 +3474,42 @@ void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
     << TRange << Op->getSourceRange();
 }
 
+/// \brief Check whether this array fits the idiom of a size-one tail padded
+/// array member of a struct.
+///
+/// We avoid emitting out-of-bounds access warnings for such arrays as they are
+/// commonly used to emulate flexible arrays in C89 code.
+static bool IsTailPaddedMemberArray(Sema &S, llvm::APInt Size,
+                                    const NamedDecl *ND) {
+  if (Size != 1 || !ND) return false;
+
+  const FieldDecl *FD = dyn_cast<FieldDecl>(ND);
+  if (!FD) return false;
+
+  // Don't consider sizes resulting from macro expansions or template argument
+  // substitution to form C89 tail-padded arrays.
+  ConstantArrayTypeLoc TL =
+    cast<ConstantArrayTypeLoc>(FD->getTypeSourceInfo()->getTypeLoc());
+  const Expr *SizeExpr = dyn_cast<IntegerLiteral>(TL.getSizeExpr());
+  if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
+    return false;
+
+  const RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());
+  if (!RD || !RD->isStruct())
+    return false;
+
+  // This is annoyingly inefficient. We don't have a bi-directional iterator
+  // here so we can't walk backwards through the decls, we have to walk
+  // forward.
+  for (RecordDecl::field_iterator FI = RD->field_begin(),
+                                  FEnd = RD->field_end();
+       FI != FEnd; ++FI) {
+    if (*FI == FD)
+      return ++FI == FEnd;
+  }
+  return false;
+}
+
 static void CheckArrayAccess_Check(Sema &S,
                                    const clang::ArraySubscriptExpr *E) {
   const Expr *BaseExpr = E->getBase()->IgnoreParenImpCasts();
@@ -3490,13 +3526,10 @@ static void CheckArrayAccess_Check(Sema &S,
     return;
 
   const NamedDecl *ND = NULL;
-  bool IsMemberDecl = false;
   if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
     ND = dyn_cast<NamedDecl>(DRE->getDecl());
-  if (const MemberExpr *ME = dyn_cast<MemberExpr>(BaseExpr)) {
+  if (const MemberExpr *ME = dyn_cast<MemberExpr>(BaseExpr))
     ND = dyn_cast<NamedDecl>(ME->getMemberDecl());
-    IsMemberDecl = true;
-  }
 
   if (index.isUnsigned() || !index.isNegative()) {
     llvm::APInt size = ArrayTy->getSize();
@@ -3514,18 +3547,14 @@ static void CheckArrayAccess_Check(Sema &S,
     // Also don't warn for arrays of size 1 which are members of some
     // structure. These are often used to approximate flexible arrays in C89
     // code.
-    // FIXME: We should also check whether there are any members after this
-    // member within the struct as that precludes the usage as a flexible
-    // array. We should also potentially check for an explicit '1' as opposed
-    // to a macro or template argument which might accidentally and erroneously
-    // expand to '1'.
-    if (IsMemberDecl && size == 1)
+    if (IsTailPaddedMemberArray(S, size, ND))
       return;
 
     S.DiagRuntimeBehavior(E->getBase()->getLocStart(), BaseExpr,
                           S.PDiag(diag::warn_array_index_exceeds_bounds)
                             << index.toString(10, true)
                             << size.toString(10, true)
+                            << (unsigned)size.ugt(1)
                             << IndexExpr->getSourceRange());
   } else {
     S.DiagRuntimeBehavior(E->getBase()->getLocStart(), BaseExpr,

diff --git a/test/SemaCXX/array-bounds.cpp b/test/SemaCXX/array-bounds.cpp
index 044949200e0e9230a986fb450d844f49b46e960a..e8156320db4a3ee34e6e4d4cd89f5ae05cfcc9b1 100644 (file)
--- a/test/SemaCXX/array-bounds.cpp
+++ b/test/SemaCXX/array-bounds.cpp
@@ -3,9 +3,11 @@
 int foo() {
   int x[2]; // expected-note 4 {{array 'x' declared here}}
   int y[2]; // expected-note 2 {{array 'y' declared here}}
+  int z[1]; // expected-note {{array 'z' declared here}}
   int *p = &y[2]; // no-warning
   (void) sizeof(x[2]); // no-warning
   y[2] = 2; // expected-warning {{array index of '2' indexes past the end of an array (that contains 2 elements)}}
+  z[1] = 'x'; // expected-warning {{array index of '1' indexes past the end of an array (that contains 1 element)}}
   return x[2] +  // expected-warning {{array index of '2' indexes past the end of an array (that contains 2 elements)}}
          y[-1] + // expected-warning {{array index of '-1' indexes before the beginning of the array}}
          x[sizeof(x)] +  // expected-warning {{array index of '8' indexes past the end of an array (that contains 2 elements)}}
@@ -176,11 +178,24 @@ void test_all_enums_covered(enum Values v) {
 
 namespace tailpad {
   struct foo {
+    char c1[1]; // expected-note {{declared here}}
     int x;
-    char c[1];
+    char c2[1];
   };
   
   char bar(struct foo *F) {
-    return F->c[3];  // no warning, foo could have tail padding allocated.
+    return F->c1[3]; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 element)}}
+    return F->c2[3]; // no warning, foo could have tail padding allocated.
+  }
+}
+
+namespace metaprogramming {
+#define ONE 1
+  struct foo { char c[ONE]; }; // expected-note {{declared here}}
+  template <int N> struct bar { char c[N]; }; // expected-note {{declared here}}
+
+  char test(foo *F, bar<1> *B) {
+    return F->c[3] + // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 element)}}
+           B->c[3]; // expected-warning {{array index of '3' indexes past the end of an array (that contains 1 element)}}
   }
 }