[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible arrays

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D126864#3614235 <https://reviews.llvm.org/D126864#3614235>, @sberg wrote:

> In D126864#3612149 <https://reviews.llvm.org/D126864#3612149>, @sberg wrote:
>
>> see https://reviews.llvm.org/D128643 "[clang] -fsanitize=array-bounds: treat all trailing size-1 arrays as flexible" for another regression
>
> For the record (now that the original commit has been reverted anyway for now):
>
> Besides the above regression for `-fsanitize=array-bounds` and macro'ized array size in HarfBuzz, I also ran into yet another regression when Firebird is built with `-fsanitize=array-bounds`:  That project, in https://github.com/FirebirdSQL/firebird/blob/master/src/lock/lock_proto.h uses a trailing member
>
>   	srq lhb_hash[1];			// Hash table
>
> as a flexible array, but declared in a
>
>   struct lhb : public Firebird::MemoryHeader
>
> that is not a standard-layout class (because the `Firebird::MemoryHeader` base class also declares non-static data members).  And `isFlexibleArrayMember` returns `false` if the surrounding class is not a standard-layout class.
>
> In the end, what brought back a successful `-fsanitize=array-bounds` LibreOffice (which bundles those HarfBuzz and Firebird, among others) for me was to exclude the whole set of blocks
>
>   // Don't consider sizes resulting from macro expansions or template argument
>   // substitution to form C89 tail-padded arrays.
>   
>   TypeSourceInfo *TInfo = FD->getTypeSourceInfo();
>   while (TInfo) {
>     TypeLoc TL = TInfo->getTypeLoc();
>     // Look through typedefs.
>     if (TypedefTypeLoc TTL = TL.getAs<TypedefTypeLoc>()) {
>       const TypedefNameDecl *TDL = TTL.getTypedefNameDecl();
>       TInfo = TDL->getTypeSourceInfo();
>       continue;
>     }
>     if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
>       const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
>       if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
>         return false;
>     }
>     break;
>   }
>   
>   const ObjCInterfaceDecl *ID =
>       dyn_cast<ObjCInterfaceDecl>(FD->getDeclContext());
>   const RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());
>   if (RD) {
>     if (RD->isUnion())
>       return false;
>     if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
>       if (!CRD->isStandardLayout())
>         return false;
>     }
>   } else if (!ID)
>     return false;
>
> (by means of an additional `bool` parameter) when `isFlexibleArrayMember` is called from `getArrayIndexingBound` (in `clang/lib/CodeGen/CGExpr.cpp`).

Oh, I did not see this when pushing a test efd90ffbfc427ad4c4675ac1fcae9d53cc7f1322 <https://reviews.llvm.org/rGefd90ffbfc427ad4c4675ac1fcae9d53cc7f1322> . Consider adding additional tests in `clang/test/CodeGen/bounds-checking-fam.c`.
It's worth bringing up such issue to the project. GCC and Clang have been working around them so far but they should eventually be fixed.
For CPython I try to be loud on https://github.com/python/cpython/issues/84301


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126864/new/

https://reviews.llvm.org/D126864