[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101236.340288.patch
Type: text/x-patch
Size: 3011 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210424/7c24c3aa/attachment.bin>