[clang] 31769e4 - Revert "Reland [clang][ASTImport] Add support for import of empty records" (#100903)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 29 00:48:01 PDT 2024
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"(
More information about the cfe-commits
mailing list