This revision was automatically updated to reflect the committed changes.
Closed by commit rL366325: [ASTImporter] Fix LLDB lookup in transparent ctx and
with ext src (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D61333?vs=210281&id=210318#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61333/new/
https://reviews.llvm.org/D61333
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/main.c
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===================================================================
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1707,6 +1707,17 @@
Error ASTNodeImporter::ImportDefinition(
RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind) {
+ auto DefinitionCompleter = [To]() {
+ // There are cases in LLDB when we first import a class without its
+ // members. The class will have DefinitionData, but no members. Then,
+ // importDefinition is called from LLDB, which tries to get the members, so
+ // when we get here, the class already has the DefinitionData set, so we
+ // must unset the CompleteDefinition here to be able to complete again the
+ // definition.
+ To->setCompleteDefinition(false);
+ To->completeDefinition();
+ };
+
if (To->getDefinition() || To->isBeingDefined()) {
if (Kind == IDK_Everything ||
// In case of lambdas, the class already has a definition ptr set, but
@@ -1717,7 +1728,7 @@
Error Result = ImportDeclContext(From, /*ForceImport=*/true);
// Finish the definition of the lambda, set isBeingDefined to false.
if (To->isLambda())
- To->completeDefinition();
+ DefinitionCompleter();
return Result;
}
@@ -1728,8 +1739,8 @@
// 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.
- auto DefinitionCompleter =
- llvm::make_scope_exit([To]() { To->completeDefinition(); });
+ auto DefinitionCompleterScopeExit =
+ llvm::make_scope_exit(DefinitionCompleter);
if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
return Err;
@@ -7757,10 +7768,20 @@
SharedState->getLookupTable()->lookup(ReDC, Name);
return FoundDeclsTy(LookupResult.begin(), LookupResult.end());
} else {
- // FIXME Can we remove this kind of lookup?
- // Or lldb really needs this C/C++ lookup?
- FoundDeclsTy Result;
- ReDC->localUncachedLookup(Name, Result);
+ DeclContext::lookup_result NoloadLookupResult = ReDC->noload_lookup(Name);
+ FoundDeclsTy Result(NoloadLookupResult.begin(), NoloadLookupResult.end());
+ // We must search by the slow case of localUncachedLookup because that is
+ // working even if there is no LookupPtr for the DC. We could use
+ // DC::buildLookup() to create the LookupPtr, but that would load external
+ // decls again, we must avoid that case.
+ // Also, even if we had the LookupPtr, we must find Decls which are not
+ // in the LookupPtr, so we need the slow case.
+ // These cases are handled in ASTImporterLookupTable, but we cannot use
+ // that with LLDB since that traverses through the AST which initiates the
+ // load of external decls again via DC::decls(). And again, we must avoid
+ // loading external decls during the import.
+ if (Result.empty())
+ ReDC->localUncachedLookup(Name, Result);
return Result;
}
}
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===================================================================
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -5122,6 +5122,51 @@
EXPECT_EQ(ToLSize, FromLSize);
}
+struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
+ LLDBLookupTest() {
+ Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr<ASTImporterSharedState> &SharedState) {
+ return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+ };
+ }
+};
+
+TEST_P(LLDBLookupTest, ImporterShouldFindInTransparentContext) {
+ TranslationUnitDecl *ToTU = getToTuDecl(
+ R"(
+ extern "C" {
+ class X{};
+ };
+ )",
+ Lang_CXX);
+ auto *ToX = FirstDeclMatcher<CXXRecordDecl>().match(
+ ToTU, cxxRecordDecl(hasName("X")));
+
+ // Set up a stub external storage.
+ ToTU->setHasExternalLexicalStorage(true);
+ // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+ ToTU->setMustBuildLookupTable();
+ struct TestExternalASTSource : ExternalASTSource {};
+ ToTU->getASTContext().setExternalSource(new TestExternalASTSource());
+
+ Decl *FromTU = getTuDecl(
+ R"(
+ class X;
+ )",
+ Lang_CXX);
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X")));
+ auto *ImportedX = Import(FromX, Lang_CXX);
+ // The lookup must find the existing class definition in the LinkageSpecDecl.
+ // Then the importer renders the existing and the new decl into one chain.
+ EXPECT_EQ(ImportedX->getCanonicalDecl(), ToX->getCanonicalDecl());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, );
@@ -5168,5 +5213,8 @@
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
DefaultTestValuesForRunOptions, );
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
+ DefaultTestValuesForRunOptions, );
+
} // end namespace ast_matchers
} // end namespace clang
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/main.c
===================================================================
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
typedef struct {
int a;
int b;
-} FILE;
+} MYFILE;
int main()
{
- FILE *myFile = malloc(sizeof(FILE));
+ MYFILE *myFile = malloc(sizeof(MYFILE));
myFile->a = 5;
myFile->b = 9;
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
===================================================================
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
@@ -47,6 +47,10 @@
self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
substrs=[' resolved, hit count = 1'])
+ # Enable logging of the imported AST.
+ log_file = os.path.join(self.getBuildDir(), "lldb-ast-log.txt")
+ self.runCmd("log enable lldb ast -f '%s'" % log_file)
+
self.expect(
"expr -l objc++ -- @import Darwin; 3",
VARIABLES_DISPLAYED_CORRECTLY,
@@ -54,6 +58,8 @@
"int",
"3"])
+ # This expr command imports __sFILE with definition
+ # (FILE is a typedef to __sFILE.)
self.expect(
"expr *fopen(\"/dev/zero\", \"w\")",
VARIABLES_DISPLAYED_CORRECTLY,
@@ -61,6 +67,14 @@
"FILE",
"_close"])
+ # Check that the AST log contains exactly one definition of __sFILE.
+ f = open(log_file)
+ log_lines = f.readlines()
+ f.close()
+ os.remove(log_file)
+ self.assertEqual(" ".join(log_lines).count("struct __sFILE definition"),
+ 1)
+
self.expect("expr *myFile", VARIABLES_DISPLAYED_CORRECTLY,
substrs=["a", "5", "b", "9"])
Index: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===================================================================
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -612,10 +612,15 @@
if (!original_decl_context)
return;
+ // Indicates whether we skipped any Decls of the original DeclContext.
+ bool SkippedDecls = false;
for (TagDecl::decl_iterator iter = original_decl_context->decls_begin();
iter != original_decl_context->decls_end(); ++iter) {
Decl *decl = *iter;
+ // The predicate function returns true if the passed declaration kind is
+ // the one we are looking for.
+ // See clang::ExternalASTSource::FindExternalLexicalDecls()
if (predicate(decl->getKind())) {
if (log) {
ASTDumper ast_dumper(decl);
@@ -640,21 +645,22 @@
m_ast_importer_sp->RequireCompleteType(copied_field_type);
}
-
- DeclContext *decl_context_non_const =
- const_cast<DeclContext *>(decl_context);
-
- if (copied_decl->getDeclContext() != decl_context) {
- if (copied_decl->getDeclContext()->containsDecl(copied_decl))
- copied_decl->getDeclContext()->removeDecl(copied_decl);
- copied_decl->setDeclContext(decl_context_non_const);
- }
-
- if (!decl_context_non_const->containsDecl(copied_decl))
- decl_context_non_const->addDeclInternal(copied_decl);
+ } else {
+ SkippedDecls = true;
}
}
+ // CopyDecl may build a lookup table which may set up ExternalLexicalStorage
+ // to false. However, since we skipped some of the external Decls we must
+ // set it back!
+ if (SkippedDecls) {
+ decl_context->setHasExternalLexicalStorage(true);
+ // This sets HasLazyExternalLexicalLookups to true. By setting this bit we
+ // ensure that the lookup table is rebuilt, which means the external source
+ // is consulted again when a clang::DeclContext::lookup is called.
+ const_cast<DeclContext *>(decl_context)->setMustBuildLookupTable();
+ }
+
return;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
