jankratochvil created this revision.
jankratochvil added reviewers: teemperor, shafik, dblaikie.
jankratochvil added a project: clang.
Herald added a subscriber: martong.
Herald added a reviewer: a.sidorin.
jankratochvil requested review of this revision.
When LLDB needs to use `[[no_unique_address]]` (which is in fact always but
that is topic of the next patch) `FieldDecl::isZeroSize` was missing the
`NoUniqueAddressAttr` attribute calculating wrong field offsets and thus
failing on an assertion:
lldb: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:802: void (anonymous
namespace)::CGRecordLowering::insertPadding(): Assertion `Offset >= Size'
failed.
After importing just the `NoUniqueAddressAttr` attribute various testcases were
failing on:
clang/lib/AST/Decl.cpp:4163: bool clang::FieldDecl::isZeroSize(const
clang::ASTContext &) const: Assertion `isInvalidDecl() && "valid field has
incomplete type"' failed.
The LLDB testsuite exercises the patch fine. But the clang testcase I have
provided here succeeds even with no patch applied. I expect I would need to
import it X->Y->Z and not just X->Y but despite I tried some such code it was
still working even without my patch.
Is it OK without a testcase or is there some suggestion how to write the clang
testcase?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D101236
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,38 @@
ToD->getTemplateSpecializationKind());
}
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+ auto SrcCode =
+ R"(
+ struct A {};
+ struct B {
+ [[no_unique_address]] A a;
+ };
+ struct C : B {
+ char c;
+ } c;
+ )";
+ Decl *FromTU = getTuDecl(SrcCode, Lang_CXX20);
+
+ // It does not fail even without the patch!
+ auto *BFromD = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("B")));
+ ASSERT_TRUE(BFromD);
+ auto *BToD = Import(BFromD, Lang_CXX20);
+ EXPECT_TRUE(BToD);
+ auto *BTo2D = Import(BToD, Lang_CXX20);
+ EXPECT_TRUE(BTo2D);
+
+ // It does not fail even without the patch!
+ auto *CFromD = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("C")));
+ ASSERT_TRUE(CFromD);
+ auto *CToD = Import(CFromD, Lang_CXX20);
+ EXPECT_TRUE(CToD);
+ auto *CTo2D = Import(CToD, Lang_CXX20);
+ EXPECT_TRUE(CTo2D);
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, );
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
D->getInClassInitStyle()))
return ToField;
+ // FieldDecl::isZeroSize may need to check these.
+ if (auto FromAttr = D->getAttr<NoUniqueAddressAttr>()) {
+ if (auto ToAttrOrErr = Importer.Import(FromAttr))
+ ToField->addAttr(*ToAttrOrErr);
+ else
+ return ToAttrOrErr.takeError();
+ RecordType *FromRT =
+ const_cast<RecordType *>(D->getType()->getAs<RecordType>());
+ // Is this field a struct? FieldDecl::isZeroSize will need its definition.
+ if (FromRT) {
+ RecordDecl *FromRD = FromRT->getDecl();
+ RecordType *ToRT =
+ const_cast<RecordType *>(ToField->getType()->getAs<RecordType>());
+ assert(ToRT && "RecordType import has different type");
+ RecordDecl *ToRD = ToRT->getDecl();
+ if (FromRD->isCompleteDefinition() && !ToRD->isCompleteDefinition()) {
+ if (Error Err = ImportDefinition(FromRD, ToRD, IDK_Basic))
+ return std::move(Err);
+ }
+ }
+ }
+
ToField->setAccess(D->getAccess());
ToField->setLexicalDeclContext(LexicalDC);
if (ToInitializer)
@@ -8445,6 +8467,8 @@
// Make sure that ImportImpl registered the imported decl.
assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
+ // There may have been NoUniqueAddressAttr for FieldDecl::isZeroSize.
+ ToD->dropAttrs();
if (FromD->hasAttrs())
for (const Attr *FromAttr : FromD->getAttrs()) {
auto ToAttrOrErr = Import(FromAttr);
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,38 @@
ToD->getTemplateSpecializationKind());
}
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+ auto SrcCode =
+ R"(
+ struct A {};
+ struct B {
+ [[no_unique_address]] A a;
+ };
+ struct C : B {
+ char c;
+ } c;
+ )";
+ Decl *FromTU = getTuDecl(SrcCode, Lang_CXX20);
+
+ // It does not fail even without the patch!
+ auto *BFromD = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("B")));
+ ASSERT_TRUE(BFromD);
+ auto *BToD = Import(BFromD, Lang_CXX20);
+ EXPECT_TRUE(BToD);
+ auto *BTo2D = Import(BToD, Lang_CXX20);
+ EXPECT_TRUE(BTo2D);
+
+ // It does not fail even without the patch!
+ auto *CFromD = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("C")));
+ ASSERT_TRUE(CFromD);
+ auto *CToD = Import(CFromD, Lang_CXX20);
+ EXPECT_TRUE(CToD);
+ auto *CTo2D = Import(CToD, Lang_CXX20);
+ EXPECT_TRUE(CTo2D);
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, );
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
D->getInClassInitStyle()))
return ToField;
+ // FieldDecl::isZeroSize may need to check these.
+ if (auto FromAttr = D->getAttr<NoUniqueAddressAttr>()) {
+ if (auto ToAttrOrErr = Importer.Import(FromAttr))
+ ToField->addAttr(*ToAttrOrErr);
+ else
+ return ToAttrOrErr.takeError();
+ RecordType *FromRT =
+ const_cast<RecordType *>(D->getType()->getAs<RecordType>());
+ // Is this field a struct? FieldDecl::isZeroSize will need its definition.
+ if (FromRT) {
+ RecordDecl *FromRD = FromRT->getDecl();
+ RecordType *ToRT =
+ const_cast<RecordType *>(ToField->getType()->getAs<RecordType>());
+ assert(ToRT && "RecordType import has different type");
+ RecordDecl *ToRD = ToRT->getDecl();
+ if (FromRD->isCompleteDefinition() && !ToRD->isCompleteDefinition()) {
+ if (Error Err = ImportDefinition(FromRD, ToRD, IDK_Basic))
+ return std::move(Err);
+ }
+ }
+ }
+
ToField->setAccess(D->getAccess());
ToField->setLexicalDeclContext(LexicalDC);
if (ToInitializer)
@@ -8445,6 +8467,8 @@
// Make sure that ImportImpl registered the imported decl.
assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
+ // There may have been NoUniqueAddressAttr for FieldDecl::isZeroSize.
+ ToD->dropAttrs();
if (FromD->hasAttrs())
for (const Attr *FromAttr : FromD->getAttrs()) {
auto ToAttrOrErr = Import(FromAttr);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
