Flag Enumerator Attribute

[scshunt]

Summary

This patch introduces a new attribute, flag_enum, which indicates to the compiler that an enumerator is a flag type. Compiler warnings about the range of values are adjusted accordingly.

[majnemer]

Did you forget to include tests?

[scshunt]

Correctly generate patch to include tests.

[scshunt]

This fixes most of the comments from Aaron Ballman.

I've left the C++ attribute syntax off, as I haven't yet considered the semantics in C++ carefully.

[aaron.ballman]

Sean and I spoke on IRC about this review, and the items we came up with were:

• Move the FlagBits value off EnumDecl and into the FlagEnumAttr itself
• Rangeify a for loop
• Remove another spurious call to getName()

~Aaron

[scshunt]

A few more comments from Aaron.

On the C++ side, I realized that forward declarations and redeclarations possibly break everything. I don't want to think about that now. It could be made an error in C++ for the time being if that's a large concern, or I could simply work on FIXMEs or a later patch to make it work in C++.

include/clang/Basic/Attr.td

   let Documentation = [Undocumented];
 }
 
+def FlagEnum : InheritableAttr {
+  let Spellings = [GNU<"flag_enum">];
+  let Subjects = SubjectList<[Enum]>;
+  let Documentation = [FlagEnumDocs];
+  let AdditionalMembers = [{
+private:
+    llvm::APInt FlagBits;
+public:
+    llvm::APInt &getFlagBits() {
+      return FlagBits;
+    }
+
+    const llvm::APInt &getFlagBits() const {
+      return FlagBits;
+    }
+}];
+}
+
 def Flatten : InheritableAttr {
   let Spellings = [GCC<"flatten">];
   let Subjects = SubjectList<[Function], ErrorDiag>;

include/clang/Basic/AttrDocs.td

   }];
 }
 
+def FlagEnumDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+This attribute can be added to an enumerator to signal to the compiler that it
+is intended to be used as a flag type. This will cause the compiler to assume
+that the range of the type includes all of the values that you can get by
+manipulating bits of the enumerator when issuing warnings.
+  }];
+} 
+
 def MSInheritanceDocs : Documentation {
   let Category = DocCatType;
   let Heading = "__single_inhertiance, __multiple_inheritance, __virtual_inheritance";

include/clang/Basic/DiagnosticGroups.td

 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
+def FlagEnum : DiagGroup<"flag-enum">;
 def InfiniteRecursion : DiagGroup<"infinite-recursion">;
 def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
 def IgnoredQualifiers : DiagGroup<"ignored-qualifiers">;

include/clang/Basic/DiagnosticSemaKinds.td

   "%0 attribute only applies to %select{functions|unions|"
   "variables and functions|functions and methods|parameters|"
   "functions, methods and blocks|functions, methods, and classes|"
-  "functions, methods, and parameters|classes|variables|methods|"
+  "functions, methods, and parameters|classes|enums|variables|methods|"
   "variables, functions and labels|fields and global variables|structs|"
   "variables and typedefs|thread-local variables|"
   "variables and fields|variables, data members and tag types|"

 def ext_enumerator_increment_too_large : ExtWarn<
   "incremented enumerator value %0 is not representable in the "
   "largest integer type">, InGroup<EnumTooLarge>;
+def warn_flag_enum_constant_out_of_range : Warning<
+  "enumeration value %0 is out of range of flags in enumeration type %1">,
+  InGroup<FlagEnum>;
   
 def warn_illegal_constant_array_size : Extension<
   "size of static array must be an integer constant expression">;

include/clang/Sema/AttributeList.h

   ExpectedFunctionMethodOrClass,
   ExpectedFunctionMethodOrParameter,
   ExpectedClass,
+  ExpectedEnum,
   ExpectedVariable,
   ExpectedMethod,
   ExpectedVariableFunctionOrLabel,

include/clang/Sema/Sema.h

                                 Expr *SrcExpr, AssignmentAction Action,
                                 bool *Complained = nullptr);
 
+  /// IsValueInFlagEnum - Determine if a value is allowed as part of a flag
+  /// enum. If AllowMask is true, then we also allow the complement of a valid
+  /// value, to be used as a mask.
+  bool IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
+                         bool AllowMask) const;
+
   /// DiagnoseAssignmentEnum - Warn if assignment to enum is a constant
   /// integer not in the range of enum values.
   void DiagnoseAssignmentEnum(QualType DstType, QualType SrcType,

lib/Sema/SemaDecl.cpp

   }
 }
 
