[clang] -fsanitize=array-bounds: treat all trailing size-1 arrays as flexible

[sberg]

Summary

...even if the size resulted from a macro expansion. This reverts back to the behavior prior to
https://github.com/llvm/llvm-project/commit/886715af962de2c92fac4bd37104450345711e4a "[clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible arrays". The new behavior caused false out-of-bounds-index reports from e.g. HarfBuzz built with -fsanitize=array-bounds: HarfBuzz has various "fake" flexible array members of the form

Type                arrayZ[HB_VAR_ARRAY];

in https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-open-type.hh, where HB_VAR_ARRAY is defined as

#ifndef HB_VAR_ARRAY
#define HB_VAR_ARRAY 1
#endif

in https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-machinery.hh.

Also added a test.

[serge-sans-paille]

clang/include/clang/AST/Expr.h

455

Maybe default to false here?

clang/test/CodeGen/bounds-checking.c

69

The above checks look for CHECK-NOT: call {{.*}} @llvm.{{(ubsan)?trap}}

[sberg]

I'm abandoning this as the underlying https://reviews.llvm.org/D126864 "[clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible arrays" has been reverted for now.

I documented all my issues with that underlying commit (including, but not limited to what was presented here) in my comment at https://reviews.llvm.org/D126864#3614235.

clang/include/clang/AST/Expr.h

public:
   /// True when this expression refers to a flexible array member in a
   /// struct. \c StrictFlexArraysLevel controls which array bounds are
   /// acceptable for such arrays:
   ///
   /// - 0 => any array bound,
   /// - 1 => [0], [1], [ ]
   /// - 2 => [0], [ ]
   /// - 3 => [ ]
-  bool isFlexibleArrayMember(ASTContext &Ctx, int StrictFlexArraysLevel) const;
+  bool isFlexibleArrayMember(ASTContext &Ctx, int StrictFlexArraysLevel,
+                             bool IgnoreSizeFromMacro) const;
 
   /// setValueKind - Set the value kind produced by this expression.
   void setValueKind(ExprValueKind Cat) { ExprBits.ValueKind = Cat; }
 
   /// setObjectKind - Set the object kind produced by this expression.
   void setObjectKind(ExprObjectKind Cat) { ExprBits.ObjectKind = Cat; }
 
 private:

clang/lib/AST/Expr.cpp

if (const FieldDecl *FD = E->getSourceBitField())
     if (!Semantic && FD->getType()->isUnsignedIntegerType() &&
         !FD->getBitWidth()->isValueDependent() &&
         FD->getBitWidthValue(FD->getASTContext()) == 1)
       return true;
 
   return false;
 }
 
-bool Expr::isFlexibleArrayMember(ASTContext &Ctx,
-                                 int StrictFlexArraysLevel) const {
+bool Expr::isFlexibleArrayMember(ASTContext &Ctx, int StrictFlexArraysLevel,
+                                 bool IgnoreSizeFromMacro) const {
   const NamedDecl *ND = nullptr;
   if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(this))
     ND = DRE->getDecl();
   if (const MemberExpr *ME = dyn_cast<MemberExpr>(this))
     ND = ME->getMemberDecl();
   if (const ObjCIvarRefExpr *RE = dyn_cast<ObjCIvarRefExpr>(this))
     ND = RE->getDecl();
   if (!ND)
while (TInfo) {
     // 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())
+      if (!SizeExpr ||
+          (IgnoreSizeFromMacro && SizeExpr->getExprLoc().isMacroID()))
         return false;
     }
     break;
   }
 
   const ObjCInterfaceDecl *ID =
       dyn_cast<ObjCInterfaceDecl>(FD->getDeclContext());
   const RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());

clang/lib/CodeGen/CGExpr.cpp

if (const VectorType *VT = Base->getType()->getAs<VectorType>()) {
     return CGF.Builder.getInt32(VT->getNumElements());
   }
 
   Base = Base->IgnoreParens();
 
   if (const auto *CE = dyn_cast<CastExpr>(Base)) {
     if (CE->getCastKind() == CK_ArrayToPointerDecay &&
         !CE->getSubExpr()->IgnoreParens()->isFlexibleArrayMember(
-            Context, std::max(StrictFlexArraysLevel, 1))) {
+            Context, std::max(StrictFlexArraysLevel, 1), false)) {
       IndexedType = CE->getSubExpr()->getType();
       const ArrayType *AT = IndexedType->castAsArrayTypeUnsafe();
       if (const auto *CAT = dyn_cast<ConstantArrayType>(AT))
         return CGF.Builder.getInt(CAT->getSize());
       else if (const auto *VAT = dyn_cast<VariableArrayType>(AT))
         return CGF.getVLASize(VAT).NumElts;
       // Ignore pass_object_size here. It's not applicable on decayed pointers.
     }

clang/lib/Sema/SemaChecking.cpp

if (index.isUnsigned() || !index.isNegative()) {
     // 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 members.
-    if (BaseExpr->isFlexibleArrayMember(Context,
-                                        getLangOpts().StrictFlexArrays))
+    if (BaseExpr->isFlexibleArrayMember(Context, getLangOpts().StrictFlexArrays,
+                                        true))
       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());

clang/test/CodeGen/bounds-checking.c

 __attribute__((no_sanitize("bounds")))
 int f6(int i) {
   int b[64];
   // CHECK-NOT: call void @llvm.trap()
   // CHECK-NOT: trap:
   // CHECK-NOT: cont:
   return b[i];
 }
+
+#define FLEXIBLE 1
+struct Macro {
+  int a[FLEXIBLE];
+};
+
+// CHECK-LABEL: define {{.*}} @f7
+int f7(struct Macro *m, int i) {
+  // CHECK-NOT: call {{.*}} @llvm.ubsantrap
+  return m->a[i];
+}