[clang] Rework IsTailPaddedMemberArray into isFlexibleArrayMemberExpr

[serge-sans-paille]

Summary

This fixes a bunch of FIXME within IsTailPaddedMemberArray related code.

As a side effect, this now also triggers a warning when trying to access a
"struct hack" field with an index that cannot be represented in associated address space.

[msebor]

clang/lib/Sema/SemaChecking.cpp

15969

I'm not familiar with this code but I'm guessing that at most one of the if conditionals is true, and if it's the first one then the second dyn_cast will fail. Should the if guarding this assignment then be an else if?

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

15881

depending on <ins>the value of</ins> -fstrict-flex-array <del>value</del><ins>.</ins>

(Sorry, the Suggest Edits button seems to be unavailable)

15969

While this code is moving around from elsewhere, +1, this should use an else if. Also, you should use const auto * for the declarations since the type is explicitly spelled out in the initializers.

15974

You can drop the parens.

[serge-sans-paille]

Take reviews into account

[aaron.ballman]

Found a few more things while trying to convince myself this really is NFC, and I don't think it is. If you agree, then I think we should have some additional test coverage to show the behavioral changes.

clang/lib/Sema/SemaChecking.cpp

15977

Missed this before, but drop the top-level const or remove the local and just use the call directly in the one place the value is needed.

15989

This seems like a functional change....

15990–15991

I think the parens helped to add clarity here (also, there's probably a warning generated about mixed || and && from this too, isn't there?)

16004

...because now we can get into this block...

16048

... which means we can now hit this diagnostic.

[serge-sans-paille]

Address minor nits, plus add a test case that showcases the extra (legitimate!) warning we now get when accessing a "struct hack" field with an index that cannot be represented in associated address space.

Kudos to @aaron.ballman for spotting this one!

As a comparison, current behavior for the same test case: https://godbolt.org/z/86Wcbrz5q (only the flexible array member is flagged with a warning, the struct hacks are ignored)

[msebor]

clang/test/Sema/unbounded-array-bounds.c

101 ↗

(On Diff #460899)

There's a difference between the sizes of fam1 and fam that makes accesses to the four leading elements of fam1.tail strictly in bounds, while no access to either fam.tail or fam0.tail is (sizeof fam is the same as sizeof int while sizeof fam1 is equal to sizeof (int[2]) on common targets). It would be helpful to capture that difference in the tests, both for the warning and for __builtin_object_size.

There should also be a difference between accessing elements of an object of an initialized struct with a flexible array member (i.e., one whose size is known) and those of an object that's only declared but that's defined in some other translation unit. Since the size of the object is determined by its initializer, it should be reflected in __builtin_object_size and accesses to it checked by -Warray-bounds. The size of the latter object is unknown it must be assumed to be PTRDIFF_MAX - sizeof (int) - 1. It would also be helpful to add tests for these cases.

As far as I can see, none of these cases seems to be handled quite right on trunk. For example, the size of s below should be 8 but Clang evaluates __builtin_object_size(&s, N) to 4, without diagnosing any past-the-end accesses to s.a:

struct S {
  int n;
  char a[];
} s = { 1, { 2, 3, 4, 5 } };

[msebor]

clang/test/Sema/unbounded-array-bounds.c

101 ↗

(On Diff #460899)

I opened PR #57860 to better show what I mean.

[serge-sans-paille]

clang/test/Sema/unbounded-array-bounds.c

101 ↗

(On Diff #460899)

Agreed. I suggest we discuss that in this PR, while me merge this "code cleanup with enhancement", if @aaron.ballman agrees :-)

[aaron.ballman]

LGTM, though this should probably have a release note for it (we can augment the release note when we make further changes in this area).

clang/test/Sema/unbounded-array-bounds.c

101 ↗

(On Diff #460899)

+1 -- there are bugs there to be fixed, but handling that separately from this cleanup seems like it will be easier to review and discuss.

clang/lib/Sema/SemaChecking.cpp

   const PointerType *SrcPtr = Op->getType()->getAs<PointerType>();
   if (!SrcPtr) return;
   QualType SrcPointee = SrcPtr->getPointeeType();
 
   // Explicitly allow casts from cv void*.  We already implicitly
   // allowed casts to cv void*, since they have alignment 1.
   // Also allow casts involving incomplete types, which implicitly
   // includes 'void'.
   if (SrcPointee->isIncompleteType()) return;
 
   CharUnits SrcAlign = getPresumedAlignmentOfPointer(Op, *this);
 
   if (SrcAlign >= DestAlign) return;
 
   Diag(TRange.getBegin(), diag::warn_cast_align)
     << Op->getType() << T
     << static_cast<unsigned>(SrcAlign.getQuantity())
     << static_cast<unsigned>(DestAlign.getQuantity())
     << TRange << Op->getSourceRange();
 }
 
-/// Check whether this array fits the idiom of a size-one tail padded
-/// array member of a struct.
+/// Check whether this array fits the idiom of a flexible array member,
+/// depending on the value of -fstrict-flex-array.
 ///
-/// 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, const llvm::APInt &Size,
-                                    const NamedDecl *ND,
-                                    unsigned StrictFlexArraysLevel) {
+/// We avoid emitting out-of-bounds access warnings for such arrays.
+static bool isFlexibleArrayMemberExpr(Sema &S, const Expr *E,
+                                      const NamedDecl *ND,
+                                      unsigned StrictFlexArraysLevel) {
+
   if (!ND)
     return false;
 
+  const ConstantArrayType *ArrayTy =
+      S.Context.getAsConstantArrayType(E->getType());
+  llvm::APInt Size = ArrayTy->getSize();
+
+  if (Size == 0)
+    return true;
+
   // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
   // arrays to be treated as flexible-array-members, we still emit diagnostics
   // as if they are not. Pending further discussion...
   if (StrictFlexArraysLevel >= 2 || Size.uge(2))
     return false;
 
   const FieldDecl *FD = dyn_cast<FieldDecl>(ND);
   if (!FD)

                             const ArraySubscriptExpr *ASE,
                             bool AllowOnePastEnd, bool IndexNegated) {
   // Already diagnosed by the constant evaluator.
   if (isConstantEvaluated())
     return;
 
   IndexExpr = IndexExpr->IgnoreParenImpCasts();
   if (IndexExpr->isValueDependent())
     return;
 
   const Type *EffectiveType =
       BaseExpr->getType()->getPointeeOrArrayElementType();
   BaseExpr = BaseExpr->IgnoreParenCasts();
   const ConstantArrayType *ArrayTy =
       Context.getAsConstantArrayType(BaseExpr->getType());
 
+  const unsigned StrictFlexArraysLevel = getLangOpts().StrictFlexArrays;
+
+  const NamedDecl *ND = nullptr;
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
+    ND = DRE->getDecl();
+  else if (const auto *ME = dyn_cast<MemberExpr>(BaseExpr))
+    ND = ME->getMemberDecl();
+
   const Type *BaseType =
       ArrayTy == nullptr ? nullptr : ArrayTy->getElementType().getTypePtr();
-  bool IsUnboundedArray = (BaseType == nullptr);
+  bool IsUnboundedArray =
+      BaseType == nullptr ||
+      isFlexibleArrayMemberExpr(*this, BaseExpr, ND, StrictFlexArraysLevel);
   if (EffectiveType->isDependentType() ||
-      (!IsUnboundedArray && BaseType->isDependentType()))
+      !IsUnboundedArray && BaseType->isDependentType())
     return;
 
   Expr::EvalResult Result;
   if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects))
     return;
 
   llvm::APSInt index = Result.Val.getInt();
   if (IndexNegated) {
     index.setIsUnsigned(false);
     index = -index;
   }
 
