[PATCH] Flag Enumerator Attribute
Aaron Ballman
aaron at aaronballman.com
Sun Nov 2 14:12:41 PST 2014
On Sun, Nov 2, 2014 at 11:47 AM, Sean Hunt <scshunt at csclub.uwaterloo.ca> wrote:
> On Fri, Oct 17, 2014 at 8:30 PM, Sean Hunt <scshunt at csclub.uwaterloo.ca> wrote:
>> 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.
>>
>> http://reviews.llvm.org/D5853
>>
>> Files:
>> include/clang/AST/Decl.h
>> include/clang/Basic/Attr.td
>> include/clang/Basic/AttrDocs.td
>> include/clang/Basic/DiagnosticGroups.td
>> include/clang/Basic/DiagnosticSemaKinds.td
>> include/clang/Sema/AttributeList.h
>> include/clang/Sema/Sema.h
>> lib/Sema/SemaDecl.cpp
>> lib/Sema/SemaDeclAttr.cpp
>> lib/Sema/SemaStmt.cpp
>> utils/TableGen/ClangAttrEmitter.cpp
>
> Is there anyone able to look at this?
Sorry about the delayed review -- I hadn't seen this come by my inbox
earlier, for some reason. Nits and questions below.
> Index: include/clang/AST/Decl.h
> ===================================================================
> --- include/clang/AST/Decl.h
> +++ include/clang/AST/Decl.h
> @@ -2915,6 +2915,11 @@
> /// information.
> MemberSpecializationInfo *SpecializationInfo;
>
> + /// FlagBits - Cached calculation of all the bits that are set in enumerators
> + /// for use with the flag_enum attribute. This is not valid except on a
> + /// complete definition with that attribute.
> + llvm::APInt FlagBits;
> +
> EnumDecl(ASTContext &C, DeclContext *DC, SourceLocation StartLoc,
> SourceLocation IdLoc, IdentifierInfo *Id, EnumDecl *PrevDecl,
> bool Scoped, bool ScopedUsingClassTag, bool Fixed)
> @@ -3059,6 +3064,14 @@
> NumNegativeBits = Num;
> }
>
> + llvm::APInt *getFlagBits() {
> + return &FlagBits;
> + }
> +
> + const llvm::APInt *getFlagBits() const {
> + return &FlagBits;
> + }
Why a pointer instead of a reference?
> +
> /// \brief Returns true if this is a C++11 scoped enumeration.
> bool isScoped() const {
> return IsScoped;
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -696,6 +696,12 @@
> let Documentation = [Undocumented];
> }
>
> +def FlagEnum : InheritableAttr {
> + let Spellings = [GNU<"flag_enum">];
What about a C++11 clang-specific attribute?
> + let Subjects = SubjectList<[Enum]>;
> + let Documentation = [FlagEnumDocs];
> +}
> +
> def Flatten : InheritableAttr {
> let Spellings = [GCC<"flatten">];
> let Subjects = SubjectList<[Function], ErrorDiag>;
> Index: include/clang/Basic/AttrDocs.td
> ===================================================================
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -1031,6 +1031,16 @@
> }];
> }
>
> +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";
> Index: include/clang/Basic/DiagnosticGroups.td
> ===================================================================
> --- include/clang/Basic/DiagnosticGroups.td
> +++ include/clang/Basic/DiagnosticGroups.td
> @@ -187,6 +187,7 @@
> 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">;
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2219,7 +2219,7 @@
> "%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|"
> @@ -4005,6 +4005,9 @@
> 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">;
> Index: include/clang/Sema/AttributeList.h
> ===================================================================
> --- include/clang/Sema/AttributeList.h
> +++ include/clang/Sema/AttributeList.h
> @@ -822,6 +822,7 @@
> ExpectedFunctionMethodOrClass,
> ExpectedFunctionMethodOrParameter,
> ExpectedClass,
> + ExpectedEnum,
> ExpectedVariable,
> ExpectedMethod,
> ExpectedVariableFunctionOrLabel,
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -7847,6 +7847,12 @@
> 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);
> +
> /// DiagnoseAssignmentEnum - Warn if assignment to enum is a constant
> /// integer not in the range of enum values.
> void DiagnoseAssignmentEnum(QualType DstType, QualType SrcType,
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -13474,6 +13474,48 @@
> }
> }
>
> +bool
> +Sema::IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt *Val,
> + bool AllowMask) {
> + assert(ED->hasAttr<FlagEnumAttr>() && "looking for value in non-flag enum");
> +
> + llvm::APInt FlagMask = ~*ED->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,
> @@ -13559,10 +13601,8 @@
> 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
> @@ -13627,8 +13667,14 @@
> }
> }
>
> + bool FlagType = Enum->hasAttr<FlagEnumAttr>();
> + llvm::APInt *FlagBits = Enum->getFlagBits();
> + if (FlagType)
> + *FlagBits = llvm::APInt(BestWidth, 0);
> +
> // Loop over all of the enumerator constants, changing their types to match
> - // the type of the enum if needed.
> + // the type of the enum if needed. If we have a flag type, we also prepare the
> + // FlagBits cache.
> for (unsigned i = 0, e = Elements.size(); i != e; ++i) {
> EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(Elements[i]);
> if (!ECD) continue; // Already issued a diagnostic.
> @@ -13660,7 +13706,7 @@
> // enum-specifier, each enumerator has the type of its
> // enumeration.
> ECD->setType(EnumType);
> - continue;
> + goto flagbits;
> } else {
> NewTy = BestType;
> NewWidth = BestWidth;
> @@ -13687,8 +13733,32 @@
> 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 (FlagType) {
> + llvm::APInt ExtVal = InitVal.zextOrSelf(BestWidth);
> + if (ExtVal != 0 && !(ExtVal & (ExtVal - 1))) {
> + *FlagBits |= ExtVal;
> + }
> + }
> }
>
> + if (FlagType) {
> + for (unsigned i = 0, e = Elements.size(); i != e; ++i) {
Range-based for loop?
> + EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(Elements[i]);
> + 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->getName() << Enum->getName();
No need to call getName() -- the diagnostic builder should already
handle NamedDecl.
> + }
> + }
> +
> +
> +
> Enum->completeDefinition(BestType, BestPromotionType,
> NumPositiveBits, NumNegativeBits);
>
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -4284,6 +4284,9 @@
> 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;
> Index: lib/Sema/SemaStmt.cpp
> ===================================================================
> --- lib/Sema/SemaStmt.cpp
> +++ lib/Sema/SemaStmt.cpp
> @@ -1038,13 +1038,21 @@
> EnumValsTy::iterator EIend =
> std::unique(EnumVals.begin(), EnumVals.end(), EqEnumVals);
>
> + bool FlagType = ED->hasAttr<FlagEnumAttr>();
> // 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) {
> + CI != CaseVals.end(); CI++) {
> + bool Diagnose;
> + if (FlagType) {
> + Diagnose = !IsValueInFlagEnum(ED, &CI->first, false);
> + } else {
> + while (EI != EIend && EI->first < CI->first)
> + EI++;
> + Diagnose = EI == EIend || EI->first > CI->first;
> + }
> +
> + if (Diagnose) {
> Expr *CaseExpr = CI->second->getLHS();
> if (ShouldDiagnoseSwitchCaseNotInEnum(Context, ED, CaseExpr))
> Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
> @@ -1054,23 +1062,36 @@
> // 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() && EI != EIend; RI++) {
> + bool Diagnose;
> + if (FlagType) {
> + Diagnose = !IsValueInFlagEnum(ED, &RI->first, false);
> + } else {
> + while (EI != EIend && EI->first < RI->first)
> + EI++;
> + Diagnose = EI == EIend || EI->first != RI->first;
Any way to abstract this out since it's basically a copy/paste from above?
> +
> + if (Diagnose) {
> + Expr *CaseExpr = RI->second->getLHS();
> + if (ShouldDiagnoseSwitchCaseNotInEnum(Context, ED, CaseExpr))
> + 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) {
> +
> + if (FlagType) {
> + Diagnose = !IsValueInFlagEnum(ED, &Hi, false);
> + } else {
> + while (EI != EIend && EI->first < Hi)
> + EI++;
> + Diagnose = EI == EIend || EI->first != Hi;
> + }
> +
> + if (Diagnose) {
> Expr *CaseExpr = RI->second->getRHS();
> if (ShouldDiagnoseSwitchCaseNotInEnum(Context, ED, CaseExpr))
> Diag(CaseExpr->getExprLoc(), diag::warn_not_in_enum)
> @@ -1172,30 +1193,37 @@
> 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();
> + }
> }
> }
> }
> Index: test/Sema/attr-flag-enum.c
> ===================================================================
> --- /dev/null
> +++ test/Sema/attr-flag-enum.c
> @@ -0,0 +1,73 @@
> +// RUN: %clang_cc1 -verify -fsyntax-only -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() {
For sanity, can you set the parameter list to void please? ;-)
> + 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
Also for sanity, can you add -std=c99 (or c11) explicitly to the RUN line?
> + f = ~0x1u; // no-warning
> +
> + enum flag4 h = ~0x1; // no-warning
> + h = ~0x1u; // expected-warning {{integer constant not in range of enumerated type}}
> +}
> Index: utils/TableGen/ClangAttrEmitter.cpp
> ===================================================================
> --- utils/TableGen/ClangAttrEmitter.cpp
> +++ utils/TableGen/ClangAttrEmitter.cpp
> @@ -2170,7 +2170,8 @@
> Namespace = 1U << 11,
> Field = 1U << 12,
> CXXMethod = 1U << 13,
> - ObjCProtocol = 1U << 14
> + ObjCProtocol = 1U << 14,
> + Enum = 1U << 15
> };
> uint32_t SubMask = 0;
>
> @@ -2204,6 +2205,7 @@
> .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
> @@ -2222,6 +2224,7 @@
> 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.
>
~Aaron
More information about the cfe-commits
mailing list