OK, should be fixed in r246836. On Thu, Sep 3, 2015 at 7:50 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Thu, Sep 3, 2015 at 6:21 PM, Steven Wu via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> In case you didn’t get an email from the failure because it was >> overshadowed by the previous error. This commit seems to break the green >> dragon bots: >> >> http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental_check/12990/testReport/junit/Clang/Sema/attr_flag_enum_c/ >> > > I have no idea what's wrong here; is there anything unusual about that bot > that might give me a clue? (This bot: > http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/4637 seems > to be failing the same way.) > > >> Thanks >> >> Steven >> >> On Sep 3, 2015, at 6:03 PM, Richard Smith via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >> Author: rsmith >> Date: Thu Sep 3 20:03:03 2015 >> New Revision: 246830 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=246830&view=rev >> Log: >> Fix a potential APInt memory leak when using __attribute__((flag_enum)), >> and >> simplify the implementation a bit. >> >> Modified: >> cfe/trunk/include/clang/Basic/Attr.td >> cfe/trunk/include/clang/Sema/Sema.h >> cfe/trunk/lib/Sema/SemaDecl.cpp >> cfe/trunk/lib/Sema/SemaStmt.cpp >> >> Modified: cfe/trunk/include/clang/Basic/Attr.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=246830&r1=246829&r2=246830&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/Attr.td (original) >> +++ cfe/trunk/include/clang/Basic/Attr.td Thu Sep 3 20:03:03 2015 >> @@ -747,18 +747,6 @@ def FlagEnum : InheritableAttr { >> let Subjects = SubjectList<[Enum]>; >> let Documentation = [FlagEnumDocs]; >> let LangOpts = [COnly]; >> - let AdditionalMembers = [{ >> -private: >> - llvm::APInt FlagBits; >> -public: >> - llvm::APInt &getFlagBits() { >> - return FlagBits; >> - } >> - >> - const llvm::APInt &getFlagBits() const { >> - return FlagBits; >> - } >> -}]; >> } >> >> def Flatten : InheritableAttr { >> >> Modified: cfe/trunk/include/clang/Sema/Sema.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=246830&r1=246829&r2=246830&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Sema/Sema.h (original) >> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Sep 3 20:03:03 2015 >> @@ -903,6 +903,10 @@ public: >> /// for C++ records. >> llvm::FoldingSet<SpecialMemberOverloadResult> SpecialMemberCache; >> >> + /// \brief A cache of the flags available in enumerations with the >> flag_bits >> + /// attribute. >> + mutable llvm::DenseMap<const EnumDecl*, llvm::APInt> FlagBitsCache; >> + >> /// \brief The kind of translation unit we are processing. >> /// >> /// When we're processing a complete translation unit, Sema will perform >> >> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=246830&r1=246829&r2=246830&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Sep 3 20:03:03 2015 >> @@ -14017,14 +14017,21 @@ static void CheckForDuplicateEnumValues( >> 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"); >> + assert(ED->hasAttr<FlagEnumAttr>() && "looking for value in non-flag >> enum"); >> >> - llvm::APInt FlagMask = ~FEAttr->getFlagBits(); >> - unsigned Width = FlagMask.getBitWidth(); >> + auto R = FlagBitsCache.insert(std::make_pair(ED, llvm::APInt())); >> + llvm::APInt &FlagBits = R.first->second; >> >> - // We will try a zero-extended value for the regular check first. >> - llvm::APInt ExtVal = Val.zextOrSelf(Width); >> + if (R.second) { >> + for (auto *E : ED->enumerators()) { >> + const auto &Val = E->getInitVal(); >> + // Only single-bit enumerators introduce new flag values. >> + if (Val.isPowerOf2()) >> + FlagBits = FlagBits.zextOrSelf(Val.getBitWidth()) | Val; >> + } >> + } >> + >> + llvm::APInt FlagMask = ~FlagBits.zextOrTrunc(Val.getBitWidth()); >> >> // 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 >> @@ -14034,26 +14041,10 @@ Sema::IsValueInFlagEnum(const EnumDecl * >> // 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)) >> + if (!(FlagMask & Val) || >> + (AllowMask && !(FlagMask & ~Val))) >> 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; >> } >> >> @@ -14208,13 +14199,8 @@ void Sema::ActOnEnumBody(SourceLocation >> } >> } >> >> - 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. If we have a flag type, we also >> prepare the >> - // FlagBits cache. >> + // the type of the enum if needed. >> for (auto *D : Elements) { >> auto *ECD = cast_or_null<EnumConstantDecl>(D); >> if (!ECD) continue; // Already issued a diagnostic. >> @@ -14246,7 +14232,7 @@ void Sema::ActOnEnumBody(SourceLocation >> // enum-specifier, each enumerator has the type of its >> // enumeration. >> ECD->setType(EnumType); >> - goto flagbits; >> + continue; >> } else { >> NewTy = BestType; >> NewWidth = BestWidth; >> @@ -14273,32 +14259,21 @@ void Sema::ActOnEnumBody(SourceLocation >> 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) { >> + if (Enum->hasAttr<FlagEnumAttr>()) { >> 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)) >> + if (InitVal != 0 && !InitVal.isPowerOf2() && >> + !IsValueInFlagEnum(Enum, InitVal, true)) >> Diag(ECD->getLocation(), >> diag::warn_flag_enum_constant_out_of_range) >> << ECD << Enum; >> } >> } >> >> - >> - >> Enum->completeDefinition(BestType, BestPromotionType, >> NumPositiveBits, NumNegativeBits); >> >> >> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=246830&r1=246829&r2=246830&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Thu Sep 3 20:03:03 2015 >> @@ -698,8 +698,6 @@ static bool ShouldDiagnoseSwitchCaseNotI >> 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())) { >> @@ -711,7 +709,7 @@ static bool ShouldDiagnoseSwitchCaseNotI >> } >> } >> >> - if (FlagType) { >> + if (ED->hasAttr<FlagEnumAttr>()) { >> return !S.IsValueInFlagEnum(ED, Val, false); >> } else { >> while (EI != EIEnd && EI->first < Val) >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
