r246830 - Fix a potential APInt memory leak when using __attribute__((flag_enum)), and
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 3 21:09:47 PDT 2015
OK, should be fixed in r246836.
On Thu, Sep 3, 2015 at 7:50 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, Sep 3, 2015 at 6:21 PM, Steven Wu via cfe-commits <
> cfe-commits at 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 at 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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150903/e9eebfa5/attachment-0001.html>
More information about the cfe-commits
mailing list