Revert "[clang] Introduce -fstrict-flex-arrays=<n> for stricter handling of flexible arrays"

vitalybuka · vitalybuka · commit cdfa15da94f0 · 2022-06-27T14:03:09.000-07:00
This reverts D126864 and related fixes. This reverts commit 572b087. This reverts commit 886715a.
diff --git a/clang/docs/ClangCommandLineReference.rst b/clang/docs/ClangCommandLineReference.rst
@@ -2639,12 +2639,6 @@ Enable unstable and experimental features
 
 .. option:: -fuse-init-array, -fno-use-init-array
 
-.. option:: -fstrict-flex-arrays=<arg>, -fno-strict-flex-arrays
-
-Control which arrays are considered as flexible arrays members. <arg>
-can be 1 (array of size 0, 1 and undefined are considered), 2 (array of size 0
-and undefined are considered) or 3 (only array of undefined size are considered).
-
 .. option:: -fuse-ld=<arg>
 
 .. option:: -fuse-line-directives, -fno-use-line-directives
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -68,16 +68,11 @@ Major New Features
 
       Randomizing structure layout is a C-only feature.
 
-- Clang now supports the ``-fstrict-flex-arrays=<arg>`` option to control which
-  array bounds lead to flexible array members. The option yields more accurate
-  ``__builtin_object_size`` and ``__builtin_dynamic_object_size`` results in
-  most cases but may be overly conservative for some legacy code.
 - Experimental support for HLSL has been added. The implementation is
   incomplete and highly experimental. For more information about the ongoing
   work to support HLSL see the `documentation
   <https://clang.llvm.org/docs/HLSLSupport.html>`_, or the `GitHub project
   <https://github.com/orgs/llvm/projects/4>`_.
-
 Bug Fixes
 ---------
 - ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
@@ -443,16 +443,6 @@ class Expr : public ValueStmt {
     return (OK == OK_Ordinary || OK == OK_BitField);
   }
 
-  /// 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;
-
   /// setValueKind - Set the value kind produced by this expression.
   void setValueKind(ExprValueKind Cat) { ExprBits.ValueKind = Cat; }
 
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
@@ -424,7 +424,6 @@ LANGOPT(PaddingOnUnsignedFixedPoint, 1, 0,
 LANGOPT(RegisterStaticDestructors, 1, 1, "Register C++ static destructors")
 
 LANGOPT(MatrixTypes, 1, 0, "Enable or disable the builtin matrix type")
-LANGOPT(StrictFlexArrays, 2, 0, "Rely on strict definition of flexible arrays")
 
 COMPATIBLE_VALUE_LANGOPT(MaxTokens, 32, 0, "Max number of tokens per TU or 0")
 
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
@@ -1140,12 +1140,6 @@ def fallow_unsupported : Flag<["-"], "fallow-unsupported">, Group<f_Group>;
 def fapple_kext : Flag<["-"], "fapple-kext">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use Apple's kernel extensions ABI">,
   MarshallingInfoFlag<LangOpts<"AppleKext">>;
-def fstrict_flex_arrays_EQ : Joined<["-"], "fstrict-flex-arrays=">,Group<f_Group>,
-  MetaVarName<"<n>">, Values<"0,1,2,3">,
-  LangOpts<"StrictFlexArrays">,
-  Flags<[CC1Option]>,
-  HelpText<"Enable optimizations based on the strict definition of flexible arrays">,
-  MarshallingInfoInt<LangOpts<"StrictFlexArrays">>;
 defm apple_pragma_pack : BoolFOption<"apple-pragma-pack",
   LangOpts<"ApplePragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable Apple gcc-compatible #pragma pack handling">,
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1364,7 +1364,6 @@ class MemRegionManager {
   ~MemRegionManager();
 
   ASTContext &getContext() { return Ctx; }
-  const ASTContext &getContext() const { return Ctx; }
 
   llvm::BumpPtrAllocator &getAllocator() { return A; }
 
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
@@ -203,91 +203,6 @@ bool Expr::isKnownToHaveBooleanValue(bool Semantic) const {
   return false;
 }
 
-bool Expr::isFlexibleArrayMember(ASTContext &Ctx,
-                                 int StrictFlexArraysLevel) 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)
-    return false;
-
-  const ConstantArrayType *ArrayTy = Ctx.getAsConstantArrayType(getType());
-
-  const Type *BaseType =
-      ArrayTy == nullptr ? nullptr : ArrayTy->getElementType().getTypePtr();
-  bool IsUnboundedArray = (BaseType == nullptr);
-
-  if (!IsUnboundedArray) {
-    llvm::APInt Size = ArrayTy->getSize();
-
-    switch (StrictFlexArraysLevel) {
-    case 3:
-      return false;
-    case 2:
-      if (Size != 0)
-        return false;
-      break;
-    case 1:
-      if (Size.ugt(1))
-        return false;
-      break;
-    case 0:
-      break;
-    default:
-      llvm_unreachable("Invalid strict flex arrays level");
-    }
-  }
-
-  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.
-
-  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;
-
-  // See if this is the last field decl in the record.
-  const Decl *D = FD;
-  while ((D = D->getNextDeclInContext()))
-    if (isa<FieldDecl>(D))
-      return false;
-
-  return true;
-}
-
 const ValueDecl *
 Expr::getAsBuiltinConstantDeclRef(const ASTContext &Context) const {
   Expr::EvalResult Eval;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
@@ -11592,16 +11592,9 @@ static bool isUserWritingOffTheEnd(const ASTContext &Ctx, const LValue &LVal) {
   //   conservative with the last element in structs (if it's an array), so our
   //   current behavior is more compatible than an explicit list approach would
   //   be.
-  int StrictFlexArraysLevel = Ctx.getLangOpts().StrictFlexArrays;
   return LVal.InvalidBase &&
          Designator.Entries.size() == Designator.MostDerivedPathLength &&
          Designator.MostDerivedIsArrayElement &&
-         (Designator.isMostDerivedAnUnsizedArray() ||
-          (Designator.getMostDerivedArraySize() == 0 &&
-           StrictFlexArraysLevel < 3) ||
-          (Designator.getMostDerivedArraySize() == 1 &&
-           StrictFlexArraysLevel < 2) ||
-          StrictFlexArraysLevel == 0) &&
          isDesignatorAtObjectEnd(Ctx, LVal);
 }
 
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
@@ -875,6 +875,44 @@ void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
   }
 }
 
