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