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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
