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 18:21:50 PDT 2015


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/>

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150903/a6c1ae02/attachment-0001.html>


More information about the cfe-commits mailing list