[clang][ASTImport] Add support for import of empty records

[kpdev42]

Summary

Patch represents the clang part of changes in D143347

[balazske]

clang/lib/AST/ASTImporter.cpp

3900

I am not familiar with this use case, is there a path where the attributes are read from a FieldDecl before return from VisitFieldDecl? Probably ImportImpl is overridden? Importing the attributes here should work but not totally sure if it does not cause problems. 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 *). Can it work if the ImportAttrs is made a protected function and called from (overridden) ImportImpl (still there can be a second import in Import(Decl *)?

clang/unittests/AST/ASTImporterTest.cpp

8161

8164

8165

Does this test fail without the changes applied? And does it not fail after (is the "Empty" value copied at import)?

[kpdev42]

clang/lib/AST/ASTImporter.cpp

3900

The problem is that field attributes are required when we are adding a copy of a field to a structure in VisitFieldDecl. Attributes themselves are read in CXXRecordDecl::addedMember (see the call to isZeroSize). Adding them after ImportImpl is called is too late: record is already marked as non-empty.

[kpdev42]

clang/unittests/AST/ASTImporterTest.cpp

8165

Yes the tests will fail due to above mentioned problem with field attributes

[balazske]

clang/lib/AST/ASTImporter.cpp

3900

I understand now. But a comment could be added here to explain why the attribute import is needed here.

8991

Should use braces in this case (https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements).

[balazske]

clang/include/clang/AST/DeclCXX.h

1171

This change looks not related.

[kpdev42]

Address review comments

[kpdev42]

clang/include/clang/AST/DeclCXX.h

1171

The markEmpty is still needed and is used in corresponding unit tests. It's gonna be used in DWARFASTParserClang to mark records with empty fields as having zero size. Then by copying attributes in ASTImporter we prevent this empty mark to be overwritten

[balazske]

There are some unresolved comments in the test (cast is not needed at Import and EXPECT_TRUE can be used) but otherwise LGTM.
(But I have a feeling that problems may happen with attributes like GuardedByAttr if these are imported before the field is added to the record. Probably it is better to only import the NoUniqueAddressAttr in VisitFieldDecl.)

clang/include/clang/AST/ASTImporter.h

private:
     /// Declaration (from, to) pairs that are known not to be equivalent
     /// (which we have already complained about).
     NonEquivalentDeclSet NonEquivalentDecls;
 
     using FoundDeclsTy = SmallVector<NamedDecl *, 2>;
     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.
     /// The overwritten method should call this method if it didn't import the
     /// decl on its own.
     virtual Expected<Decl *> ImportImpl(Decl *From);
 
     /// Used only in unittests to verify the behaviour of the error handling.

clang/include/clang/AST/DeclCXX.h

public:
   ///
   /// The CXXRecordDecl is a class type, but not a union type,
   /// with no non-static data members other than bit-fields of length 0,
   /// no virtual member functions, no virtual base classes,
   /// and no base class B for which is_empty<B>::value is false.
   ///
   /// \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; }
 
   bool hasPrivateFields() const {
     return data().HasPrivateFields;
   }
 

clang/lib/AST/ASTImporter.cpp

ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
 
   FieldDecl *ToField;
   if (GetImportedOrCreateDecl(ToField, D, Importer.getToContext(), DC,
                               ToInnerLocStart, Loc, Name.getAsIdentifierInfo(),
                               ToType, ToTInfo, ToBitWidth, D->isMutable(),
                               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);
   if (ToInitializer)
     ToField->setInClassInitializer(ToInitializer);
   ToField->setImplicit(D->isImplicit());
   if (ToCapturedVLAType)
     ToField->setCapturedVLAType(cast<VariableArrayType>(ToCapturedVLAType));
   LexicalDC->addDeclInternal(ToField);

 
 TranslationUnitDecl *ASTImporter::GetFromTU(Decl *ToD) {
   auto FromDPos = ImportedFromDecls.find(ToD);
   if (FromDPos == ImportedFromDecls.end())
     return nullptr;
   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;
 
   // Push FromD to the stack, and remove that when we return.
   ImportPath.push(FromD);
   auto ImportPathBuilder =
       llvm::make_scope_exit([this]() { ImportPath.pop(); });
Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   // ASTImporter object) and with an error.
   if (auto Error = SharedState->getImportDeclErrorIfAny(ToD)) {
     setImportDeclError(FromD, *Error);
     return make_error<ASTImportError>(*Error);
   }
 
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
-
-  if (FromD->hasAttrs())
-    for (const Attr *FromAttr : FromD->getAttrs()) {
-      auto ToAttrOrErr = Import(FromAttr);
-      if (ToAttrOrErr)
-        ToD->addAttr(*ToAttrOrErr);
-      else
-        return ToAttrOrErr.takeError();
-    }
+  if (auto Error = ImportAttrs(ToD, FromD))
+    return std::move(Error);
 
   // Notify subclasses.
   Imported(FromD, ToD);
 
   updateFlags(FromD, ToD);
   SavedImportPaths.erase(FromD);
   return ToDOrErr;
 }

clang/unittests/AST/ASTImporterTest.cpp

protected:
   }
 
   void checkInjType(const CXXRecordDecl *D) {
     // The whole redecl chain should have the same InjectedClassNameType
     // instance. The injected record declaration is a separate chain, this
     // should contain the same type too.
     const Type *Ty = nullptr;
     for (const Decl *ReD : D->redecls()) {
       const auto *ReRD = cast<CXXRecordDecl>(ReD);
       EXPECT_TRUE(ReRD->getTypeForDecl());
       EXPECT_TRUE(!Ty || Ty == ReRD->getTypeForDecl());
       Ty = ReRD->getTypeForDecl();
     }
     ASSERT_TRUE(Ty);
     const auto *InjTy = Ty->castAs<InjectedClassNameType>();
     EXPECT_TRUE(InjTy);
     if (CXXRecordDecl *Def = D->getDefinition()) {
       const CXXRecordDecl *InjRD = findInjected(Def);
       EXPECT_TRUE(InjRD);
       EXPECT_EQ(InjRD->getTypeForDecl(), InjTy);
     }
TEST_P(ASTImporterOptionSpecificTestBase, VaListCpp) {
   auto *ToBuiltinVaList = FirstDeclMatcher<TypedefDecl>().match(
       ToAST->getASTContext().getTranslationUnitDecl(),
       typedefDecl(hasName("__builtin_va_list")));
 
   ASSERT_TRUE(ToAST->getASTContext().hasSameType(
       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 = cast<CXXRecordDecl>(Import(FromD, Lang_CXX20));
+  EXPECT_EQ(true, ToD->isEmpty());
+  for (auto *FD : ToD->fields())
+    EXPECT_EQ(true, FD->hasAttr<NoUniqueAddressAttr>());
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);