+bool
+Sema::IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
+                        bool AllowMask) const {
+  FlagEnumAttr *FEAttr = ED->getAttr<FlagEnumAttr>();
+  assert(FEAttr && "looking for value in non-flag enum");
+
+  llvm::APInt FlagMask = ~FEAttr->getFlagBits();
+  unsigned Width = FlagMask.getBitWidth();
+
+  // We will try a zero-extended value for the regular check first.
+  llvm::APInt ExtVal = Val.zextOrSelf(Width);
+
+  // A value is in a flag enum if either its bits are a subset of the enum's
+  // flag bits (the first condition) or we are allowing masks and the same is
+  // true of its complement (the second condition). When masks are allowed, we
+  // allow the common idiom of ~(enum1 | enum2) to be a valid enum value.
+  //
+  // While it's true that any value could be used as a mask, the assumption is
+  // that a mask will have all of the insignificant bits set. Anything else is
+  // likely a logic error.
+  if (!(FlagMask & ExtVal))
+    return true;
+
+  if (AllowMask) {
+    // Try a one-extended value instead. This can happen if the enum is wider
+    // than the constant used, in C with extensions to allow for wider enums.
+    // The mask will still have the correct behaviour, so we give the user the
+    // benefit of the doubt.
+    //
+    // FIXME: This heuristic can cause weird results if the enum was extended
+    // to a larger type and is signed, because then bit-masks of smaller types
+    // that get extended will fall out of range (e.g. ~0x1u). We currently don't
+    // detect that case and will get a false positive for it. In most cases,
+    // though, it can be fixed by making it a signed type (e.g. ~0x1), so it may
+    // be fine just to accept this as a warning.
+    ExtVal |= llvm::APInt::getHighBitsSet(Width, Width - Val.getBitWidth());
+    if (!(FlagMask & ~ExtVal))
+      return true;
+  }
+
+  return false;
+}
+
 void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceLocation LBraceLoc,
                          SourceLocation RBraceLoc, Decl *EnumDeclX,
                          ArrayRef<Decl *> Elements,

       BestPromotionType = Context.getPromotedIntegerType(BestType);
     else
       BestPromotionType = BestType;
-    // We don't need to set BestWidth, because BestType is going to be the type
-    // of the enumerators, but we do anyway because otherwise some compilers
-    // warn that it might be used uninitialized.
-    BestWidth = CharWidth;
+
+    BestWidth = Context.getIntWidth(BestType);
   }
   else if (NumNegativeBits) {
     // If there is a negative value, figure out the smallest integer type (of

     }
   }
 
+  FlagEnumAttr *FEAttr = Enum->getAttr<FlagEnumAttr>();
+  if (FEAttr)
+    FEAttr->getFlagBits() = llvm::APInt(BestWidth, 0);
+
   // Loop over all of the enumerator constants, changing their types to match
-  // the type of the enum if needed.
-  for (unsigned i = 0, e = Elements.size(); i != e; ++i) {
-    EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(Elements[i]);
+  // the type of the enum if needed. If we have a flag type, we also prepare the
+  // FlagBits cache.
+  for (auto *D : Elements) {
+    auto *ECD = cast_or_null<EnumConstantDecl>(D);
     if (!ECD) continue;  // Already issued a diagnostic.
 
     // Standard C says the enumerators have int type, but we allow, as an

         // enum-specifier, each enumerator has the type of its
         // enumeration.
         ECD->setType(EnumType);
-      continue;
+      goto flagbits;
     } else {
       NewTy = BestType;
       NewWidth = BestWidth;

       ECD->setType(EnumType);
     else
       ECD->setType(NewTy);
+
+flagbits:
+    // Check to see if we have a constant with exactly one bit set. Note that x
+    // & (x - 1) will be nonzero if and only if x has more than one bit set.
+    if (FEAttr) {
+      llvm::APInt ExtVal = InitVal.zextOrSelf(BestWidth);
+      if (ExtVal != 0 && !(ExtVal & (ExtVal - 1))) {
+        FEAttr->getFlagBits() |= ExtVal;
+      }
+    }
   }
 
+  if (FEAttr) {
+    for (Decl *D : Elements) {
+      EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(D);
+      if (!ECD) continue;  // Already issued a diagnostic.
+
+      llvm::APSInt InitVal = ECD->getInitVal();
+      if (InitVal != 0 && !IsValueInFlagEnum(Enum, InitVal, true))
+        Diag(ECD->getLocation(), diag::warn_flag_enum_constant_out_of_range)
+          << ECD << Enum;
+    }
+  }
+
+
+
   Enum->completeDefinition(BestType, BestPromotionType,
                            NumPositiveBits, NumNegativeBits);
 

lib/Sema/SemaDeclAttr.cpp

   case AttributeList::AT_OptimizeNone:
     handleOptimizeNoneAttr(S, D, Attr);
     break;
+  case AttributeList::AT_FlagEnum:
+    handleSimpleAttribute<FlagEnumAttr>(S, D, Attr);
+    break;
   case AttributeList::AT_Flatten:
     handleSimpleAttribute<FlattenAttr>(S, D, Attr);
     break;

lib/Sema/SemaStmt.cpp

   }
 }
 
+typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl*>, 64> EnumValsTy;
+
 /// Returns true if we should emit a diagnostic about this case expression not
 /// being a part of the enum used in the switch controlling expression.
-static bool ShouldDiagnoseSwitchCaseNotInEnum(const ASTContext &Ctx,
+static bool ShouldDiagnoseSwitchCaseNotInEnum(const Sema &S,
                                               const EnumDecl *ED,
-                                              const Expr *CaseExpr) {
-  // Don't warn if the 'case' expression refers to a static const variable of
-  // the enum type.
-  CaseExpr = CaseExpr->IgnoreParenImpCasts();
-  if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(CaseExpr)) {
+                                              const Expr *CaseExpr,
+                                              EnumValsTy::iterator &EI,
+                                              EnumValsTy::iterator &EIEnd,
+                                              const llvm::APSInt &Val) {
+  bool FlagType = ED->hasAttr<FlagEnumAttr>();
+
+  if (const DeclRefExpr *DRE =
+          dyn_cast<DeclRefExpr>(CaseExpr->IgnoreParenImpCasts())) {
     if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
-      if (!VD->hasGlobalStorage())
-        return true;
       QualType VarType = VD->getType();
-      if (!VarType.isConstQualified())
-        return true;
-      QualType EnumType = Ctx.getTypeDeclType(ED);
-      if (Ctx.hasSameUnqualifiedType(EnumType, VarType))
+      QualType EnumType = S.Context.getTypeDeclType(ED);
+      if (VD->hasGlobalStorage() && VarType.isConstQualified() &&
+          S.Context.hasSameUnqualifiedType(EnumType, VarType))
         return false;
     }
   }
+
+  if (FlagType) {
+    return !S.IsValueInFlagEnum(ED, Val, false);
+  } else {
+    while (EI != EIEnd && EI->first < Val)
+      EI++;
+
+    if (EI != EIEnd && EI->first == Val)
+      return false;
+  }
+
   return true;
 }
 

     // If switch has default case, then ignore it.
     if (!CaseListIsErroneous  && !HasConstantCond && ET) {
       const EnumDecl *ED = ET->getDecl();
-      typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl*>, 64>
-        EnumValsTy;
       EnumValsTy EnumVals;
 
       // Gather all enum values, set their type and sort them,

         EnumVals.push_back(std::make_pair(Val, EDI));
       }
       std::stable_sort(EnumVals.begin(), EnumVals.end(), CmpEnumVals);
-      EnumValsTy::iterator EIend =
+      auto EI = EnumVals.begin(), EIEnd =
         std::unique(EnumVals.begin(), EnumVals.end(), EqEnumVals);
 
       // See which case values aren't in enum.
-      EnumValsTy::const_iterator EI = EnumVals.begin();
       for (CaseValsTy::const_iterator CI = CaseVals.begin();
-           CI != CaseVals.end(); CI++) {
-        while (EI != EIend && EI->first < CI->first)
-          EI++;
-        if (EI == EIend || EI->first > CI->first) {
-          Expr *CaseExpr = CI->second->getLHS();
-          if (ShouldDiagnoseSwitchCaseNotInEnum(Context, ED, CaseExpr))
-            Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
-              << CondTypeBeforePromotion;
-        }
+        Expr *CaseExpr = CI->second->getLHS();
+        if (ShouldDiagnoseSwitchCaseNotInEnum(*this, ED, CaseExpr, EI, EIEnd,
+                                              CI->first))
+          Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
+            << CondTypeBeforePromotion;
       }
