[clang][AST][ASTImporter] Set record to complete during import of its members.

[balazske]

Summary

At import of a member it may require that the record is already set to complete.
(For example 'computeDependence' at create of some Expr nodes.)
The record at this time may not be completely imported, the result of layout
calculations can be incorrect, but at least no crash occurs this way.

A good solution would be if fields of every encountered record are imported
before other members of all records. This is much more difficult to implement.

[shafik]

clang/lib/AST/ASTImporter.cpp

2021

So DefinitionCompleterScopeExit will run To->setCompleteDefinition(false); after this function exits but this will be in effect during the import the base classes. I don't see how the tests you added hit that code.

clang/unittests/AST/ASTImporterTest.cpp

7485

The member function body should be considered complete-class context so the correct thing to do would be have all the fields laid out by this point.

[balazske]

clang/lib/AST/ASTImporter.cpp

2021

Should the test CompleteRecordBeforeImporting not do the import in minimal mode? The comment says minimal but in ASTImporter minimal mode is not set. The test will fail because this added line. But I think it should work to call the To->setCompleteDefinition here only in non-minimal mode.

2088

My fix was not correct. This line was added to make a test CompleteRecordBeforeImporting not fail but it makes the original fix not work. Something other is needed.

clang/unittests/AST/ASTImporterTest.cpp

7485

My initial plan was to import first all fields, then everything else. But it is possible that a field has reference to the same record before the import of all fields finishes (like in the second test) so this can not work in all cases (and it caused other test failures).

[balazske]

clang/lib/AST/ASTImporter.cpp

2021

And remove the later added line 2088 To->setCompleteDefinition(false);.

[martong]

clang/lib/AST/ASTImporter.cpp

2021

Should the test CompleteRecordBeforeImporting not do the import in minimal mode?

Yes, that should, however, I can confirm it probably does not (see the false arg below):

Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
                       Unit->getASTContext(), Unit->getFileManager(), false,
                       SharedState));

So, first we should fix the test case CompleteRecordBeforeImporting to set up the ASTImporter in minimal mode.

Then, we should call the To->setCompleteDefinition here only in non-minimal mode as you suggests. Once again, an ugly branching because of the "minimal" mode, we should really get rid of that (and hope that Raphael patch evolves in D101950).

[balazske]

Ping.
I want to see opinion of @shafik (or others) about change of test CompleteRecordBeforeImporting (turn on minimal import mode in this test).

[martong]

Adding Raphael @teemperor , he might have useful comments about the minimal mode.

[teemperor]

I'm really sorry @martong , but I no longer work on LLDB (or the ASTImporter) and I'm not really in the loop regarding LLDB development.

(Having said that, I still happy to answer questions about my own patches/reviews that I did in the past in case there are questions about that)

[balazske]

@martong Ping
We do not have possibility currently to check all LLDB tests. I think we can commit this and observe the buildbots for failure.

[balazske]

(The current code does not work, the mentioned fixes must be applied first.)

[steakhal]

In D116155#3303622, @balazske wrote:

@martong Ping
We do not have possibility currently to check all LLDB tests. I think we can commit this and observe the buildbots for failure.

I agree. Let's see if it works out. I don't think we shall wait for longer.
This should be already better than crashing time-to-time.
We can always revert if necessary.

[steakhal]

Well, I've just recognized that the "Build Status" of this latest diff shows that it crashes.

[balazske]

Fix of test failures.

[balazske]

Rebase.

clang/lib/AST/ASTImporter.cpp

if (Kind == IDK_Everything ||
         DefinitionCompleter();
       return Result;
     }
 
     return Error::success();
   }
 
   To->startDefinition();
+  // Set the definition to complete even if it is really not complete during
+  // import. Some AST constructs (expressions) require the record layout
+  // to be calculated (see 'clang::computeDependence') at the time they are
+  // constructed. Import of such AST node is possible during import of the
+  // same record, there is no way to have a completely defined record (all
+  // fields imported) at that time without multiple AST import passes.
+  To->setCompleteDefinition(true);
   // Complete the definition even if error is returned.
   // The RecordDecl may be already part of the AST so it is better to
   // have it in complete state even if something is wrong with it.
   auto DefinitionCompleterScopeExit =
       llvm::make_scope_exit(DefinitionCompleter);
 
   if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
     return Err;
for (const auto &Base1 : FromCXX->bases()) {
               Base1.getAccessSpecifierAsWritten(),
               *TSIOrErr,
               EllipsisLoc));
     }
     if (!Bases.empty())
       ToCXX->setBases(Bases.data(), Bases.size());
   }
 
-  if (shouldForceImportDeclContext(Kind))
+  if (shouldForceImportDeclContext(Kind)) {
+    // Set to false here to force "completion" of the record.
+    To->setCompleteDefinition(false);
     if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
       return Err;
+  }
 
   return Error::success();
 }
 
 Error ASTNodeImporter::ImportInitializer(VarDecl *From, VarDecl *To) {
   if (To->getAnyInitializer())
     return Error::success();
 

clang/unittests/AST/ASTImporterTest.cpp

EXPECT_EQ(
       ToDGCtor->getTemplatedDecl());
   EXPECT_EQ(ToDGOther->getDescribedTemplate()
                 ->getTemplateParameters()
                 ->getParam(0)
                 ->getDeclContext(),
             ToDGOther);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportRecordWithLayoutRequestingExpr) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+      R"(
+      struct A {
+        int idx;
+        static void foo(A x) {
+          (void)&"text"[x.idx];
+        }
+      };
+      )",
+      Lang_CXX11);
+
+  auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("A")));
+
+  // Test that during import of 'foo' the record layout can be obtained without
+  // crash.
+  auto *ToA = Import(FromA, Lang_CXX11);
+  EXPECT_TRUE(ToA);
+  EXPECT_TRUE(ToA->isCompleteDefinition());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportRecordWithLayoutRequestingExprDifferentRecord) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+      R"(
+      struct B;
+      struct A {
+        int idx;
+        B *b;
+      };
+      struct B {
+        static void foo(A x) {
+          (void)&"text"[x.idx];
+        }
+      };
+      )",
+      Lang_CXX11);
+
+  auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("A")));
+
+  // Test that during import of 'foo' the record layout (of 'A') can be obtained
+  // without crash. It is not possible to have all of the fields of 'A' imported
+  // at that time (without big code changes).
+  auto *ToA = Import(FromA, Lang_CXX11);
+  EXPECT_TRUE(ToA);
+  EXPECT_TRUE(ToA->isCompleteDefinition());
+}
+
 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);