llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: Bill Wendling (bwendling) <details> <summary>Changes</summary> There are many issues that popped up with the counted_by feature. The patch has grown too large and approval is blocking Linux testing. Includes reverts of: commit 769bc11f684d ("[Clang] Implement the 'counted_by' attribute (#<!-- -->68750)") commit bc09ec696209 ("[CodeGen] Revamp counted_by calculations (#<!-- -->70606)") commit 1a09cfb2f35d ("[Clang] counted_by attr can apply only to C99 flexible array members (#<!-- -->72347)") commit a76adfb992c6 ("[NFC][Clang] Refactor code to calculate flexible array member size (#<!-- -->72790)") commit d8447c78ab16 ("[Clang] Correct handling of negative and out-of-bounds indices (#<!-- -->71877)") Partial commit b31cd07de5b7 ("[Clang] Regenerate test checks (NFC)") --- Patch is 101.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75857.diff 21 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (-5) - (modified) clang/include/clang/AST/Decl.h (-24) - (modified) clang/include/clang/AST/DeclBase.h (-10) - (modified) clang/include/clang/Basic/Attr.td (-18) - (modified) clang/include/clang/Basic/AttrDocs.td (-66) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (-11) - (modified) clang/include/clang/Sema/Sema.h (-3) - (modified) clang/include/clang/Sema/TypoCorrection.h (+4-8) - (modified) clang/lib/AST/ASTImporter.cpp (-13) - (modified) clang/lib/AST/DeclBase.cpp (+1-73) - (modified) clang/lib/AST/Expr.cpp (+73-10) - (modified) clang/lib/CodeGen/CGBuiltin.cpp (-167) - (modified) clang/lib/CodeGen/CGExpr.cpp (+3-126) - (modified) clang/lib/CodeGen/CodeGenFunction.h (-16) - (modified) clang/lib/Sema/SemaDecl.cpp (-14) - (modified) clang/lib/Sema/SemaDeclAttr.cpp (-90) - (modified) clang/lib/Sema/SemaExpr.cpp (+7-9) - (removed) clang/test/CodeGen/attr-counted-by.c (-742) - (modified) clang/test/CodeGen/bounds-checking.c (+1-9) - (modified) clang/test/Misc/pragma-attribute-supported-attributes-list.test (-1) - (removed) clang/test/Sema/attr-counted-by.c (-55) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 2e32f8b36d23de..edb97347f07716 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -199,11 +199,6 @@ C Language Changes - ``structs``, ``unions``, and ``arrays`` that are const may now be used as constant expressions. This change is more consistent with the behavior of GCC. -- Clang now supports the C-only attribute ``counted_by``. When applied to a - struct's flexible array member, it points to the struct field that holds the - number of elements in the flexible array member. This information can improve - the results of the array bound sanitizer and the - ``__builtin_dynamic_object_size`` builtin. - Enums will now be represented in TBAA metadata using their actual underlying integer type. Previously they were treated as chars, which meant they could alias with all other types. diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index cd0878d7082514..f9bf9cc5de7cb4 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/include/clang/AST/Decl.h @@ -4332,30 +4332,6 @@ class RecordDecl : public TagDecl { return field_begin() == field_end(); } - FieldDecl *getLastField() { - FieldDecl *FD = nullptr; - for (FieldDecl *Field : fields()) - FD = Field; - return FD; - } - const FieldDecl *getLastField() const { - return const_cast<RecordDecl *>(this)->getLastField(); - } - - template <typename Functor> - const FieldDecl *findFieldIf(Functor &Pred) const { - for (const Decl *D : decls()) { - if (const auto *FD = dyn_cast<FieldDecl>(D); FD && Pred(FD)) - return FD; - - if (const auto *RD = dyn_cast<RecordDecl>(D)) - if (const FieldDecl *FD = RD->findFieldIf(Pred)) - return FD; - } - - return nullptr; - } - /// Note that the definition of this type is now complete. virtual void completeDefinition(); diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 5b1038582bc674..10dcbdb262d84e 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -19,7 +19,6 @@ #include "clang/AST/SelectorLocationsKind.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LLVM.h" -#include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" #include "llvm/ADT/ArrayRef.h" @@ -489,15 +488,6 @@ class alignas(8) Decl { // Return true if this is a FileContext Decl. bool isFileContextDecl() const; - /// Whether it resembles a flexible array member. This is a static member - /// because we want to be able to call it with a nullptr. That allows us to - /// perform non-Decl specific checks based on the object's type and strict - /// flex array level. - static bool isFlexibleArrayMemberLike( - ASTContext &Context, const Decl *D, QualType Ty, - LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel, - bool IgnoreTemplateOrMacroSubstitution); - ASTContext &getASTContext() const LLVM_READONLY; /// Helper to get the language options from the ASTContext. diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 2b57058d3f1c75..db17211747b17d 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -4331,24 +4331,6 @@ def AvailableOnlyInDefaultEvalMethod : InheritableAttr { let Documentation = [Undocumented]; } -def CountedBy : InheritableAttr { - let Spellings = [Clang<"counted_by">]; - let Subjects = SubjectList<[Field]>; - let Args = [IdentifierArgument<"CountedByField">]; - let Documentation = [CountedByDocs]; - let LangOpts = [COnly]; - // FIXME: This is ugly. Let using a DeclArgument would be nice, but a Decl - // isn't yet available due to the fact that we're still parsing the - // structure. Maybe that code could be changed sometime in the future. - code AdditionalMembers = [{ - private: - SourceRange CountedByFieldLoc; - public: - SourceRange getCountedByFieldLoc() const { return CountedByFieldLoc; } - void setCountedByFieldLoc(SourceRange Loc) { CountedByFieldLoc = Loc; } - }]; -} - def PreferredType: InheritableAttr { let Spellings = [Clang<"preferred_type">]; let Subjects = SubjectList<[BitField], ErrorDiag>; diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td index 90041fa8dbb30b..98a7ecc7fd7df3 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -7500,72 +7500,6 @@ attribute, they default to the value ``65535``. }]; } -def CountedByDocs : Documentation { - let Category = DocCatField; - let Content = [{ -Clang supports the ``counted_by`` attribute on the flexible array member of a -structure in C. The argument for the attribute is the name of a field member in -the same structure holding the count of elements in the flexible array. This -information can be used to improve the results of the array bound sanitizer and -the ``__builtin_dynamic_object_size`` builtin. - -For example, the following code: - -.. code-block:: c - - struct bar; - - struct foo { - size_t count; - char other; - struct bar *array[] __attribute__((counted_by(count))); - }; - -specifies that the flexible array member ``array`` has the number of elements -allocated for it stored in ``count``. This establishes a relationship between -``array`` and ``count``. Specifically, ``p->array`` must have at least -``p->count`` number of elements available. It's the user's responsibility to -ensure that this relationship is maintained through changes to the structure. - -In the following example, the allocated array erroneously has fewer elements -than what's specified by ``p->count``. This would result in an out-of-bounds -access not being detected. - -.. code-block:: c - - #define SIZE_INCR 42 - - struct foo *p; - - void foo_alloc(size_t count) { - p = malloc(MAX(sizeof(struct foo), - offsetof(struct foo, array[0]) + count * sizeof(struct bar *))); - p->count = count + SIZE_INCR; - } - -The next example updates ``p->count``, breaking the relationship requirement -that ``p->array`` must have at least ``p->count`` number of elements available: - -.. code-block:: c - - #define SIZE_INCR 42 - - struct foo *p; - - void foo_alloc(size_t count) { - p = malloc(MAX(sizeof(struct foo), - offsetof(struct foo, array[0]) + count * sizeof(struct bar *))); - p->count = count; - } - - void use_foo(int index) { - p->count += SIZE_INCR + 1; /* 'count' is now larger than the number of elements of 'array'. */ - p->array[index] = 0; /* the sanitizer can't properly check if this is an out-of-bounds access. */ - } - - }]; -} - def CoroOnlyDestroyWhenCompleteDocs : Documentation { let Category = DocCatDecl; let Content = [{ diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 6e6f56ff75e5f9..c100041ca400f2 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6429,17 +6429,6 @@ def warn_superclass_variable_sized_type_not_at_end : Warning< "field %0 can overwrite instance variable %1 with variable sized type %2" " in superclass %3">, InGroup<ObjCFlexibleArray>; -def err_counted_by_attr_not_on_flexible_array_member : Error< - "'counted_by' only applies to C99 flexible array members">; -def err_counted_by_attr_refers_to_flexible_array : Error< - "'counted_by' cannot refer to the flexible array %0">; -def err_counted_by_must_be_in_structure : Error< - "field %0 in 'counted_by' not inside structure">; -def err_flexible_array_counted_by_attr_field_not_integer : Error< - "field %0 in 'counted_by' must be a non-boolean integer type">; -def note_flexible_array_counted_by_attr_field : Note< - "field %0 declared here">; - let CategoryName = "ARC Semantic Issue" in { // ARC-mode diagnostics. diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index a4f8fc1845b1ce..9887cc4ba4658d 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -4799,8 +4799,6 @@ class Sema final { bool CheckAlwaysInlineAttr(const Stmt *OrigSt, const Stmt *CurSt, const AttributeCommonInfo &A); - bool CheckCountedByAttr(Scope *Scope, const FieldDecl *FD); - /// Adjust the calling convention of a method to be the ABI default if it /// wasn't specified explicitly. This handles method types formed from /// function type typedefs and typename template arguments. @@ -5644,7 +5642,6 @@ class Sema final { CorrectionCandidateCallback &CCC, TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr, ArrayRef<Expr *> Args = std::nullopt, - DeclContext *LookupCtx = nullptr, TypoExpr **Out = nullptr); DeclResult LookupIvarInObjCMethod(LookupResult &Lookup, Scope *S, diff --git a/clang/include/clang/Sema/TypoCorrection.h b/clang/include/clang/Sema/TypoCorrection.h index 09de164297e7ba..e0f8d152dbe554 100644 --- a/clang/include/clang/Sema/TypoCorrection.h +++ b/clang/include/clang/Sema/TypoCorrection.h @@ -282,7 +282,7 @@ class CorrectionCandidateCallback { public: static const unsigned InvalidDistance = TypoCorrection::InvalidDistance; - explicit CorrectionCandidateCallback(const IdentifierInfo *Typo = nullptr, + explicit CorrectionCandidateCallback(IdentifierInfo *Typo = nullptr, NestedNameSpecifier *TypoNNS = nullptr) : Typo(Typo), TypoNNS(TypoNNS) {} @@ -319,7 +319,7 @@ class CorrectionCandidateCallback { /// this method. virtual std::unique_ptr<CorrectionCandidateCallback> clone() = 0; - void setTypoName(const IdentifierInfo *II) { Typo = II; } + void setTypoName(IdentifierInfo *II) { Typo = II; } void setTypoNNS(NestedNameSpecifier *NNS) { TypoNNS = NNS; } // Flags for context-dependent keywords. WantFunctionLikeCasts is only @@ -345,13 +345,13 @@ class CorrectionCandidateCallback { candidate.getCorrectionSpecifier() == TypoNNS; } - const IdentifierInfo *Typo; + IdentifierInfo *Typo; NestedNameSpecifier *TypoNNS; }; class DefaultFilterCCC final : public CorrectionCandidateCallback { public: - explicit DefaultFilterCCC(const IdentifierInfo *Typo = nullptr, + explicit DefaultFilterCCC(IdentifierInfo *Typo = nullptr, NestedNameSpecifier *TypoNNS = nullptr) : CorrectionCandidateCallback(Typo, TypoNNS) {} @@ -365,10 +365,6 @@ class DefaultFilterCCC final : public CorrectionCandidateCallback { template <class C> class DeclFilterCCC final : public CorrectionCandidateCallback { public: - explicit DeclFilterCCC(const IdentifierInfo *Typo = nullptr, - NestedNameSpecifier *TypoNNS = nullptr) - : CorrectionCandidateCallback(Typo, TypoNNS) {} - bool ValidateCandidate(const TypoCorrection &candidate) override { return candidate.getCorrectionDeclAs<C>(); } diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index f1f335118f37a4..49d0dd218d6830 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -9003,10 +9003,6 @@ class AttrImporter { public: AttrImporter(ASTImporter &I) : Importer(I), NImporter(I) {} - // Useful for accessing the imported attribute. - template <typename T> T *castAttrAs() { return cast<T>(ToAttr); } - template <typename T> const T *castAttrAs() const { return cast<T>(ToAttr); } - // Create an "importer" for an attribute parameter. // Result of the 'value()' of that object is to be passed to the function // 'importAttr', in the order that is expected by the attribute class. @@ -9214,15 +9210,6 @@ Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) { From->args_size()); break; } - case attr::CountedBy: { - AI.cloneAttr(FromAttr); - const auto *CBA = cast<CountedByAttr>(FromAttr); - Expected<SourceRange> SR = Import(CBA->getCountedByFieldLoc()).get(); - if (!SR) - return SR.takeError(); - AI.castAttrAs<CountedByAttr>()->setCountedByFieldLoc(SR.get()); - break; - } default: { // The default branch works for attributes that have no arguments to import. diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp index e4d7169752bc85..5e03f0223d311c 100644 --- a/clang/lib/AST/DeclBase.cpp +++ b/clang/lib/AST/DeclBase.cpp @@ -29,6 +29,7 @@ #include "clang/AST/Type.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LLVM.h" +#include "clang/Basic/LangOptions.h" #include "clang/Basic/Module.h" #include "clang/Basic/ObjCRuntime.h" #include "clang/Basic/PartialDiagnostic.h" @@ -410,79 +411,6 @@ bool Decl::isFileContextDecl() const { return DC && DC->isFileContext(); } -bool Decl::isFlexibleArrayMemberLike( - ASTContext &Ctx, const Decl *D, QualType Ty, - LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel, - bool IgnoreTemplateOrMacroSubstitution) { - // For compatibility with existing code, we treat arrays of length 0 or - // 1 as flexible array members. - const auto *CAT = Ctx.getAsConstantArrayType(Ty); - if (CAT) { - using FAMKind = LangOptions::StrictFlexArraysLevelKind; - - llvm::APInt Size = CAT->getSize(); - if (StrictFlexArraysLevel == FAMKind::IncompleteOnly) - return false; - - // GCC extension, only allowed to represent a FAM. - if (Size.isZero()) - return true; - - if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete && Size.uge(1)) - return false; - - if (StrictFlexArraysLevel == FAMKind::OneZeroOrIncomplete && Size.uge(2)) - return false; - } else if (!Ctx.getAsIncompleteArrayType(Ty)) { - return false; - } - - if (const auto *OID = dyn_cast_if_present<ObjCIvarDecl>(D)) - return OID->getNextIvar() == nullptr; - - const auto *FD = dyn_cast_if_present<FieldDecl>(D); - if (!FD) - return false; - - if (CAT) { - // GCC treats an array memeber of a union as an FAM if the size is one or - // zero. - llvm::APInt Size = CAT->getSize(); - if (FD->getParent()->isUnion() && (Size.isZero() || Size.isOne())) - return true; - } - - // Don't consider sizes resulting from macro expansions or template argument - // substitution to form C89 tail-padded arrays. - if (IgnoreTemplateOrMacroSubstitution) { - TypeSourceInfo *TInfo = FD->getTypeSourceInfo(); - while (TInfo) { - TypeLoc TL = TInfo->getTypeLoc(); - - // Look through typedefs. - if (TypedefTypeLoc TTL = TL.getAsAdjusted<TypedefTypeLoc>()) { - const TypedefNameDecl *TDL = TTL.getTypedefNameDecl(); - TInfo = TDL->getTypeSourceInfo(); - continue; - } - - if (auto CTL = TL.getAs<ConstantArrayTypeLoc>()) { - if (const Expr *SizeExpr = - dyn_cast_if_present<IntegerLiteral>(CTL.getSizeExpr()); - !SizeExpr || SizeExpr->getExprLoc().isMacroID()) - return false; - } - - break; - } - } - - // Test that the field is the last in the structure. - RecordDecl::field_iterator FI( - DeclContext::decl_iterator(const_cast<FieldDecl *>(FD))); - return ++FI == FD->getParent()->field_end(); -} - TranslationUnitDecl *Decl::getTranslationUnitDecl() { if (auto *TUD = dyn_cast<TranslationUnitDecl>(this)) return TUD; diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index b125fc676da841..a90f92d07f86d2 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -205,22 +205,85 @@ bool Expr::isKnownToHaveBooleanValue(bool Semantic) const { } bool Expr::isFlexibleArrayMemberLike( - ASTContext &Ctx, + ASTContext &Context, LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel, bool IgnoreTemplateOrMacroSubstitution) const { + + // For compatibility with existing code, we treat arrays of length 0 or + // 1 as flexible array members. + const auto *CAT = Context.getAsConstantArrayType(getType()); + if (CAT) { + llvm::APInt Size = CAT->getSize(); + + using FAMKind = LangOptions::StrictFlexArraysLevelKind; + + if (StrictFlexArraysLevel == FAMKind::IncompleteOnly) + return false; + + // GCC extension, only allowed to represent a FAM. + if (Size == 0) + return true; + + if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete && Size.uge(1)) + return false; + + if (StrictFlexArraysLevel == FAMKind::OneZeroOrIncomplete && Size.uge(2)) + return false; + } else if (!Context.getAsIncompleteArrayType(getType())) + return false; + const Expr *E = IgnoreParens(); - const Decl *D = nullptr; - if (const auto *ME = dyn_cast<MemberExpr>(E)) - D = ME->getMemberDecl(); - else if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) - D = DRE->getDecl(); + const NamedDecl *ND = nullptr; + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + ND = DRE->getDecl(); + else if (const auto *ME = dyn_cast<MemberExpr>(E)) + ND = ME->getMemberDecl(); else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E)) - D = IRE->getDecl(); + return IRE->getDecl()->getNextIvar() == nullptr; + + if (!ND) + return false; - return Decl::isFlexibleArrayMemberLike(Ctx, D, E->getType(), - StrictFlexArraysLevel, - IgnoreTemplateOrMacroSubstitution); + // A flexible array member must be the last member in the class. + // FIXME: If the base type of the member expr is not FD->getParent(), + // this should not be treated as a flexible array member access. + if (const auto *FD = dyn_cast<FieldDecl>(ND)) { + // GCC treats an array memeber of a union as an FAM if the size is one or + // zero. + if (CAT) { + llvm::APInt Size = CAT->getSize(); + if (FD->getParent()->isUnion() && (Size.isZero() || Size.isOne())) + return true; + } + + // Don't consider sizes resulting from macro expansions or template argument + // substitution to form C89 tail-padded arrays. + if (IgnoreTemplateOrMacroSubstitution) { + TypeSourceInfo *TInfo = FD->getTypeSourceInfo(); + while (TInfo) { + TypeLoc TL = TInfo->getTypeLoc(); + // Look through typedefs. + if (TypedefTypeLoc TTL = TL.getAsAdjusted<TypedefTypeLoc>()) { + const TypedefNameDecl *TDL = TTL.getTypedefNameDecl(); + TInfo = TDL->getTypeSourceInfo(); + continue; + } + if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) { + const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr()); + if (!SizeExpr || SizeExpr->getExprLoc().isMacroID()) + return false; + } + break; + } + } + + RecordDecl::field_iterator FI( + DeclContext::decl_iterator(const_cast<FieldDecl *>(FD))); + return ++FI == FD->getParent()->field_end(); + } + + return false; } const ValueDecl * diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp index 4eb1686f095062..a29304c81928cc 100644 --- a/clang/lib/CodeGen/CGBuiltin.cpp +++ b/clang/lib/CodeGen/CGBuiltin.cpp @@ -25,7 +25,6 @@ #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/OSLog.h" -#include "clang/AST/OperationKinds.h" #include "clang/Basic/TargetBuiltins.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/TargetOptions.h" @@ -819,165 +818,6 @@ CodeGenFunction::evaluateOrEmitBuiltinObjectSize(const Expr *E, unsigned Type, return ConstantInt::get(ResType, ObjectSize, /*isSigned=*/true); } -llvm::Value * -CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type, - llvm::IntegerType *ResType) { - // The code generated here calculates the size of a struct with a flexible - // array member that uses the counted_by attribute. There are two instances - // we handle: - // - // struct s { - // unsigned long flags; - // int count; - // int array[] __attribute__((counted_by(count))); - // } - // - // 1) bdos of the flexible array itself: - // - // __builtin_dynamic_object_size(p->array, 1) == - // p->count * sizeof(*p->... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/75857 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits