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

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Andrew Savonichev (asavonic)

<details>
<summary>Changes</summary>

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/107828.diff


2 Files Affected:

- (modified) clang/lib/AST/ASTImporter.cpp (+20-2) 
- (modified) lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py (-1) 


``````````diff
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"))

``````````

</details>


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