[ASTImporter] Fix 'isVirtual()' assert failure while importing overridden methods

[danix800]

Summary

CXXMethodDecl::isVirtual() count the number of overridden methods.
This assertion is not true before overridden methods are fully loaded.
The body of this CXXMethodDecl can introduce deps on a derived class
which contains a method overriding this method, causing the assertion failure.

ImportOverriddenMethods() is moved before body loading to fix this issue.

Testcase is contributed by Balázs Kéri (balazske)

[balazske]

It is possible to reproduce the same problem with this test added to ASTImporterTest.cpp:

TEST_P(ASTImporterOptionSpecificTestBase, ImportVirtualOverriddenMethodTest) {
  const char *Code =
      R"(
      void f1();
      class A {
        virtual void f(){}
      };
      class B: public A {
        void f() override {
          f1();
        }
      };
      class C: public B {
        void f() override {}
      };
      void f1() { C c; }
      )";
  Decl *FromTU = getTuDecl(Code, Lang_CXX11);

  auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
      FromTU, cxxMethodDecl(hasName("B::f")));

  auto *ToBF = Import(FromF, Lang_CXX11);
  EXPECT_TRUE(ToBF->isVirtual());

  auto *ToCF = FirstDeclMatcher<CXXMethodDecl>().match(
      ToBF->getTranslationUnitDecl(), cxxMethodDecl(hasName("C::f")));
  EXPECT_TRUE(ToCF->isVirtual());
}

I am not opposed to removal of the assertion as fix because it looks not very important (probably it can be replaced by another assertion for example to not allow constructors here, see this commit) but another person in the AST area should check this.

clang/test/Analysis/ctu-astimport-virtual-assertion/main.cpp

22 ↗

(On Diff #538067)

Such tests are not in the Analysis folder but in the ASTMerge folder instead. I would say that this test is not necessary if the other test (in my added note) is inserted.

[danix800]

clang/test/Analysis/ctu-astimport-virtual-assertion/main.cpp

22 ↗

(On Diff #538067)

Thanks for the testcase! The orignal one will be removed.

[danix800]

The root cause is on a circular dependency loop. Importing overridden methods before method body can fix this.

clang/lib/AST/ASTImporter.cpp

if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
       ToCtor->setNumCtorInitializers(NumInitializers);
     }
   }
 
   // If it is a template, import all related things.
   if (Error Err = ImportTemplateInformation(D, ToFunction))
     return std::move(Err);
 
+  if (auto *FromCXXMethod = dyn_cast<CXXMethodDecl>(D))
+    if (Error Err = ImportOverriddenMethods(cast<CXXMethodDecl>(ToFunction),
+                                            FromCXXMethod))
+      return std::move(Err);
+
   if (D->doesThisDeclarationHaveABody()) {
     Error Err = ImportFunctionDeclBody(D, ToFunction);
 
     if (Err)
       return std::move(Err);
   }
 
   // Import and set the original type in case we used another type.
   if (UsedDifferentProtoType) {
     if (ExpectedType TyOrErr = import(D->getType()))
       ToFunction->setType(*TyOrErr);
     else
       return TyOrErr.takeError();
     if (Expected<TypeSourceInfo *> TSIOrErr = import(D->getTypeSourceInfo()))
       ToFunction->setTypeSourceInfo(*TSIOrErr);
     else
       return TSIOrErr.takeError();
   }
 
   // FIXME: Other bits to merge?
 
   addDeclToContexts(D, ToFunction);
 
-  if (auto *FromCXXMethod = dyn_cast<CXXMethodDecl>(D))
-    if (Error Err = ImportOverriddenMethods(cast<CXXMethodDecl>(ToFunction),
-                                            FromCXXMethod))
-      return std::move(Err);
-
   // Import the rest of the chain. I.e. import all subsequent declarations.
   for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt) {
     ExpectedDecl ToRedeclOrErr = import(*RedeclIt);
     if (!ToRedeclOrErr)
       return ToRedeclOrErr.takeError();
   }
 
   return ToFunction;

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportFunctions,
   EXPECT_EQ(ToDFOutOfClass->getDeclContext(), ToD);
   EXPECT_EQ(ToDFOutOfClass->getLexicalDeclContext(), ToTU);
 
   // Check that the redecl chain is intact.
   EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
   EXPECT_EQ(ToDFOutOfClass->getPreviousDecl(), ToDFInClass);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportVirtualOverriddenMethodOnALoopTest) {
+  // B::f() calls => f1() ==> C ==> C::f()
+  //     \
+  //      \---- A::f()
+  //
+  // C::f()'s ImportOverriddenMethods() asserts B::isVirtual(), so B::f()'s
+  // ImportOverriddenMethods() should be completed before B::f()'s body
+  const char *Code =
+      R"(
+      void f1();
+      class A {
+        virtual void f(){}
+      };
+      class B: public A {
+        void f() override {
+          f1();
+        }
+      };
+      class C: public B {
+        void f() override {}
+      };
+      void f1() { C c; }
+      )";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+
+  auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
+      FromTU, cxxMethodDecl(hasName("B::f")));
+
+  auto *ToBF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToBF->isVirtual());
+
+  auto *ToCF = FirstDeclMatcher<CXXMethodDecl>().match(
+      ToBF->getTranslationUnitDecl(), cxxMethodDecl(hasName("C::f")));
+  EXPECT_TRUE(ToCF->isVirtual());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, ImportVariableChainInC) {
     std::string Code = "static int v; static int v = 0;";
     auto Pattern = varDecl(hasName("v"));
 
     TranslationUnitDecl *FromTu = getTuDecl(Code, Lang_C99, "input0.c");
 
     auto *From0 = FirstDeclMatcher<VarDecl>().match(FromTu, Pattern);
     auto *From1 = LastDeclMatcher<VarDecl>().match(FromTu, Pattern);