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 19:50:43 PDT 2015
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/79de168a/attachment.html>
More information about the cfe-commits
mailing list