[clang][NFC] Use enum for -fstrict-flex-arrays

[void]

Summary

Use enums for the strict flex arrays flag so that it's more readable.

[serge-sans-paille]

Thanks for working on this! A few nits inline

clang/include/clang/Basic/LangOptions.h

369

typo: member

373

+ size

375

I think you meant Any trailing array member of undefined size is a FAM

clang/include/clang/Driver/Options.td

1155

Note that this depends on https://reviews.llvm.org/D134902 to have the "Incomplete" support.

[steakhal]

The StaticAnalyzer changes seem to be correct.

clang/include/clang/Basic/LangOptions.h

369–376

For most cases, I can either see no initializers or only the first entry is being initialized.
I believe, by default, the entries would be initialized to 0, 1, and 2 here anyway.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

799

[void]

Fix typos.

clang/include/clang/Basic/LangOptions.h

369–376

I wanted to be explicit with the numbering, because it corresponds with the value specified with -fstrict-flex-arrays=<n>.

[void]

Use an already available context.

clang/include/clang/Driver/Options.td

1155

Would it be better to remove Incomplete until that is approved?

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

799

Doh! Thanks.

[void]

Rebase with ToT.

[kees]

LGTM

[serge-sans-paille]

Thanks for the cleanup!

[ronlieb]

hi, seeing our amdgpu buildbot broken with this patch. please look into ?
let me know if you any help on our end.
https://lab.llvm.org/buildbot/#/builders/193/builds/19744

[void]

In D135107#3840602, @ronlieb wrote:

hi, seeing our amdgpu buildbot broken with this patch. please look into ?
let me know if you any help on our end.
https://lab.llvm.org/buildbot/#/builders/193/builds/19744

It's been fixed. Sorry about that!

clang/include/clang/Basic/LangOptions.h

public:
   enum class DefaultVisiblityExportMapping {
     None,
     /// map only explicit default visibilities to exported
     Explicit,
     /// map all default visibilities to exported
     All,
   };
 
+  enum class StrictFlexArraysLevelKind {
+    /// Any trailing array member is a FAM.
+    Default = 0,
+    /// Any trailing array member of undefined, 0, or 1 size is a FAM.
+    OneZeroOrIncomplete = 1,
+    /// Any trailing array member of undefined or 0 size is a FAM.
+    ZeroOrIncomplete = 2,
+    /// Any trailing array member of undefined size is a FAM.
+    Incomplete = 3,
+  };
+
 public:
   /// The used language standard.
   LangStandard::Kind LangStd;
 
   /// Set of enabled sanitizers.
   SanitizerSet Sanitize;
   /// Is at least one coverage instrumentation type enabled.
   bool SanitizeCoverage = false;

clang/include/clang/Basic/LangOptions.def

 
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
 LANGOPT(PaddingOnUnsignedFixedPoint, 1, 0,
         "unsigned fixed point types having one extra padding bit")
 
 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")
+
+ENUM_LANGOPT(StrictFlexArraysLevel, StrictFlexArraysLevelKind, 2,
+             StrictFlexArraysLevelKind::Default,
+             "Rely on strict definition of flexible arrays")
 
 COMPATIBLE_VALUE_LANGOPT(MaxTokens, 32, 0, "Max number of tokens per TU or 0")
 
 ENUM_LANGOPT(SignReturnAddressScope, SignReturnAddressScopeKind, 2, SignReturnAddressScopeKind::None,
              "Scope of return address signing")
 ENUM_LANGOPT(SignReturnAddressKey, SignReturnAddressKeyKind, 1, SignReturnAddressKeyKind::AKey,
              "Key used for return address signing")
 LANGOPT(BranchTargetEnforcement, 1, 0, "Branch-target enforcement enabled")

clang/include/clang/Driver/Options.td

defm allow_editor_placeholders : BoolFOption<"allow-editor-placeholders",
   PosFlag<SetTrue, [CC1Option], "Treat editor placeholders as valid source code">,
   NegFlag<SetFalse>>;
 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">,
-  LangOpts<"StrictFlexArrays">,
+  LangOpts<"StrictFlexArraysLevel">,
   Flags<[CC1Option]>,
+  NormalizedValuesScope<"LangOptions::StrictFlexArraysLevelKind">,
+  NormalizedValues<["Default", "OneZeroOrIncomplete", "ZeroOrIncomplete", "Incomplete"]>,
   HelpText<"Enable optimizations based on the strict definition of flexible arrays">,
-  MarshallingInfoInt<LangOpts<"StrictFlexArrays">>;
+  MarshallingInfoEnum<LangOpts<"StrictFlexArraysLevel">, "Default">;
 defm apple_pragma_pack : BoolFOption<"apple-pragma-pack",
   LangOpts<"ApplePragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable Apple gcc-compatible #pragma pack handling">,
   NegFlag<SetFalse>>;
 defm xl_pragma_pack : BoolFOption<"xl-pragma-pack",
   LangOpts<"XLPragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable IBM XL #pragma pack handling">,
   NegFlag<SetFalse>>;

clang/lib/AST/ExprConstant.cpp