-  const NamedDecl *ND = nullptr;
-  if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
-    ND = DRE->getDecl();
-  if (const MemberExpr *ME = dyn_cast<MemberExpr>(BaseExpr))
-    ND = ME->getMemberDecl();
-
   if (IsUnboundedArray) {
     if (EffectiveType->isFunctionType())
       return;
     if (index.isUnsigned() || !index.isNegative()) {
       const auto &ASTC = getASTContext();
       unsigned AddrBits =
           ASTC.getTargetInfo().getPointerWidth(ASTC.getTargetAddressSpace(
               EffectiveType->getCanonicalTypeInternal()));
       if (index.getBitWidth() < AddrBits)

       MaxElems = MaxElems.udiv(ElemBytes);
 
       unsigned DiagID =
           ASE ? diag::warn_array_index_exceeds_max_addressable_bounds
               : diag::warn_ptr_arith_exceeds_max_addressable_bounds;
 
       // Diag message shows element size in bits and in "bytes" (platform-
       // dependent CharUnits)
       DiagRuntimeBehavior(BaseExpr->getBeginLoc(), BaseExpr,
                           PDiag(DiagID)
                               << toString(index, 10, true) << AddrBits
                               << (unsigned)ASTC.toBits(*ElemCharUnits)
                               << toString(ElemBytes, 10, false)
                               << toString(MaxElems, 10, false)
                               << (unsigned)MaxElems.getLimitedValue(~0U)
                               << IndexExpr->getSourceRange());
 

 
   if (index.isUnsigned() || !index.isNegative()) {
     // It is possible that the type of the base expression after
     // IgnoreParenCasts is incomplete, even though the type of the base
     // expression before IgnoreParenCasts is complete (see PR39746 for an
     // example). In this case we have no information about whether the array
     // access exceeds the array bounds. However we can still diagnose an array
     // access which precedes the array bounds.
-    //
-    // FIXME: this check should be redundant with the IsUnboundedArray check
-    // above.
     if (BaseType->isIncompleteType())
       return;
 
-    // FIXME: this check should be used to set IsUnboundedArray from the
-    // beginning.
     llvm::APInt size = ArrayTy->getSize();
-    if (!size.isStrictlyPositive())
-      return;
 
     if (BaseType != EffectiveType) {
       // Make sure we're comparing apples to apples when comparing index to size
       uint64_t ptrarith_typesize = Context.getTypeSize(EffectiveType);
       uint64_t array_typesize = Context.getTypeSize(BaseType);
       // Handle ptrarith_typesize being zero, such as when casting to void*
       if (!ptrarith_typesize) ptrarith_typesize = 1;
       if (ptrarith_typesize != array_typesize) {

 
     // For array subscripting the index must be less than size, but for pointer
     // arithmetic also allow the index (offset) to be equal to size since
     // computing the next address after the end of the array is legal and
     // commonly done e.g. in C++ iterators and range-based for loops.
     if (AllowOnePastEnd ? index.ule(size) : index.ult(size))
       return;
 
-    // Also don't warn for Flexible Array Member emulation.
-    const unsigned StrictFlexArraysLevel = getLangOpts().StrictFlexArrays;
-    if (IsTailPaddedMemberArray(*this, size, ND, StrictFlexArraysLevel))
-      return;
-
     // Suppress the warning if the subscript expression (as identified by the
     // ']' location) and the index expression are both from macro expansions
     // within a system header.
     if (ASE) {
       SourceLocation RBracketLoc = SourceMgr.getSpellingLoc(
           ASE->getRBracketLoc());
       if (SourceMgr.isInSystemHeader(RBracketLoc)) {
         SourceLocation IndexLoc =