Author: Michael Buch Date: 2024-07-29T08:47:57+01:00 New Revision: 31769e4d0892312948812fbc2d0a56249ea72492
URL: https://github.com/llvm/llvm-project/commit/31769e4d0892312948812fbc2d0a56249ea72492 DIFF: https://github.com/llvm/llvm-project/commit/31769e4d0892312948812fbc2d0a56249ea72492.diff LOG: Revert "Reland [clang][ASTImport] Add support for import of empty records" (#100903) This reverts commit 88e5206f2c96a34e23a4d63f0a38afb2db044f0a. 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 https://github.com/llvm/llvm-project/issues/93069. There, `__compressed_pair`s 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. Added: Modified: clang/include/clang/AST/ASTImporter.h clang/include/clang/AST/DeclCXX.h clang/lib/AST/ASTImporter.cpp clang/unittests/AST/ASTImporterTest.cpp Removed: ################################################################################ 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"( _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
