[ASTImporter] Fix import of objects with anonymous types

[martong]

Summary

Currently, anonymous types are merged into the same redecl chain even if they
are structurally inequivalent. This results that global objects are not
imported, if there are at least two global objects with different anonymous
types. This patch provides a fix.

[a_sidorin]

Hello Gabor,

The change looks reasonable to me. But could you please check if we can have some code unification with structural matching?

lib/AST/ASTImporter.cpp

2075

Is it possible to use the added code for the entire condition if (auto *FoundRecord = dyn_cast<RecordDecl>(Found)), replacing its body? Our structural matching already knows how to handle unnamed structures, and the upper code partially duplicates IsStructurallyEquivalent(StructuralEquivalenceContext &Context,RecordDecl *D1, RecordDecl *D2). Can we change structural matching to handle this stuff instead of doing it in ASTNodeImporter?

[martong]

Remove redundant code and use only StructurlaEquivalence

[martong]

lib/AST/ASTImporter.cpp

2075

Yes, this is a good point, updated the patch accordingly.

[balazske]

lib/AST/ASTImporter.cpp

2075

My idea was to remove this whole if (!SearchName) branch, but some restructuring of the next conditions may be needed. Setting of PrevDecl in the if branch does not look correct (if !SearchName is false it is never set, unlike in the old code).

[martong]

lib/AST/ASTImporter.cpp

2075

I tried what you mentioned Balazs.
It turned out, we can't just skip the whole if (!SearchName) branch. Because in the below case we would match the first unnamed union with the second, therefore we will break functionality (and some lit tests). The continue at line 2078 is very important in this edge case. (I agree that this code is very messy and would be good if we could refactor that to be cleaner, particularly if we could remove the SearchName variable would make it way better.)

// Matches
struct Unnamed {
  union {
    struct {
      int i;
    } S;
    struct {
      float i;
    } R;
  } U;
} x14;

The failing lit test and it's output:

/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:84:5: warning: type 'struct Unnamed::(anonymous at /home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:84:5)' has incompatible definitions in different translation units
    struct {
    ^
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:85:11: note: field 'i' has type 'int' here
      int i;
          ^
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:88:13: note: field 'i' has type 'float' here
      float i;
            ^

[a_sidorin]

Nice!

[martong]

PrevDecl is set after the !SearchName branch, as it had been done before.
This way original behaviour is kept as much as possible.

lib/AST/ASTImporter.cpp

for (auto *FoundDecl : FoundDecls) {
         if (auto *Template = dyn_cast<ClassTemplateDecl>(Found))
           Found = Template->getTemplatedDecl();
         else
           continue;
       }
 
       if (auto *FoundRecord = dyn_cast<RecordDecl>(Found)) {
         if (!SearchName) {
-          // If both unnamed structs/unions are in a record context, make sure
-          // they occur in the same location in the context records.
-          if (Optional<unsigned> Index1 =
-                  StructuralEquivalenceContext::findUntaggedStructOrUnionIndex(
-                      D)) {
-            if (Optional<unsigned> Index2 = StructuralEquivalenceContext::
-                    findUntaggedStructOrUnionIndex(FoundRecord)) {
-              if (*Index1 != *Index2)
-                continue;
-            }
-          }
-        }
+          if (!IsStructuralMatch(D, FoundRecord, false))
 
         PrevDecl = FoundRecord;
 
         if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
           if ((SearchName && !D->isCompleteDefinition())
               || (D->isCompleteDefinition() &&
                   D->isAnonymousStructOrUnion()
                     == FoundDef->isAnonymousStructOrUnion() &&

unittests/AST/ASTImporterTest.cpp

TEST_P(
   // We should have only one definition.
   EXPECT_EQ(1u, DeclCounterWithPredicate<ClassTemplateSpecializationDecl>(
                     [](const ClassTemplateSpecializationDecl *T) {
                       return T->isThisDeclarationADefinition();
                     })
                     .match(ToTU, classTemplateSpecializationDecl()));
 }
 
+TEST_P(ASTImporterTestBase, ObjectsWithUnnamedStructType) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct { int a; int b; } object0 = { 2, 3 };
+      struct { int x; int y; int z; } object1;
+      )",
+      Lang_CXX, "input0.cc");
+
+  auto getRecordDecl = [](VarDecl *VD) {
+    auto *ET = cast<ElaboratedType>(VD->getType().getTypePtr());
+    return cast<RecordType>(ET->getNamedType().getTypePtr())->getDecl();
+  };
+
+  auto *Obj0 =
+      FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("object0")));
+  auto *From0 = getRecordDecl(Obj0);
+  auto *Obj1 =
+      FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("object1")));
+  auto *From1 = getRecordDecl(Obj1);
+
+  auto *To0 = Import(From0, Lang_CXX);
+  auto *To1 = Import(From1, Lang_CXX);
+
+  EXPECT_TRUE(To0);
+  EXPECT_TRUE(To1);
+  EXPECT_NE(To0, To1);
+  EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
+}
+
 struct ImportFunctions : ASTImporterTestBase {};
 
 TEST_P(ImportFunctions,
        DefinitionShouldBeImportedAsDefintionWhenThereIsAPrototype) {
   Decl *FromTU = getTuDecl("void f(); void f() {}", Lang_CXX);
   auto Pattern = functionDecl(hasName("f"));
   FunctionDecl *FromD = // Definition
       LastDeclMatcher<FunctionDecl>().match(FromTU, Pattern);