Revert "[Modules] Delay deserialization of preferred_name attribute at r… (#122726)"

ilya-biryukov · ilya-biryukov · commit f63e8ed16ef1 · 2025-01-22T18:17:37.000+01:00
This reverts commit c3ba6f3. We are seeing performance regressions of up to 40% on some compilations with this patch, we will investigate and reland after fixing performance issues.
diff --git a/clang/include/clang/AST/Attr.h b/clang/include/clang/AST/Attr.h
@@ -60,8 +60,6 @@ class Attr : public AttributeCommonInfo {
   unsigned IsLateParsed : 1;
   LLVM_PREFERRED_TYPE(bool)
   unsigned InheritEvenIfAlreadyPresent : 1;
-  LLVM_PREFERRED_TYPE(bool)
-  unsigned DeferDeserialization : 1;
 
   void *operator new(size_t bytes) noexcept {
     llvm_unreachable("Attrs cannot be allocated with regular 'new'.");
@@ -82,11 +80,10 @@ class Attr : public AttributeCommonInfo {
 
 protected:
   Attr(ASTContext &Context, const AttributeCommonInfo &CommonInfo,
-       attr::Kind AK, bool IsLateParsed, bool DeferDeserialization = false)
+       attr::Kind AK, bool IsLateParsed)
       : AttributeCommonInfo(CommonInfo), AttrKind(AK), Inherited(false),
         IsPackExpansion(false), Implicit(false), IsLateParsed(IsLateParsed),
-        InheritEvenIfAlreadyPresent(false),
-        DeferDeserialization(DeferDeserialization) {}
+        InheritEvenIfAlreadyPresent(false) {}
 
 public:
   attr::Kind getKind() const { return static_cast<attr::Kind>(AttrKind); }
@@ -108,8 +105,6 @@ class Attr : public AttributeCommonInfo {
   void setPackExpansion(bool PE) { IsPackExpansion = PE; }
   bool isPackExpansion() const { return IsPackExpansion; }
 
-  bool shouldDeferDeserialization() const { return DeferDeserialization; }
-
   // Clone this attribute.
   Attr *clone(ASTContext &C) const;
 
@@ -151,9 +146,8 @@ class InheritableAttr : public Attr {
 protected:
   InheritableAttr(ASTContext &Context, const AttributeCommonInfo &CommonInfo,
                   attr::Kind AK, bool IsLateParsed,
-                  bool InheritEvenIfAlreadyPresent,
-                  bool DeferDeserialization = false)
-      : Attr(Context, CommonInfo, AK, IsLateParsed, DeferDeserialization) {
+                  bool InheritEvenIfAlreadyPresent)
+      : Attr(Context, CommonInfo, AK, IsLateParsed) {
     this->InheritEvenIfAlreadyPresent = InheritEvenIfAlreadyPresent;
   }
 
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
@@ -713,12 +713,6 @@ class Attr {
   // attribute may be documented under multiple categories, more than one
   // Documentation entry may be listed.
   list<Documentation> Documentation;
-  // Set to true if deserialization of this attribute must be deferred until 
-  // the parent Decl is fully deserialized (during header module file
-  // deserialization). E.g., this is the case for the preferred_name attribute,
-  // since its type deserialization depends on its target Decl type.
-  // (See https://github.com/llvm/llvm-project/issues/56490 for details).
-  bit DeferDeserialization = 0;
 }
 
 /// Used to define a set of mutually exclusive attributes.
@@ -3260,11 +3254,6 @@ def PreferredName : InheritableAttr {
   let InheritEvenIfAlreadyPresent = 1;
   let MeaningfulToClassTemplateDefinition = 1;
   let TemplateDependent = 1;
-  // Type of this attribute depends on the target Decl type.
-  // Therefore, its deserialization must be deferred until
-  // deserialization of the target Decl is complete
-  // (for header modules).
-  let DeferDeserialization = 1;
 }
 
 def PreserveMost : DeclOrTypeAttr {
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
@@ -1236,24 +1236,6 @@ class ASTReader
   /// been completed.
   std::deque<PendingDeclContextInfo> PendingDeclContextInfos;
 
-  /// Deserialization of some attributes must be deferred since they refer
-  /// to themselves in their type (e.g., preferred_name attribute refers to the
-  /// typedef that refers back to the template specialization of the template
-  /// that the attribute is attached to).
-  /// More attributes that store TypeSourceInfo might be potentially affected,
-  /// see https://github.com/llvm/llvm-project/issues/56490 for details.
-  struct DeferredAttribute {
-    // Index of the deferred attribute in the Record of the TargetedDecl.
-    uint64_t RecordIdx;
-    // Decl to attach a deferred attribute to.
-    Decl *TargetedDecl;
-  };
-
-  /// The collection of Decls that have been loaded but some of their attributes
-  /// have been deferred, paired with the index inside the record pointing
-  /// at the skipped attribute.
-  SmallVector<DeferredAttribute> PendingDeferredAttributes;
-
   template <typename DeclTy>
   using DuplicateObjCDecls = std::pair<DeclTy *, DeclTy *>;
 
@@ -1606,7 +1588,6 @@ class ASTReader
   void loadPendingDeclChain(Decl *D, uint64_t LocalOffset);
   void loadObjCCategories(GlobalDeclID ID, ObjCInterfaceDecl *D,
                           unsigned PreviousGeneration = 0);
-  void loadDeferredAttribute(const DeferredAttribute &DA);
 
   RecordLocation getLocalBitOffset(uint64_t GlobalOffset);
   uint64_t getGlobalBitOffset(ModuleFile &M, uint64_t LocalOffset);
diff --git a/clang/include/clang/Serialization/ASTRecordReader.h b/clang/include/clang/Serialization/ASTRecordReader.h
@@ -83,12 +83,6 @@ class ASTRecordReader
   /// Returns the current value in this record, without advancing.
   uint64_t peekInt() { return Record[Idx]; }
 
-  /// Returns the next N values in this record, without advancing.
-  uint64_t peekInts(unsigned N) { return Record[Idx + N]; }
-
-  /// Skips the current value.
-  void skipInt() { Idx += 1; }
-
   /// Skips the specified number of values.
   void skipInts(unsigned N) { Idx += N; }
 
@@ -341,12 +335,7 @@ class ASTRecordReader
   Attr *readAttr();
 
   /// Reads attributes from the current stream position, advancing Idx.
-  /// For some attributes (where type depends on itself recursively), defer
-  /// reading the attribute until the type has been read.
-  void readAttributes(AttrVec &Attrs, Decl *D = nullptr);
-
-  /// Reads one attribute from the current stream position, advancing Idx.
-  Attr *readOrDeferAttrFor(Decl *D);
+  void readAttributes(AttrVec &Attrs);
 
   /// Read an BTFTypeTagAttr object.
   BTFTypeTagAttr *readBTFTypeTagAttr() {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
@@ -10239,11 +10239,6 @@ void ASTReader::finishPendingActions() {
     }
     PendingDeducedVarTypes.clear();
 
-    // Load the delayed preferred name attributes.
-    for (unsigned I = 0; I != PendingDeferredAttributes.size(); ++I)
-      loadDeferredAttribute(PendingDeferredAttributes[I]);
-    PendingDeferredAttributes.clear();
-
     // For each decl chain that we wanted to complete while deserializing, mark
     // it as "still needs to be completed".
     for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) {
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -613,7 +613,7 @@ void ASTDeclReader::VisitDecl(Decl *D) {
 
   if (HasAttrs) {
     AttrVec Attrs;
-    Record.readAttributes(Attrs, D);
+    Record.readAttributes(Attrs);
     // Avoid calling setAttrs() directly because it uses Decl::getASTContext()
     // internally which is unsafe during derialization.
     D->setAttrsImpl(Attrs, Reader.getContext());
@@ -3098,8 +3098,6 @@ class AttrReader {
     return Reader.readInt();
   }
 
-  uint64_t peekInts(unsigned N) { return Reader.peekInts(N); }
-
   bool readBool() { return Reader.readBool(); }
 
   SourceRange readSourceRange() {
@@ -3130,29 +3128,18 @@ class AttrReader {
     return Reader.readVersionTuple();
   }
 
-  void skipInt() { Reader.skipInts(1); }
-
-  void skipInts(unsigned N) { Reader.skipInts(N); }
-
-  unsigned getCurrentIdx() { return Reader.getIdx(); }
-
   OMPTraitInfo *readOMPTraitInfo() { return Reader.readOMPTraitInfo(); }
 
   template <typename T> T *readDeclAs() { return Reader.readDeclAs<T>(); }
 };
 }
 
-/// Reads one attribute from the current stream position, advancing Idx.
 Attr *ASTRecordReader::readAttr() {
   AttrReader Record(*this);
   auto V = Record.readInt();
   if (!V)
     return nullptr;
 
-  // Read and ignore the skip count, since attribute deserialization is not
-  // deferred on this pass.
-  Record.skipInt();
-
   Attr *New = nullptr;
   // Kind is stored as a 1-based integer because 0 is used to indicate a null
   // Attr pointer.
@@ -3182,28 +3169,13 @@ Attr *ASTRecordReader::readAttr() {
   return New;
 }
 
-/// Reads attributes from the current stream position, advancing Idx.
-/// For some attributes (where type depends on itself recursively), defer
-/// reading the attribute until the type has been read.
-void ASTRecordReader::readAttributes(AttrVec &Attrs, Decl *D) {
+/// Reads attributes from the current stream position.
+void ASTRecordReader::readAttributes(AttrVec &Attrs) {
   for (unsigned I = 0, E = readInt(); I != E; ++I)
-    if (auto *A = readOrDeferAttrFor(D))
+    if (auto *A = readAttr())
       Attrs.push_back(A);
 }
 
-/// Reads one attribute from the current stream position, advancing Idx.
-/// For some attributes (where type depends on itself recursively), defer
-/// reading the attribute until the type has been read.
-Attr *ASTRecordReader::readOrDeferAttrFor(Decl *D) {
-  AttrReader Record(*this);
-  unsigned SkipCount = Record.peekInts(1);
-  if (!SkipCount)
-    return readAttr();
-  Reader->PendingDeferredAttributes.push_back({Record.getCurrentIdx(), D});
-  Record.skipInts(SkipCount);
-  return nullptr;
-}
-
 //===----------------------------------------------------------------------===//
 // ASTReader Implementation
 //===----------------------------------------------------------------------===//
@@ -4512,49 +4484,6 @@ void ASTReader::loadPendingDeclChain(Decl *FirstLocal, uint64_t LocalOffset) {
   ASTDeclReader::attachLatestDecl(CanonDecl, MostRecent);
 }
 
-void ASTReader::loadDeferredAttribute(const DeferredAttribute &DA) {
-  Decl *D = DA.TargetedDecl;
-  ModuleFile *M = getOwningModuleFile(D);
-
-  unsigned LocalDeclIndex = D->getGlobalID().getLocalDeclIndex();
-  const DeclOffset &DOffs = M->DeclOffsets[LocalDeclIndex];
-  RecordLocation Loc(M, DOffs.getBitOffset(M->DeclsBlockStartOffset));
-
-  llvm::BitstreamCursor &Cursor = Loc.F->DeclsCursor;
-  SavedStreamPosition SavedPosition(Cursor);
-  if (llvm::Error Err = Cursor.JumpToBit(Loc.Offset)) {
-    Error(std::move(Err));
-  }
-
-  Expected<unsigned> MaybeCode = Cursor.ReadCode();
-  if (!MaybeCode) {
-    llvm::report_fatal_error(
-        Twine("ASTReader::loadPreferredNameAttribute failed reading code: ") +
-        toString(MaybeCode.takeError()));
-  }
-  unsigned Code = MaybeCode.get();
-
-  ASTRecordReader Record(*this, *Loc.F);
-  Expected<unsigned> MaybeRecCode = Record.readRecord(Cursor, Code);
-  if (!MaybeRecCode) {
-    llvm::report_fatal_error(
-        Twine(
-            "ASTReader::loadPreferredNameAttribute failed reading rec code: ") +
-        toString(MaybeCode.takeError()));
-  }
-  unsigned RecCode = MaybeRecCode.get();
-  if (RecCode < DECL_TYPEDEF || RecCode > DECL_LAST) {
-    llvm::report_fatal_error(
-        Twine("ASTReader::loadPreferredNameAttribute failed reading rec code: "
-              "expected valid DeclCode") +
-        toString(MaybeCode.takeError()));
-  }
-
-  Record.skipInts(DA.RecordIdx);
-  Attr *A = Record.readAttr();
-  getContext().getDeclAttrs(D).push_back(A);
-}
-
 namespace {
 
   /// Given an ObjC interface, goes through the modules and links to the
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
@@ -37,7 +37,6 @@
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeLocVisitor.h"
-#include "clang/Basic/AttrKinds.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileEntry.h"
@@ -5156,14 +5155,15 @@ void ASTWriter::WriteModuleFileExtension(Sema &SemaRef,
 
 void ASTRecordWriter::AddAttr(const Attr *A) {
   auto &Record = *this;
-  if (!A)
+  // FIXME: Clang can't handle the serialization/deserialization of
+  // preferred_name properly now. See
+  // https://github.com/llvm/llvm-project/issues/56490 for example.
+  if (!A || (isa<PreferredNameAttr>(A) &&
+             Writer->isWritingStdCXXNamedModules()))
     return Record.push_back(0);
 
   Record.push_back(A->getKind() + 1); // FIXME: stable encoding, target attrs
 
-  auto SkipIdx = Record.size();
-  // Add placeholder for the size of deferred attribute.
-  Record.push_back(0);
   Record.AddIdentifierRef(A->getAttrName());
   Record.AddIdentifierRef(A->getScopeName());
   Record.AddSourceRange(A->getRange());
@@ -5174,12 +5174,6 @@ void ASTRecordWriter::AddAttr(const Attr *A) {
   Record.push_back(A->isRegularKeywordAttribute());
 
 #include "clang/Serialization/AttrPCHWrite.inc"
-
-  if (A->shouldDeferDeserialization()) {
-    // Record the actual size of deferred attribute (+ 1 to count the attribute
-    // kind).
-    Record[SkipIdx] = Record.size() - SkipIdx + 1;
-  }
 }
 
 /// Emit the list of attributes to the specified record.
diff --git a/clang/test/Modules/preferred_name.cppm b/clang/test/Modules/preferred_name.cppm
@@ -53,16 +53,10 @@ import A;
 export using ::foo_templ;
 
 //--- Use1.cpp
-// expected-no-diagnostics
-import A;
-#include "foo.h"
+import A;         // expected-warning@foo.h:8 {{attribute declaration must precede definition}}
+#include "foo.h"  // expected-note@foo.h:9 {{previous definition is here}}
+
 //--- Use2.cpp
 // expected-no-diagnostics
 #include "foo.h"
 import A;
-
-//--- Use3.cpp
-#include "foo.h"
-import A;
-foo test;
-int size = test.size(); // expected-error {{no member named 'size' in 'foo'}}
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3043,10 +3043,6 @@ static void emitAttributes(const RecordKeeper &Records, raw_ostream &OS,
            << (R.getValueAsBit("InheritEvenIfAlreadyPresent") ? "true"
                                                               : "false");
       }
-      if (R.getValueAsBit("DeferDeserialization")) {
-        OS << ", "
-           << "/*DeferDeserialization=*/true";
-      }
       OS << ")\n";
 
       for (auto const &ai : Args) {
