[ASTImporter] Improved import of AlignedAttr.

[balazske]

Summary

It is not enough to clone the attributes at import.
They can contain reference to objects that should be imported.
This work is done now for AlignedAttr.

[shafik]

LGTM aside from the comments I made.

clang/lib/AST/ASTImporter.cpp

7962

This call to Create and the one below look identical can we please refactor to avoid code duplication.

clang/unittests/AST/ASTImporterTest.cpp

5891

If we have a test case that triggers this failure can we add the test as a know failing test and then we the FIXME is done the test should pass.

5910

What about getSemanticSpelling()?

[balazske]

• Do not call AlignedAttr::Create with same parameters at 2 places. I think the code duplication was near to each other and only 2 instances that is not a big problem, anyway now it is a bit better.
• Improved the comment in the test and added some new checks.

[shafik]

clang/lib/AST/ASTImporter.cpp

7962

How about something more like, maybe I am missing a detail but hopefully not:

bool IsAlignmentExpr=From->isAlignmentExpr();
auto ToEOrErr = [IsAlignmentExpr]() {
  if (IsAlignmentExpr)
    return Import(From->getAlignmentExpr());

  return  Import(From->getAlignmentType());
}();

if (!ToTOrErr)
  return ToTOrErr.takeError();

To = AlignedAttr::Create(ToContext, IsAlignmentExpr, *ToEOrErr, ToRange,
                                 From->getSyntax(),From->getSemanticSpelling());

[balazske]

clang/lib/AST/ASTImporter.cpp

7962

It is only possible by using a template. Importing of AlignmentExpr returns Expected<Expr *> and importing the AlignmentType returns Expected<TypeSourceInfo *>. The ToTOrErr and ToEOrErr are of different types. (The create function expects a void * that can be a pointer to Expr or TypeSourceInfo).

clang/lib/AST/ASTImporter.cpp

Expected<TypeSourceInfo *> ASTImporter::Import(TypeSourceInfo *FromTSI) {
   ExpectedSLoc BeginLocOrErr = Import(FromTSI->getTypeLoc().getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   return ToContext.getTrivialTypeSourceInfo(*TOrErr, *BeginLocOrErr);
 }
 
 Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) {
-  Attr *ToAttr = FromAttr->clone(ToContext);
-  if (auto ToRangeOrErr = Import(FromAttr->getRange()))
-    ToAttr->setRange(*ToRangeOrErr);
+  Attr *ToAttr = nullptr;
+  SourceRange ToRange;
+  if (Error Err = importInto(ToRange, FromAttr->getRange()))
+    return std::move(Err);
+
+  // FIXME: Is there some kind of AttrVisitor to use here?
+  switch (FromAttr->getKind()) {
+  case attr::Aligned: {
+    auto *From = cast<AlignedAttr>(FromAttr);
+    AlignedAttr *To;
+    auto CreateAlign = [&](bool IsAlignmentExpr, void *Alignment) {
+      return AlignedAttr::Create(ToContext, IsAlignmentExpr, Alignment, ToRange,
+                                 From->getSyntax(),
+                                 From->getSemanticSpelling());
+    };
+    if (From->isAlignmentExpr()) {
+      if (auto ToEOrErr = Import(From->getAlignmentExpr()))
+        To = CreateAlign(true, *ToEOrErr);
       else
-    return ToRangeOrErr.takeError();
+        return ToEOrErr.takeError();
+    } else {
+      if (auto ToTOrErr = Import(From->getAlignmentType()))
+        To = CreateAlign(false, *ToTOrErr);
+      else
+        return ToTOrErr.takeError();
+    }
+    To->setInherited(From->isInherited());
+    To->setPackExpansion(From->isPackExpansion());
+    To->setImplicit(From->isImplicit());
+    ToAttr = To;
+    break;
+  }
+  default:
+    // FIXME: 'clone' copies every member but some of them should be imported.
+    // Handle other Attrs that have parameters that should be imported.
+    ToAttr = FromAttr->clone(ToContext);
+    ToAttr->setRange(ToRange);
+    break;
+  }
+  assert(ToAttr && "Attribute should be created.");
   
   return ToAttr;
 }
 
 Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
   auto Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end())
     return Pos->second;
   else

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportSourceLocations, PreserveFileIDTreeStructure) {
   EXPECT_EQ(ToSM.getIncludeLoc(FileID2), FileStart);
 
   // Let the SourceManager check the order of the locations. The order should
   // be the order in which the declarations are imported.
   EXPECT_TRUE(ToSM.isBeforeInTranslationUnit(Location1, Location2));
   EXPECT_FALSE(ToSM.isBeforeInTranslationUnit(Location2, Location1));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportExprOfAlignmentAttr) {
+  // Test if import of these packed and aligned attributes does not trigger an
+  // error situation where source location from 'From' context is referenced in
+  // 'To' context through evaluation of the alignof attribute.
+  // This happens if the 'alignof(A)' expression is not imported correctly.
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct __attribute__((packed)) A { int __attribute__((aligned(8))) X; };
+      struct alignas(alignof(A)) S {};
+      )",
+      Lang_CXX11, "input.cc");
+  auto *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("S"), unless(isImplicit())));
+  ASSERT_TRUE(FromD);
+
+  auto *ToD = Import(FromD, Lang_CXX11);
+  ASSERT_TRUE(ToD);
+
+  auto *FromAttr = FromD->getAttr<AlignedAttr>();
+  auto *ToAttr = ToD->getAttr<AlignedAttr>();
+  EXPECT_EQ(FromAttr->isInherited(), ToAttr->isInherited());
+  EXPECT_EQ(FromAttr->isPackExpansion(), ToAttr->isPackExpansion());
+  EXPECT_EQ(FromAttr->isImplicit(), ToAttr->isImplicit());
+  EXPECT_EQ(FromAttr->getSyntax(), ToAttr->getSyntax());
+  EXPECT_EQ(FromAttr->getSemanticSpelling(), ToAttr->getSemanticSpelling());
+  EXPECT_TRUE(ToAttr->getAlignmentExpr());
+
+  auto *ToA = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToD->getTranslationUnitDecl(),
+      cxxRecordDecl(hasName("A"), unless(isImplicit())));
+  // Ensure that 'struct A' was imported (through reference from attribute of
+  // 'S').
+  EXPECT_TRUE(ToA);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );