Revert "Reland [clang][ASTImport] Add support for import of empty records"

[Michael137]

This reverts commit 88e5206. The original change went in a while ago (last year) in https://reviews.llvm.org/D145057. The specific reason I'm proposing a revert is that this is now causing exactly the issue that @balazske predicted in https://reviews.llvm.org/D145057#4164717:

Problematic case is if the attribute has pointer to a Decl or Type that is imported here in a state when the field is already created but not initialized. Another problem is that attributes are added a second time in Import(Decl *)

This now came up in the testing of LLDB support for #93069. There, __compressed_pairs are now replaced with fields that have an alignof(...) and [[no_unique_address]] attribute. In the specific failing case, we're importing following std::list method:

size_type& __sz() _NOEXCEPT { return __size_; }

During this process, we create a new __size_ FieldDecl (but don't initialize it yet). Then we go down the ImportAttrs codepath added in D145057. This imports the alignof expression which then references the uninitialized __size_ and we trigger an assertion.

Important to note, this codepath was added specifically to support [[no_unique_address]] in LLDB, and was supposed to land with https://reviews.llvm.org/D143347. But the LLDB side of that never landed, and the way we plan to support [[no_unique_address]] doesn't require things like the markEmpty method added here. So really, this is a dead codepath, which as pointed out in the original review isn't fully sound.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Michael Buch (Michael137)

Changes

This reverts commit 88e5206. The original change went in a while ago (last year) in https://reviews.llvm.org/D145057. The specific reason I'm proposing a revert is that this is now causing exactly the issue that @balazske predicted in https://reviews.llvm.org/D145057#4164717:
> Problematic case is if the attribute has pointer to a Decl or Type that is imported here in a state when the field is already created but not initialized. Another problem is that attributes are added a second time in Import(Decl *)

This now came up in the testing of LLDB support for #93069. There, __compressed_pairs are now replaced with fields that have an alignof(...) and [[no_unique_address]] attribute. In the specific case, we're importing the size_type& __sz() _NOEXCEPT { return __size_; } method of std::list. During this process, we create a new __size_ FieldDecl (but don't initialize it yet). Then we go down the ImportAttrs codepath added in D145057. This imports the alignof expression which then references the uninitialized __size_ and we trigger an assertion.

Important to note, this codepath was added specifically to support [[no_unique_address]] in LLDB, and was supposed to land with https://reviews.llvm.org/D143347. But the LLDB side of that never landed, and the way we plan to support [[no_unique_address]] doesn't require things like the markEmpty method added here. So really, this is a dead codepath, which as pointed out in the original review isn't fully sound.

---

Full diff: https://github.com/llvm/llvm-project/pull/100903.diff

4 Files Affected:

• (modified) clang/include/clang/AST/ASTImporter.h (-1)
• (modified) clang/include/clang/AST/DeclCXX.h (-4)
• (modified) clang/lib/AST/ASTImporter.cpp (+9-21)
• (modified) clang/unittests/AST/ASTImporterTest.cpp (-23)

diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h
index 4ffd913846575..f851decd0965c 100644
--- a/clang/include/clang/AST/ASTImporter.h
+++ b/clang/include/clang/AST/ASTImporter.h
@@ -258,7 +258,6 @@ class TypeSourceInfo;
     FoundDeclsTy findDeclsInToCtx(DeclContext *DC, DeclarationName Name);
 
     void AddToLookupTable(Decl *ToD);
-    llvm::Error ImportAttrs(Decl *ToD, Decl *FromD);
 
   protected:
     /// Can be overwritten by subclasses to implement their own import logic.
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index fb52ac804849d..3a110454f29ed 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1188,10 +1188,6 @@ class CXXRecordDecl : public RecordDecl {
   ///
   /// \note This does NOT include a check for union-ness.
   bool isEmpty() const { return data().Empty; }
-  /// Marks this record as empty. This is used by DWARFASTParserClang
-  /// when parsing records with empty fields having [[no_unique_address]]
-  /// attribute
-  void markEmpty() { data().Empty = true; }
 
   void setInitMethod(bool Val) { data().HasInitMethod = Val; }
   bool hasInitMethod() const { return data().HasInitMethod; }
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 08ef09d353afc..da1981d8dd05f 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -4179,12 +4179,6 @@ ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
                               D->getInClassInitStyle()))
     return ToField;
 
-  // We need [[no_unqiue_address]] attributes to be added to FieldDecl, before
-  // we add fields in CXXRecordDecl::addedMember, otherwise record will be
-  // marked as having non-zero size.
-  Err = Importer.ImportAttrs(ToField, D);
-  if (Err)
-    return std::move(Err);
   ToField->setAccess(D->getAccess());
   ToField->setLexicalDeclContext(LexicalDC);
   ToField->setImplicit(D->isImplicit());
@@ -9399,19 +9393,6 @@ TranslationUnitDecl *ASTImporter::GetFromTU(Decl *ToD) {
   return FromDPos->second->getTranslationUnitDecl();
 }
 
-Error ASTImporter::ImportAttrs(Decl *ToD, Decl *FromD) {
-  if (!FromD->hasAttrs() || ToD->hasAttrs())
-    return Error::success();
-  for (const Attr *FromAttr : FromD->getAttrs()) {
-    auto ToAttrOrErr = Import(FromAttr);
-    if (ToAttrOrErr)
-      ToD->addAttr(*ToAttrOrErr);
-    else
-      return ToAttrOrErr.takeError();
-  }
-  return Error::success();
-}
-
 Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   if (!FromD)
     return nullptr;
@@ -9545,8 +9526,15 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   }
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
-  if (auto Error = ImportAttrs(ToD, FromD))
-    return std::move(Error);
+
+  if (FromD->hasAttrs())
+    for (const Attr *FromAttr : FromD->getAttrs()) {
+      auto ToAttrOrErr = Import(FromAttr);
+      if (ToAttrOrErr)
+        ToD->addAttr(*ToAttrOrErr);
+      else
+        return ToAttrOrErr.takeError();
+    }
 
   // Notify subclasses.
   Imported(FromD, ToD);
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 9b12caa37cf79..57c5f79651824 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9376,29 +9376,6 @@ TEST_P(ASTImporterOptionSpecificTestBase, VaListCpp) {
       ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
 }
 
-TEST_P(ASTImporterOptionSpecificTestBase,
-       ImportDefinitionOfEmptyClassWithNoUniqueAddressField) {
-  Decl *FromTU = getTuDecl(
-      R"(
-      struct B {};
-      struct A { B b; };
-      )",
-      Lang_CXX20);
-
-  CXXRecordDecl *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
-      FromTU, cxxRecordDecl(hasName("A")));
-
-  for (auto *FD : FromD->fields())
-    FD->addAttr(clang::NoUniqueAddressAttr::Create(FromD->getASTContext(),
-                                                   clang::SourceRange()));
-  FromD->markEmpty();
-
-  CXXRecordDecl *ToD = Import(FromD, Lang_CXX20);
-  EXPECT_TRUE(ToD->isEmpty());
-  for (auto *FD : ToD->fields())
-    EXPECT_EQ(true, FD->hasAttr<NoUniqueAddressAttr>());
-}
-
 TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingTypedefToRecord) {
   const char *Code =
       R"(