[clang] [lldb] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl (PR #107828)

[Andrew Savonichev via cfe-commits]

https://github.com/asavonic created https://github.com/llvm/llvm-project/pull/107828

Fields from external sources are typically loaded implicitly by `RecordDecl` or `DeclContext` iterators and other functions (see LoadFieldsFromExternalStorage function and its uses). The assumption is that we only need to load such fields whenever a complete list of fields is necessary. However, there are cases where implicit loads are not expected and they may break AST importer logic.

In ASTNodeImporter::VisitFieldDecl:

  1) We first check that a field is not imported already.
  2) Then proceed to import it.
  3) Finally add it to the record with `setLexicalDeclContext` and `addDeclInternal`.

If an implicit import happens between (2) and (3), it may indirectly bring the same field into the context. When (3) happens we add it again, duplicating the field and breaking the record. This is not detected until we crash later during layout computation:

    llvm/clang/lib/AST/RecordLayoutBuilder.cpp:81
    Assertion `FieldOffsets.count(FD) && "Field does not have an external offset"' failed.

Detecting a possible duplication is difficult, especially considering that `addDeclInternal` may cause an implicit import as well.

The patch attempts to workaround this problem by triggering implicit imports before (1). However, it is hard to tell if it covers all the cases, because some of them are nested: `DeclContext::addHiddenDecl` calls `CXXRecordDecl::addedMember`, which calls `Type::isLiteralType` on a base type, which tries to iterate over fields and cause an implicit load.

It is quite tricky to get a reproducer for such problems, because they depend on order of imports of fields and records. Debugging Unreal Engine v5.3.2 with LLDB shows this problem once issue #90938 is fixed or workarounded. Only some UE classes are affected. Reducing it down to a LIT test is problematic due to size of libraries involved, and "flaky" nature of the problem.

TestCppReferenceToOuterClass shows an improvement with the patch, but it likely has no relation to the problem.

>From ee3a6a5b4eef069c5cef2820aeab5ab773df69a4 Mon Sep 17 00:00:00 2001
From: Andrew Savonichev <andrew.savonichev at gmail.com>
Date: Mon, 9 Sep 2024 17:56:20 +0900
Subject: [PATCH] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl

Fields from external sources are typically loaded implicitly by
`RecordDecl` or `DeclContext` iterators and other functions (see
LoadFieldsFromExternalStorage function and its uses). The assumption
is that we only need to load such fields whenever a complete list of
fields is necessary. However, there are cases where implicit loads are
not expected and they may break AST importer logic.

In ASTNodeImporter::VisitFieldDecl:

  1) We first check that a field is not imported already.
  2) Then proceed to import it.
  3) Finally add it to the record with `setLexicalDeclContext` and `addDeclInternal`.

If an implicit import happens between (2) and (3), it may indirectly
bring the same field into the context. When (3) happens we add it
again, duplicating the field and breaking the record. This is not
detected until we crash later during layout computation:

  llvm/clang/lib/AST/RecordLayoutBuilder.cpp:81
  Assertion `FieldOffsets.count(FD) && "Field does not have an external offset"' failed.

Detecting a possible duplication is difficult, especially considering
that `addDeclInternal` may cause an implicit import as well.

The patch attempts to workaround this problem by triggering implicit
imports before (1). However, it is hard to tell if it covers all the
cases, because some of them are nested: `DeclContext::addHiddenDecl`
calls `CXXRecordDecl::addedMember`, which calls `Type::isLiteralType`
on a base type, which tries to iterate over fields and cause an
implicit load.

It is quite tricky to get a reproducer for such problems, because they
depend on order of imports of fields and records. Debugging Unreal
Engine v5.3.2 with LLDB shows this problem once issue #90938 is fixed
or workarounded. Only some UE classes are affected. Reducing it down
to a LIT test is problematic due to size of libraries involved, and
"flaky" nature of the problem.

TestCppReferenceToOuterClass shows an improvement with the patch, but
it likely has no relation to the problem.
---
 clang/lib/AST/ASTImporter.cpp                 | 22 +++++++++++++++++--
 .../TestCppReferenceToOuterClass.py           |  1 -
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c2fb7dddcfc637..6e48f21c57d9d6 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -4172,6 +4172,26 @@ ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
   if (ToD)
     return ToD;
 
+  // Implicit imports of external fields may import the same field
+  // *after* we check for its presence with findDeclsInToCtx. If this
+  // happens we may import the field twice and break the record
+  // type.
+  //
+  // Force import the context now to avoid this problem.
+  DC->decls_begin();
+  LexicalDC->decls_begin();
+
+  // C++ types may cause an import of fields later, so force import them too.
+  Error Err = Error::success();
+  auto ToType = importChecked(Err, D->getType());
+  if (!ToType.isNull()) {
+    if (const auto *RT = ToType->getAs<RecordType>()) {
+      if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl())) {
+        ClassDecl->decls_begin();
+      }
+    }
+  }
+
   // Determine whether we've already imported this field.
   auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
   for (auto *FoundDecl : FoundDecls) {
@@ -4217,8 +4237,6 @@ ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
     }
   }
 
-  Error Err = Error::success();
-  auto ToType = importChecked(Err, D->getType());
   auto ToTInfo = importChecked(Err, D->getTypeSourceInfo());
   auto ToBitWidth = importChecked(Err, D->getBitWidth());
   auto ToInnerLocStart = importChecked(Err, D->getInnerLocStart());
diff --git a/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py b/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
index a6e419b7fcdfa2..cb28e2b31fad14 100644
--- a/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
+++ b/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
@@ -6,7 +6,6 @@
 
 
 class TestCase(TestBase):
-    @unittest.expectedFailure  # The fix for this was reverted due to llvm.org/PR52257
     def test(self):
         self.build()
         self.dbg.CreateTarget(self.getBuildArtifact("a.out"))