static bool isUserWritingOffTheEnd(const ASTContext &Ctx, const LValue &LVal) {
   // - We used to be a bit more aggressive here; we'd only be conservative if
   //   the array at the end was flexible, or if it had 0 or 1 elements. This
   //   broke some common standard library extensions (PR30346), but was
   //   otherwise seemingly fine. It may be useful to reintroduce this behavior
   //   with some sort of list. OTOH, it seems that GCC is always
   //   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;
+  using FAMKind = LangOptions::StrictFlexArraysLevelKind;
+  FAMKind StrictFlexArraysLevel = Ctx.getLangOpts().getStrictFlexArraysLevel();
   return LVal.InvalidBase &&
          Designator.Entries.size() == Designator.MostDerivedPathLength &&
          Designator.MostDerivedIsArrayElement &&
          (Designator.isMostDerivedAnUnsizedArray() ||
           Designator.getMostDerivedArraySize() == 0 ||
           (Designator.getMostDerivedArraySize() == 1 &&
-           StrictFlexArraysLevel < 2) ||
-          StrictFlexArraysLevel == 0) &&
+           StrictFlexArraysLevel != FAMKind::Incomplete &&
+           StrictFlexArraysLevel != FAMKind::ZeroOrIncomplete) ||
+          StrictFlexArraysLevel == FAMKind::Default) &&
          isDesignatorAtObjectEnd(Ctx, LVal);
 }
 
 /// Converts the given APInt to CharUnits, assuming the APInt is unsigned.
 /// Fails if the conversion would cause loss of precision.
 static bool convertUnsignedAPIntToCharUnits(const llvm::APInt &Int,
                                             CharUnits &Result) {
   auto CharUnitsMax = std::numeric_limits<CharUnits::QuantityType>::max();

clang/lib/CodeGen/CGExpr.cpp

if (Done) {
     Builder.CreateBr(Done);
     EmitBlock(Done);
   }
 }
 
 /// 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,
-                                      unsigned StrictFlexArraysLevel) {
+                                      LangOptions::StrictFlexArraysLevelKind
+                                      StrictFlexArraysLevel) {
   // 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)) {
+    using FAMKind = LangOptions::StrictFlexArraysLevelKind;
+
     // FIXME: Sema doesn't treat [1] as a flexible array member if the bound
     // was produced by macro expansion.
-    if (StrictFlexArraysLevel >= 2 && CAT->getSize().ugt(0))
+    if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete &&
+        CAT->getSize().ugt(0))
       return false;
     // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
     // arrays to be treated as flexible-array-members, we still emit ubsan
     // checks as if they are not.
     if (CAT->getSize().ugt(1))
       return false;
   } else if (!isa<IncompleteArrayType>(AT))
     return false;
llvm::Value *CodeGenFunction::LoadPassedObjectSize(const Expr *E,
   return Builder.CreateUDiv(SizeInBytes, SizeOfElement);
 }
 
 /// 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,
-                                          unsigned StrictFlexArraysLevel) {
+                                          LangOptions::StrictFlexArraysLevelKind
+                                          StrictFlexArraysLevel) {
   // For the vector indexing extension, the bound is the number of elements.
   if (const VectorType *VT = Base->getType()->getAs<VectorType>()) {
     IndexedType = Base->getType();
     return CGF.Builder.getInt32(VT->getNumElements());
   }
 
   Base = Base->IgnoreParens();
 

 
 void CodeGenFunction::EmitBoundsCheck(const Expr *E, const Expr *Base,
                                       llvm::Value *Index, QualType IndexType,
                                       bool Accessed) {
   assert(SanOpts.has(SanitizerKind::ArrayBounds) &&
          "should not be called unless adding bounds checks");
   SanitizerScope SanScope(this);
 
-  const unsigned StrictFlexArraysLevel = getLangOpts().StrictFlexArrays;
+  const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
+    getLangOpts().getStrictFlexArraysLevel();
 
   QualType IndexedType;
   llvm::Value *Bound =
       getArrayIndexingBound(*this, Base, IndexedType, StrictFlexArraysLevel);
   if (!Bound)
     return;
 
   bool IndexSigned = IndexType->isSignedIntegerOrEnumerationType();

clang/lib/Sema/SemaChecking.cpp

 }
 
 /// 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.
 static bool isFlexibleArrayMemberExpr(Sema &S, const Expr *E,
                                       const NamedDecl *ND,
-                                      unsigned StrictFlexArraysLevel) {
+                                      LangOptions::StrictFlexArraysLevelKind
+                                      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))
+  using FAMKind = LangOptions::StrictFlexArraysLevelKind;
+  if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete ||
+      Size.uge(2))
     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.
if (IndexExpr->isValueDependent())
     return;
 
   const Type *EffectiveType =
       BaseExpr->getType()->getPointeeOrArrayElementType();
   BaseExpr = BaseExpr->IgnoreParenCasts();
   const ConstantArrayType *ArrayTy =
       Context.getAsConstantArrayType(BaseExpr->getType());
 
-  unsigned StrictFlexArraysLevel = getLangOpts().StrictFlexArrays;
+  LangOptions::StrictFlexArraysLevelKind
+    StrictFlexArraysLevel = getLangOpts().getStrictFlexArraysLevel();
 
   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 =

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

const auto isFlexibleArrayMemberCandidate = [this,
       if (isa<IncompleteArrayType>(AT))
         return true;
 
       if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
         const llvm::APInt &Size = CAT->getSize();
         if (Size.isZero())
           return true;
 
-        if (getContext().getLangOpts().StrictFlexArrays >= 2)
+        using FAMKind = LangOptions::StrictFlexArraysLevelKind;
+        const FAMKind StrictFlexArraysLevel =
+          Ctx.getLangOpts().getStrictFlexArraysLevel();
+        if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete ||
+            StrictFlexArraysLevel == FAMKind::Incomplete)
           return false;
 
         const AnalyzerOptions &Opts = SVB.getAnalyzerOptions();
         // FIXME: this option is probably redundant with -fstrict-flex-arrays=1.
         if (Opts.ShouldConsiderSingleElementArraysAsFlexibleArrayMembers &&
             Size.isOne())
           return true;
       }