+
       // See which of case ranges aren't in enum
       EI = EnumVals.begin();
       for (CaseRangesTy::const_iterator RI = CaseRanges.begin();
-           RI != CaseRanges.end() && EI != EIend; RI++) {
-        while (EI != EIend && EI->first < RI->first)
-          EI++;
-
-        if (EI == EIend || EI->first != RI->first) {
-          Expr *CaseExpr = RI->second->getLHS();
-          if (ShouldDiagnoseSwitchCaseNotInEnum(Context, ED, CaseExpr))
-            Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
-              << CondTypeBeforePromotion;
-        }
+          RI != CaseRanges.end(); RI++) {
+        Expr *CaseExpr = RI->second->getLHS();
+        if (ShouldDiagnoseSwitchCaseNotInEnum(*this, ED, CaseExpr, EI, EIEnd,
+                                              RI->first))
+          Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
+            << CondTypeBeforePromotion;
 
         llvm::APSInt Hi =
           RI->second->getRHS()->EvaluateKnownConstInt(Context);
         AdjustAPSInt(Hi, CondWidth, CondIsSigned);
-        while (EI != EIend && EI->first < Hi)
-          EI++;
-        if (EI == EIend || EI->first != Hi) {
-          Expr *CaseExpr = RI->second->getRHS();
-          if (ShouldDiagnoseSwitchCaseNotInEnum(Context, ED, CaseExpr))
-            Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
-              << CondTypeBeforePromotion;
-        }
+
+        CaseExpr = RI->second->getRHS();
+        if (ShouldDiagnoseSwitchCaseNotInEnum(*this, ED, CaseExpr, EI, EIEnd, 
+                                              Hi))
+          Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
+            << CondTypeBeforePromotion;
       }
 
       // Check which enum vals aren't in switch
-      CaseValsTy::const_iterator CI = CaseVals.begin();
-      CaseRangesTy::const_iterator RI = CaseRanges.begin();
+      auto CI = CaseVals.begin();
+      auto RI = CaseRanges.begin();
       bool hasCasesNotInSwitch = false;
 
       SmallVector<DeclarationName,8> UnhandledNames;
 