+/// Determine whether this expression refers to a flexible array member in a
+/// struct. We disable array bounds checks for such members.
+static bool isFlexibleArrayMemberExpr(const Expr *E) {
+  // For compatibility with existing code, we treat arrays of length 0 or
+  // 1 as flexible array members.
+  // FIXME: This is inconsistent with the warning code in SemaChecking. Unify
+  // the two mechanisms.
+  const ArrayType *AT = E->getType()->castAsArrayTypeUnsafe();
+  if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
+    // FIXME: Sema doesn't treat [1] as a flexible array member if the bound
+    // was produced by macro expansion.
+    if (CAT->getSize().ugt(1))
+      return false;
+  } else if (!isa<IncompleteArrayType>(AT))
+    return false;
+
+  E = E->IgnoreParens();
+
+  // A flexible array member must be the last member in the class.
+  if (const auto *ME = dyn_cast<MemberExpr>(E)) {
+    // FIXME: If the base type of the member expr is not FD->getParent(),
+    // this should not be treated as a flexible array member access.
+    if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
+      // FIXME: Sema doesn't treat a T[1] union member as a flexible array
+      // member, only a T[0] or T[] member gets that treatment.
+      if (FD->getParent()->isUnion())
+        return true;
+      RecordDecl::field_iterator FI(
+          DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
+      return ++FI == FD->getParent()->field_end();
+    }
+  } else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E)) {
+    return IRE->getDecl()->getNextIvar() == nullptr;
+  }
+
+  return false;
+}
+
 llvm::Value *CodeGenFunction::LoadPassedObjectSize(const Expr *E,
                                                    QualType EltTy) {
   ASTContext &C = getContext();
@@ -916,11 +954,8 @@ llvm::Value *CodeGenFunction::LoadPassedObjectSize(const Expr *E,
 
 /// If Base is known to point to the start of an array, return the length of
 /// that array. Return 0 if the length cannot be determined.
-static llvm::Value *getArrayIndexingBound(CodeGenFunction &CGF,
-                                          const Expr *Base,
-                                          QualType &IndexedType,
-                                          ASTContext &Context,
-                                          int StrictFlexArraysLevel) {
+static llvm::Value *getArrayIndexingBound(
+    CodeGenFunction &CGF, const Expr *Base, QualType &IndexedType) {
   // For the vector indexing extension, the bound is the number of elements.
   if (const VectorType *VT = Base->getType()->getAs<VectorType>()) {
     IndexedType = Base->getType();
@@ -931,8 +966,7 @@ static llvm::Value *getArrayIndexingBound(CodeGenFunction &CGF,
 
   if (const auto *CE = dyn_cast<CastExpr>(Base)) {
     if (CE->getCastKind() == CK_ArrayToPointerDecay &&
-        !CE->getSubExpr()->IgnoreParens()->isFlexibleArrayMember(
-            Context, std::max(StrictFlexArraysLevel, 1))) {
+        !isFlexibleArrayMemberExpr(CE->getSubExpr())) {
       IndexedType = CE->getSubExpr()->getType();
       const ArrayType *AT = IndexedType->castAsArrayTypeUnsafe();
       if (const auto *CAT = dyn_cast<ConstantArrayType>(AT))
@@ -960,8 +994,7 @@ void CodeGenFunction::EmitBoundsCheck(const Expr *E, const Expr *Base,
   SanitizerScope SanScope(this);
 
   QualType IndexedType;
-  llvm::Value *Bound = getArrayIndexingBound(
-      *this, Base, IndexedType, getContext(), getLangOpts().StrictFlexArrays);
+  llvm::Value *Bound = getArrayIndexingBound(*this, Base, IndexedType);
   if (!Bound)
     return;
 
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6221,8 +6221,6 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   Args.AddLastArg(CmdArgs, options::OPT_funroll_loops,
                   options::OPT_fno_unroll_loops);
 
-  Args.AddLastArg(CmdArgs, options::OPT_fstrict_flex_arrays_EQ);
-
   Args.AddLastArg(CmdArgs, options::OPT_pthread);
 
   if (Args.hasFlag(options::OPT_mspeculative_load_hardening,
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
@@ -15716,6 +15716,53 @@ void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
     << TRange << Op->getSourceRange();
 }
 
+/// 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, const 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.
+
+  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 RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());
+  if (!RD) return false;
+  if (RD->isUnion()) return false;
+  if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
+    if (!CRD->isStandardLayout()) return false;
+  }
+
+  // See if this is the last field decl in the record.
+  const Decl *D = FD;
+  while ((D = D->getNextDeclInContext()))
+    if (isa<FieldDecl>(D))
+      return false;
+  return true;
+}
+
 void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
                             const ArraySubscriptExpr *ASE,
                             bool AllowOnePastEnd, bool IndexNegated) {
@@ -15868,9 +15915,10 @@ void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
     if (AllowOnePastEnd ? index.ule(size) : index.ult(size))
       return;
 
-    // Also don't warn for flexible array members.
-    if (BaseExpr->isFlexibleArrayMember(Context,
-                                        getLangOpts().StrictFlexArrays))
+    // 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.
+    if (IsTailPaddedMemberArray(*this, size, ND))
       return;
 
     // Suppress the warning if the subscript expression (as identified by the
diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -789,9 +789,6 @@ DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR,
       if (isa<IncompleteArrayType>(AT))
         return true;
 
-      if (getContext().getLangOpts().StrictFlexArrays)
-        return false;
-
       if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
         const llvm::APInt &Size = CAT->getSize();
         if (Size.isZero())
diff --git a/clang/test/CodeGen/bounds-checking-fma.c b/clang/test/CodeGen/bounds-checking-fma.c
diff --git a/clang/test/CodeGen/bounds-checking.c b/clang/test/CodeGen/bounds-checking.c
diff --git a/clang/test/CodeGen/object-size-flex-array.c b/clang/test/CodeGen/object-size-flex-array.c
diff --git a/clang/test/CodeGenObjC/ubsan-array-bounds.m b/clang/test/CodeGenObjC/ubsan-array-bounds.m
diff --git a/clang/test/Sema/array-bounds-ptr-arith.c b/clang/test/Sema/array-bounds-ptr-arith.c
diff --git a/clang/test/SemaCXX/array-bounds-strict-flex-arrays.cpp b/clang/test/SemaCXX/array-bounds-strict-flex-arrays.cpp
