r246830 - Fix a potential APInt memory leak when using __attribute__((flag_enum)), and
Steven Wu via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 3 21:34:19 PDT 2015
Looks fixed. Thank a lot for investigating!
> On Sep 3, 2015, at 9:21 PM, Steven Wu via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>
> Sorry I just get back to my computer. I will watch the bots going. I will try investigating if it is still failing.
> As far as I know, there is no special settingl about the bots and all our bots are complaining about it.
>
> Steven
>
>> On Sep 3, 2015, at 9:09 PM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>
>> OK, should be fixed in r246836.
>>
>> On Thu, Sep 3, 2015 at 7:50 PM, Richard Smith <richard at metafoo.co.uk <mailto: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 <mailto: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/ <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 <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 <mailto: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 <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 <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 <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 <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 <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 <mailto:cfe-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <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/94acf90a/attachment-0001.html>
More information about the cfe-commits
mailing list