[ASTImporter][Structural Eq] Check for isBeingDefined

[martong]

Summary

If one definition is currently being defined, we do not compare for
equality and we assume that the decls are equal.

[a_sidorin]

Hi Gabor,
I wonder if it is possible to get into situation where non-equivalent decls are marked equivalent with this patch? If yes, we can create a mapping between decls being imported and original decls as an alternative solution. However, I cannot find any counterexample.

lib/AST/ASTStructuralEquivalence.cpp

1037 ↗

(On Diff #171076)

Is it worth it to assert if only one Decl should be in isBeingDefined() state at time?

unittests/AST/ASTImporterTest.cpp

3729 ↗

(On Diff #171076)

Looks like this test is from another patch (D53693)?

[martong]

I wonder if it is possible to get into situation where non-equivalent decls are marked equivalent with this patch? If yes, we can create a mapping between decls being imported and original decls as an alternative solution. However, I cannot find any counterexample.

I don't think so.

This change is the natural extension what we already do (in line 1021 and 1022) with getDefinition().
getDefinition() works just as we would expect with a simple RecordDecl: getDefinition() returns a non-nullptr if isBeingDefined() is true.
However, getDefinition() may return a non-nullptr if D is a CXXRecordDecl even if D is being defined.

CXXRecordDecl *getDefinition() const {
  // We only need an update if we don't already know which
  // declaration is the definition.
  auto *DD = DefinitionData ? DefinitionData : dataPtr();
  return DD ? DD->Definition : nullptr;
}

This all depends on whether DefinitionData is set. And we do set that during ImportDefinition(RecordDecl *,).
And then we start importing methods and fields of the CXXRecordDecl via ImportDeclContext before the call to completeDefinition() which sets IsBeingDefined.
During those imports, the getDefinition() of a CXXRecordDecl will return with a non-nullptr value and we would go on checking the fields, but we are in the middle of importing the fields (or methods).

lib/AST/ASTStructuralEquivalence.cpp

1037 ↗

(On Diff #171076)

Strictly speaking D1 will always come from the "From" context and such it should report true for isBeingDefined. But the structural equivalence logic should be independent from the import logic ideally, so I would not add that assert.

unittests/AST/ASTImporterTest.cpp

3729 ↗

(On Diff #171076)

Yes, exactly, good catch, I just removed it.

[martong]

• Remove unrelated test

[a_sidorin]

Thank you for the explanation!

[shafik]

I reverted this commit since it caused a regression in the lldb test suite, specifically TestDataFormatterLibcxxVector.py.

See the logs here http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12003/

Please monitor the lldb built bots when modifying ASTImporter.

[martong]

Shafik,

Sorry for the inconvenience.
Seems like the libcxx tests of lldb are automatically skipped on Linux, so it slipped through my test execution.
Also, I did not receive any email from this build server about the build failure. Nevertheless, I'll monitor the buildbots with any next ASTImporter commit.
I think the fix for the assertion is easy to have, but it seems very difficult to test without having libcxx/MacOs at hand. Is there a way to execute libcxx tests from Linux?

[shafik]

@martong Hello, if I look at the specific test that was failing TestDataFormatterLibcxxVector.py it only has the following decorator @add_test_categories(["libc++"]) so that should not prevent it from running on Linux if you are also building libc++. If you do a local cmake build and you build libc++ as well does it still skip the test?

[martong]

@shafik,

I could run the libcxx tests on my Linux machine (Ubuntu 16.04) by installing libcxx: sudo apt-get install libc++-dev.
From the logs of your build server seems like the crash happened with a _gmodules test.
Unfortunately, the gmodules tests are still skipped on Linux:

Collected 8 tests

1: test_ref_and_ptr_dsym (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... skipped 'test case does not fall in any category of interest for this run'
2: test_ref_and_ptr_dwarf (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... ok
3: test_ref_and_ptr_dwo (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... ok
4: test_ref_and_ptr_gmodules (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... skipped 'test case does not fall in any category of interest for this run'
5: test_with_run_command_dsym (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... skipped 'test case does not fall in any category of interest for this run'
6: test_with_run_command_dwarf (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... ok
7: test_with_run_command_dwo (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... ok
8: test_with_run_command_gmodules (TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... skipped 'test case does not fall in any category of interest for this run'

----------------------------------------------------------------------
Ran 8 tests in 1.260s

This is aligned with the content of test_categories.py:

def is_supported_on_platform(category, platform, compiler_path):
    if category == "dwo":
        # -gsplit-dwarf is not implemented by clang on Windows.
        return platform in ["linux", "freebsd"]
    elif category == "dsym":
        return platform in ["darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]
    elif category == "gmodules":
        # First, check to see if the platform can even support gmodules.
        if platform not in ["freebsd", "darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]:
            return False
        return gmodules.is_compiler_clang_with_gmodules(compiler_path)
    return True

So apparently I cannot run gmodules tests on Linux.

My next step is trying to reproduce the issue on a real MacOS laptop.

[martong]

• Get data of CXXRecordDecl only if set

[martong]

I have reproduced the issue on MacOS, fixed it and updated the patch accordingly.
Soon I will commit and will monitor if any build bots are being broken.

cfe/trunk/lib/AST/ASTImporter.cpp

Error ASTNodeImporter::ImportDefinition(
   }
 
   To->startDefinition();
 
   if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
     return Err;
 
   // Add base classes.
-  if (auto *ToCXX = dyn_cast<CXXRecordDecl>(To)) {
-    auto *FromCXX = cast<CXXRecordDecl>(From);
+  auto *ToCXX = dyn_cast<CXXRecordDecl>(To);
+  auto *FromCXX = dyn_cast<CXXRecordDecl>(From);
+  if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
 
     struct CXXRecordDecl::DefinitionData &ToData = ToCXX->data();
     struct CXXRecordDecl::DefinitionData &FromData = FromCXX->data();
     ToData.UserDeclaredConstructor = FromData.UserDeclaredConstructor;
     ToData.UserDeclaredSpecialMembers = FromData.UserDeclaredSpecialMembers;
     ToData.Aggregate = FromData.Aggregate;
     ToData.PlainOldData = FromData.PlainOldData;
     ToData.Empty = FromData.Empty;

cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

for (unsigned I = 0, N = Spec1->getTemplateArgs().size(); I != N; ++I)
         return false;
   }
   // If one is a class template specialization and the other is not, these
   // structures are different.
   else if (Spec1 || Spec2)
     return false;
 
   // Compare the definitions of these two records. If either or both are
-  // incomplete, we assume that they are equivalent.
+  // incomplete (i.e. it is a forward decl), we assume that they are
+  // equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
     return true;
 
   // If any of the records has external storage and we do a minimal check (or
   // AST import) we assume they are equivalent. (If we didn't have this
   // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
   // another AST import which in turn would call the structural equivalency
   // check again and finally we'd have an improper result.)
   if (Context.EqKind == StructuralEquivalenceKind::Minimal)
     if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
       return true;
 
+  // If one definition is currently being defined, we do not compare for
+  // equality and we assume that the decls are equal.
+  if (D1->isBeingDefined() || D2->isBeingDefined())
+    return true;
+
   if (auto *D1CXX = dyn_cast<CXXRecordDecl>(D1)) {
     if (auto *D2CXX = dyn_cast<CXXRecordDecl>(D2)) {
       if (D1CXX->hasExternalLexicalStorage() &&
           !D1CXX->isCompleteDefinition()) {
         D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
       }
 
       if (D1CXX->getNumBases() != D2CXX->getNumBases()) {

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportFunctionTemplateSpecializations, DefinitionThenPrototype) {
   auto *To1 = LastDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
   EXPECT_TRUE(ImportedD == To0);
   EXPECT_TRUE(ImportedD != To1);
   EXPECT_TRUE(To0->doesThisDeclarationHaveABody());
   EXPECT_FALSE(To1->doesThisDeclarationHaveABody());
   EXPECT_EQ(To1->getPreviousDecl(), To0);
 }
 
+TEST_P(ASTImporterTestBase,
+    ImportShouldNotReportFalseODRErrorWhenRecordIsBeingDefined) {
+  {
+    Decl *FromTU = getTuDecl(
+        R"(
+            template <typename T>
+            struct B;
+            )",
+        Lang_CXX, "input0.cc");
+    auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+        FromTU, classTemplateDecl(hasName("B")));
+
+    Import(FromD, Lang_CXX);
+  }
+
+  {
+    Decl *FromTU = getTuDecl(
+        R"(
+            template <typename T>
+            struct B {
+              void f();
+              B* b;
+            };
+            )",
+        Lang_CXX, "input1.cc");
+    FunctionDecl *FromD = FirstDeclMatcher<FunctionDecl>().match(
+        FromTU, functionDecl(hasName("f")));
+    Import(FromD, Lang_CXX);
+    auto *FromCTD = FirstDeclMatcher<ClassTemplateDecl>().match(
+        FromTU, classTemplateDecl(hasName("B")));
+    auto *ToCTD = cast<ClassTemplateDecl>(Import(FromCTD, Lang_CXX));
+    EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+
+    // We expect no (ODR) warning during the import.
+    auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+    EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, CanonicalRedeclChain,
     ::testing::Values(ArgVector()),);
 
 auto DefaultTestValuesForRunOptions = ::testing::Values(