Revert counted_by attribute feature #75857

bwendling · 2023-12-18T21:14:57Z

There are many issues that popped up with the counted_by feature. The
patch #73730 has grown too large and approval is blocking Linux testing.

Includes reverts of:
commit 769bc11 ("[Clang] Implement the 'counted_by' attribute (#68750)")
commit bc09ec6 ("[CodeGen] Revamp counted_by calculations (#70606)")
commit 1a09cfb ("[Clang] counted_by attr can apply only to C99 flexible array members (#72347)")
commit a76adfb ("[NFC][Clang] Refactor code to calculate flexible array member size (#72790)")
commit d8447c7 ("[Clang] Correct handling of negative and out-of-bounds indices (#71877)")
Partial commit b31cd07 ("[Clang] Regenerate test checks (NFC)")

Closes #73168
Closes #75173

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 769bc11 ("[Clang] Implement the 'counted_by' attribute (llvm#68750)") commit bc09ec6 ("[CodeGen] Revamp counted_by calculations (llvm#70606)") commit 1a09cfb ("[Clang] counted_by attr can apply only to C99 flexible array members (llvm#72347)") commit a76adfb ("[NFC][Clang] Refactor code to calculate flexible array member size (llvm#72790)") commit d8447c7 ("[Clang] Correct handling of negative and out-of-bounds indices (llvm#71877)") Partial commit b31cd07 ("[Clang] Regenerate test checks (NFC)")

llvmbot · 2023-12-18T21:15:26Z

@llvm/pr-subscribers-clang-codegen

Author: Bill Wendling (bwendling)

Changes

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 769bc11 ("[Clang] Implement the 'counted_by' attribute (#68750)") commit bc09ec6 ("[CodeGen] Revamp counted_by calculations (#70606)") commit 1a09cfb ("[Clang] counted_by attr can apply only to C99 flexible array members (#72347)") commit a76adfb ("[NFC][Clang] Refactor code to calculate flexible array member size (#72790)") commit d8447c7 ("[Clang] Correct handling of negative and out-of-bounds indices (#71877)") Partial commit b31cd07 ("[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 --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]

llvmbot · 2023-12-18T21:15:26Z

@llvm/pr-subscribers-clang

Author: Bill Wendling (bwendling)

Changes

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 769bc11 ("[Clang] Implement the 'counted_by' attribute (#68750)") commit bc09ec6 ("[CodeGen] Revamp counted_by calculations (#70606)") commit 1a09cfb ("[Clang] counted_by attr can apply only to C99 flexible array members (#72347)") commit a76adfb ("[NFC][Clang] Refactor code to calculate flexible array member size (#72790)") commit d8447c7 ("[Clang] Correct handling of negative and out-of-bounds indices (#71877)") Partial commit b31cd07 ("[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 --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]

nickdesaulniers · 2023-12-18T21:21:17Z

@nathanchance can you test this please to verify it's unbreaking the linux kernel builds?

nathanchance · 2023-12-18T21:30:11Z

@nathanchance can you test this please to verify it's unbreaking the linux kernel builds?

A quick initial test shows this resolves the two cases that I found in #73168. I can do a fuller set of builds if necessary but since this is just backing out of __attribute__((__counted_by__(...))) altogether, I don't expect there to be any other issues as a result of this change (other than just uncovering other unrelated breakages from not having consistent ToT LLVM builds for almost a month in CI).

bwendling requested review from rjmccall, kees, nickdesaulniers, nathanchance, rapidsna and efriedma-quic December 18, 2023 21:14

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 18, 2023

bwendling mentioned this pull request Dec 18, 2023

[Clang] Generate the GEP instead of adding AST nodes #73730

Closed

nickdesaulniers approved these changes Dec 18, 2023

View reviewed changes

rapidsna requested review from devincoughlin and hnrklssn December 18, 2023 22:04

rapidsna approved these changes Dec 18, 2023

View reviewed changes

bwendling merged commit cca4d6c into llvm:main Dec 18, 2023

bwendling deleted the counted-by-revert branch December 18, 2023 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Revert counted_by attribute feature #75857

Revert counted_by attribute feature #75857

bwendling commented Dec 18, 2023 •

edited

Loading

Uh oh!

llvmbot commented Dec 18, 2023

Uh oh!

llvmbot commented Dec 18, 2023

Uh oh!

nickdesaulniers commented Dec 18, 2023

Uh oh!

nathanchance commented Dec 18, 2023

Uh oh!

Uh oh!

Revert counted_by attribute feature #75857

Revert counted_by attribute feature #75857

Conversation

bwendling commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 18, 2023

Uh oh!

llvmbot commented Dec 18, 2023

Uh oh!

nickdesaulniers commented Dec 18, 2023

Uh oh!

nathanchance commented Dec 18, 2023

Uh oh!

Uh oh!

bwendling commented Dec 18, 2023 •

edited

Loading