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

[Balázs Kéri via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG83028ad934d6: [clang][AST][ASTImporter] Set record to 
complete during import of its members. (authored by balazske).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6809,7 +6809,8 @@
  bool MinimalImport,
  const std::shared_ptr ) {
   return new ASTImporter(ToContext, ToFileManager, FromContext,
- FromFileManager, MinimalImport,
+ // Use minimal import for these tests.
+ FromFileManager, /*MinimalImport=*/true,
  // We use the regular lookup.
  /*SharedState=*/nullptr);
 };
@@ -7475,6 +7476,57 @@
 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().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().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);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2012,6 +2012,14 @@
   }
 
   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.
+  if (!Importer.isMinimalImport())
+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.
@@ -2076,9 +2084,10 @@
   ToCXX->setBases(Bases.data(), Bases.size());
   }
 
-  if (shouldForceImportDeclContext(Kind))
+  if (shouldForceImportDeclContext(Kind)) {
 if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
   return Err;
+  }
 
   return Error::success();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 407860.
balazske added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6809,7 +6809,8 @@
  bool MinimalImport,
  const std::shared_ptr ) {
   return new ASTImporter(ToContext, ToFileManager, FromContext,
- FromFileManager, MinimalImport,
+ // Use minimal import for these tests.
+ FromFileManager, /*MinimalImport=*/true,
  // We use the regular lookup.
  /*SharedState=*/nullptr);
 };
@@ -7475,6 +7476,57 @@
 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().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().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);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2012,6 +2012,14 @@
   }
 
   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.
+  if (!Importer.isMinimalImport())
+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.
@@ -2076,9 +2084,10 @@
   ToCXX->setBases(Bases.data(), Bases.size());
   }
 
-  if (shouldForceImportDeclContext(Kind))
+  if (shouldForceImportDeclContext(Kind)) {
 if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
   return Err;
+  }
 
   return Error::success();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 407822.
balazske added a comment.

Fix of test failures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6809,7 +6809,8 @@
  bool MinimalImport,
  const std::shared_ptr ) {
   return new ASTImporter(ToContext, ToFileManager, FromContext,
- FromFileManager, MinimalImport,
+ // Use minimal import for these tests.
+ FromFileManager, /*MinimalImport=*/true,
  // We use the regular lookup.
  /*SharedState=*/nullptr);
 };
@@ -7475,6 +7476,57 @@
 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().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().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);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2012,6 +2012,14 @@
   }
 
   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.
+  if (!Importer.isMinimalImport())
+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.
@@ -2076,9 +2084,10 @@
   ToCXX->setBases(Bases.data(), Bases.size());
   }
 
-  if (shouldForceImportDeclContext(Kind))
+  if (shouldForceImportDeclContext(Kind)) {
 if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
   return Err;
+  }
 
   return Error::success();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a subscriber: teemperor.
martong added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2021
+  // fields imported) at that time without multiple AST import passes.
+  To->setCompleteDefinition(true);
   // Complete the definition even if error is returned.

balazske wrote:
> balazske wrote:
> > shafik wrote:
> > > 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.
> > 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.
> And remove the later added line 2088 `To->setCompleteDefinition(false);`.
> 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).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2021
+  // fields imported) at that time without multiple AST import passes.
+  To->setCompleteDefinition(true);
   // Complete the definition even if error is returned.

balazske wrote:
> shafik wrote:
> > 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.
> 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.
And remove the later added line 2088 `To->setCompleteDefinition(false);`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2021
+  // fields imported) at that time without multiple AST import passes.
+  To->setCompleteDefinition(true);
   // Complete the definition even if error is returned.

shafik wrote:
> 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.
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.



Comment at: clang/lib/AST/ASTImporter.cpp:2088
+// Set to false here to force "completion" of the record.
+To->setCompleteDefinition(false);
 if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))

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.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:7485
+static void foo(A x) {
+  (void)&"text"[x.idx];
+}

shafik wrote:
> 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.
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2021
+  // fields imported) at that time without multiple AST import passes.
+  To->setCompleteDefinition(true);
   // Complete the definition even if error is returned.

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.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:7485
+static void foo(A x) {
+  (void)&"text"[x.idx];
+}

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116155/new/

https://reviews.llvm.org/D116155

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: steakhal, martong, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116155

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7475,6 +7475,57 @@
 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().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().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);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2012,6 +2012,13 @@
   }
 
   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.
@@ -2076,9 +2083,12 @@
   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();
 }


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7475,6 +7475,57 @@
 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().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(
+