-      for (EI = EnumVals.begin(); EI != EIend; EI++){
+      for (EI = EnumVals.begin(); EI != EIEnd; EI++){
         // Drop unneeded case values
         while (CI != CaseVals.end() && CI->first < EI->first)
           CI++;

         llvm::APSInt RhsVal = SrcExpr->EvaluateKnownConstInt(Context);
         AdjustAPSInt(RhsVal, DstWidth, DstIsSigned);
         const EnumDecl *ED = ET->getDecl();
-        typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl *>, 64>
-            EnumValsTy;
-        EnumValsTy EnumVals;
-
-        // Gather all enum values, set their type and sort them,
-        // allowing easier comparison with rhs constant.
-        for (auto *EDI : ED->enumerators()) {
-          llvm::APSInt Val = EDI->getInitVal();
-          AdjustAPSInt(Val, DstWidth, DstIsSigned);
-          EnumVals.push_back(std::make_pair(Val, EDI));
-        }
-        if (EnumVals.empty())
-          return;
-        std::stable_sort(EnumVals.begin(), EnumVals.end(), CmpEnumVals);
-        EnumValsTy::iterator EIend =
-            std::unique(EnumVals.begin(), EnumVals.end(), EqEnumVals);
-
-        // See which values aren't in the enum.
-        EnumValsTy::const_iterator EI = EnumVals.begin();
-        while (EI != EIend && EI->first < RhsVal)
-          EI++;
-        if (EI == EIend || EI->first != RhsVal) {
-          Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
+
+        if (ED->hasAttr<FlagEnumAttr>()) {
+          if (!IsValueInFlagEnum(ED, RhsVal, true))
+            Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
               << DstType.getUnqualifiedType();
+        } else {
+          typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl *>, 64>
+              EnumValsTy;
+          EnumValsTy EnumVals;
+
+          // Gather all enum values, set their type and sort them,
+          // allowing easier comparison with rhs constant.
+          for (auto *EDI : ED->enumerators()) {
+            llvm::APSInt Val = EDI->getInitVal();
+            AdjustAPSInt(Val, DstWidth, DstIsSigned);
+            EnumVals.push_back(std::make_pair(Val, EDI));
+          }
+          if (EnumVals.empty())
+            return;
+          std::stable_sort(EnumVals.begin(), EnumVals.end(), CmpEnumVals);
+          EnumValsTy::iterator EIend =
+              std::unique(EnumVals.begin(), EnumVals.end(), EqEnumVals);
+
+          // See which values aren't in the enum.
+          EnumValsTy::const_iterator EI = EnumVals.begin();
+          while (EI != EIend && EI->first < RhsVal)
+            EI++;
+          if (EI == EIend || EI->first != RhsVal) {
+            Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
+                << DstType.getUnqualifiedType();
+          }
         }
       }
     }

test/Sema/attr-flag-enum.c

+// RUN: %clang_cc1 -verify -fsyntax-only -std=c11 -Wassign-enum %s
+
+enum __attribute__((flag_enum)) flag {
+  ea = 0x1,
+  eb = 0x2,
+  ec = 0x8,
+};
+
+enum __attribute__((flag_enum)) flag2 {
+  ga = 0x1,
+  gb = 0x4,
+
+  gc = 0x5, // no-warning
+  gd = 0x7, // expected-warning {{enumeration value 'gd' is out of range}}
+  ge = ~0x2, // expected-warning {{enumeration value 'ge' is out of range}}
+  gf = ~0x4, // no-warning
+  gg = ~0x1, // no-warning
+  gh = ~0x5, // no-warning
+  gi = ~0x11, // expected-warning {{enumeration value 'gi' is out of range}}
+}; 
+
+enum __attribute__((flag_enum)) flag3 {
+  fa = 0x1,
+  fb = ~0x1u, // no-warning
+};
+
+// What happens here is that ~0x2 is negative, and so the enum must be signed.
+// But ~0x1u is unsigned and has the high bit set, so the enum must be 64-bit.
+// The result is that ~0x1u does not have high bits set, and so it is considered
+// to be an invalid value. See Sema::IsValueInFlagEnum in SemaDecl.cpp for more
+// discussion.
+enum __attribute__((flag_enum)) flag4 {
+  ha = 0x1,
+  hb = 0x2,
+
+  hc = ~0x1u, // expected-warning {{enumeration value 'hc' is out of range}}
+  hd = ~0x2, // no-warning
+};
+
+void f(void) {
+  enum flag e = 0; // no-warning
+  e = 0x1; // no-warning
+  e = 0x3; // no-warning
+  e = 0xa; // no-warning
+  e = 0x4; // expected-warning {{integer constant not in range of enumerated type}}
+  e = 0xf; // expected-warning {{integer constant not in range of enumerated type}}
+  e = ~0; // no-warning
+  e = ~0x1; // no-warning
+  e = ~0x2; // no-warning
+  e = ~0x3; // no-warning
+  e = ~0x4; // expected-warning {{integer constant not in range of enumerated type}}
+
+  switch (e) {
+    case 0: break; // no-warning
+    case 0x1: break; // no-warning
+    case 0x3: break; // no-warning
+    case 0xa: break; // no-warning
+    case 0x4: break; // expected-warning {{case value not in enumerated type}}
+    case 0xf: break; // expected-warning {{case value not in enumerated type}}
+    case ~0: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x1: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x2: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x3: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x4: break; // expected-warning {{case value not in enumerated type}}
+    default: break;
+  }
+
+  enum flag2 f = ~0x1; // no-warning
+  f = ~0x1u; // no-warning
+
+  enum flag4 h = ~0x1; // no-warning
+  h = ~0x1u; // expected-warning {{integer constant not in range of enumerated type}}
+}

utils/TableGen/ClangAttrEmitter.cpp

     Namespace = 1U << 11,
     Field = 1U << 12,
     CXXMethod = 1U << 13,
-    ObjCProtocol = 1U << 14
+    ObjCProtocol = 1U << 14,
+    Enum = 1U << 15
   };
   uint32_t SubMask = 0;
 

                    .Case("Namespace", Namespace)
                    .Case("Field", Field)
                    .Case("CXXMethod", CXXMethod)
+                   .Case("Enum", Enum)
                    .Default(0);
     if (!V) {
       // Something wasn't in our mapping, so be helpful and let the developer

     case Var:   return "ExpectedVariable";
     case Param: return "ExpectedParameter";
     case Class: return "ExpectedClass";
+    case Enum:  return "ExpectedEnum";
     case CXXMethod:
       // FIXME: Currently, this maps to ExpectedMethod based on existing code,
       // but should map to something a bit more accurate at some point.