[ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[martong]

Summary

With LLDB we use localUncachedLookup(), however, that fails to find
Decls when a transparent context is involved and the given DC has
external lexical storage. The solution is to use noload_lookup, which
works well with transparent contexts. But, we cannot use only the
noload_lookup since the slow case of localUncachedLookup is still needed
in some other cases.

These other 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().

We must avoid loading external decls during the import becuase
ExternalASTSource is implemented with ASTImporter, so external loads
during import results in uncontrolled and faulty import.

[martong]

lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c

8 ↗

(On Diff #197409)

In TestCmodules.py we have import Darwin and then expr *fopen("/dev/zero", "w"). This imports the name FILE from the Darwin module. Then when we want expr *myFile, the two different definition of the FILE structs collides.
This happens because now the lookup finds the existing definition for the FILE in the TU associated with the expression evaluator, and that comes from the Darwin module.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

640 ↗

(On Diff #197409)

We must remove the below addDeclInternal call, because that may be called from another
addDeclInternal:

6  clang::CXXConstructorDecl::isDefaultConstructor() const + 87
7  clang::CXXRecordDecl::addedMember(clang::Decl*) + 803
8  clang::DeclContext::addHiddenDecl(clang::Decl*) + 341
9  clang::DeclContext::addDeclInternal(clang::Decl*) + 29
10 lldb_private::ClangASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 3917
11 lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 102
12 clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::SmallVectorImpl<clang::Decl*>&) + 101
13 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const + 269
14 clang::DeclContext::buildLookup() + 336
15 clang::DeclContext::makeDeclVisibleInContextWithFlags(clang::NamedDecl*, bool, bool) + 403
16 clang::DeclContext::addDeclInternal(clang::Decl*) + 92
17 clang::ASTNodeImporter::VisitFieldDecl(clang::FieldDecl*) + 3851

... and at the second call of addDeclInternal we may add an incomplete Decl (CXXConstructorDecl above).
We must always avoid redundant work in lldb which is already handled in Clang::ASTImporter.

[a_sidorin]

Hello Gabor!
This looks good to me, but let's wait for LLDB guys to take a look at the patch. Thanks!

[martong]

Ping @shafik @teemperor

[shafik]

I ran check-lldb and I hit one regression in TestFormatters.py, part of what I am seeing is as follows:

AssertionError: False is not True : FileCheck'ing result of `expression --show-types -- *(new foo(47))`
Error output:
error: no matching constructor for initialization of 'foo'
candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const foo' for 1st argument
candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'foo' for 1st argument

clang/lib/AST/ASTImporter.cpp

1818 ↗

(On Diff #197409)

se -> so?

lldb/source/Symbol/ClangASTImporter.cpp

922 ↗

(On Diff #197409)

I am going to say that the logging change is an excellent additions and stands alone from this change. Although I realize the test depends on this new feature. It makes sense to add the logging in a separate PR.

I also say this b/c I found a regression and it would be nice to get the logging in while we resolve the regression.

[martong]

• Rebase to master
• Rebase to D62061

[martong]

lldb/source/Symbol/ClangASTImporter.cpp

922 ↗

(On Diff #197409)

Ok, I have created a new patch for logging (https://reviews.llvm.org/D62061).
I made this patch to be the child of that and rebased to that.

[martong]

• se -> so

[martong]

• Fix regression of TestFormatters.py

[martong]

About the regression of TestFormatters.py: I realized that the problem is about the wrong implementation of the ExternalASTSource interface.
In the implementation of FindExternalLexicalDecls of this interface, we simply ignored those cases when the given predicate (passed as a param) is false. When that happens, that means we still have some more external decls which should be dug up by the upcoming calls of DeclContext::lookup. The fix is about to indicate this.

[martong]

@shafik @jingham This is a polite Ping.

[teemperor]

@martong Sorry for the delay, feel free to ping me in the future about these patches. I'll review them ASAP now that I'm back in office, so these delay's hopefully won't happen again.

I tried applying this patch and it seems it needs to be rebased. I would do it myself, but I'm not entirely sure how to rebase the changes to ASTNodeImporter::ImportDefinition. It seems we got rid of To->completeDefinition();, so not sure if the code that this patch adds is still in the right place.

[martong]

• Rebase to master

[martong]

In D61333#1583173, @teemperor wrote:

@martong Sorry for the delay, feel free to ping me in the future about these patches. I'll review them ASAP now that I'm back in office, so these delay's hopefully won't happen again.

I tried applying this patch and it seems it needs to be rebased. I would do it myself, but I'm not entirely sure how to rebase the changes to ASTNodeImporter::ImportDefinition. It seems we got rid of To->completeDefinition();, so not sure if the code that this patch adds is still in the right place.

Raphael,

Thank you for looking into this. I've rebased to master and updated the patch.

[shafik]

This LGTM now but I will wait for @teemperor to take a look at it.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

620 ↗

(On Diff #209832)

I think a comment on the predicate would be helpful as well.

[teemperor]

On my system clang-format has some complaints, so I think you need to rerun clang-format. Probably caused by the rebasing.

I have some minor comments about the TestAST.py (see the inline comments), but otherwise this patch LGTM.

lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py

1 ↗

(On Diff #209832)

This comment (and the name of the test class) are copy-pasted from TestCModules and should be updated. If we can merge the two test cases (see the inline comment below), then this would be fixed as a side-effect.

57 ↗

(On Diff #209832)

I think this test has a good chance to fail once someone renamed/removes/changes this internal struct (especially if it's currently abstracted with a typedef). Would it be possible to just make a minimal module with open and FILE/__sFILE instead? If it's too much trouble, then I'm also fine with merging this as-is (as this regression is easy to fix).

67 ↗

(On Diff #209832)

It seems that this is the only different in the test compared to TestCModules.py. Would it be possible to just add this logging/checking to TestCModules.py as it's anyway testing something very similar?

[martong]

• Applied clang-format on lldb parts (this changed two lines)
• Added a comment for predicate
• Merged the test into TestCModules.py

[martong]

lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py

57 ↗

(On Diff #209832)

I'd rather keep this as-is, because I don't have enough confidence and experience with c modules (and with macOS).

67 ↗

(On Diff #209832)

Ok, I have removed the TestAST.py and added the extra logging and the check into TestCModules.py.

[teemperor]

LGTM, thanks a lot for fixing this!

[martong]

Thank you guys for the review!

[martong]

Jenkins looks okay: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31157/ .The build is red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py

cfe/trunk/lib/AST/ASTImporter.cpp

if (TypedefNameDecl *FromTypedef = From->getTypedefNameForAnonDecl()) {
     else
       return ToTypedefOrErr.takeError();
   }
   return Error::success();
 }
 
 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
         // the contained decls are not imported yet. Also, isBeingDefined was
         // set in CXXRecordDecl::CreateLambda.  We must import the contained
         // decls here and finish the definition.
         (To->isLambda() && shouldForceImportDeclContext(Kind))) {
       Error Result = ImportDeclContext(From, /*ForceImport=*/true);
       // Finish the definition of the lambda, set isBeingDefined to false.
       if (To->isLambda())
-        To->completeDefinition();
+        DefinitionCompleter();
       return Result;
     }
 
     return Error::success();
   }
 
   To->startDefinition();
   // 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;
 
   // Add base classes.
   auto *ToCXX = dyn_cast<CXXRecordDecl>(To);
   auto *FromCXX = dyn_cast<CXXRecordDecl>(From);
   if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
ASTImporter::findDeclsInToCtx(DeclContext *DC, DeclarationName Name) {
   // then the enum constant 'A' and the variable 'A' violates ODR.
   // We can diagnose this only if we search in the redecl context.
   DeclContext *ReDC = DC->getRedeclContext();
   if (SharedState->getLookupTable()) {
     ASTImporterLookupTable::LookupResult LookupResult =
         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;
+    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;
   }
 }
 
 void ASTImporter::AddToLookupTable(Decl *ToD) {
   SharedState->addDeclToLookup(ToD);
 }
 

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionParam) {
   auto ToL = Import(FromL, Lang_CXX11);
   unsigned ToLSize = std::distance(ToL->decls().begin(), ToL->decls().end());
   unsigned FromLSize =
       std::distance(FromL->decls().begin(), FromL->decls().end());
   EXPECT_NE(ToLSize, 0u);
   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, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
INSTANTIATE_TEST_CASE_P(ParameterizedTests,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
+                        DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang

lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py

def test_expr(self):
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped',
                              'stop reason = breakpoint'])
 
         # The breakpoint should have a hit count of 1.
         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,
             substrs=[
                 "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,
             substrs=[
                 "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"])
 
         self.expect(
             "expr MIN((uint64_t)2, (uint64_t)3)",
             VARIABLES_DISPLAYED_CORRECTLY,
             substrs=[
                 "uint64_t",

lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/main.c

 #include <stdlib.h>
 
 int printf(const char * __restrict format, ...);
 
 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;
 
     printf("%d\n", myFile->a + myFile->b); // Set breakpoint 0 here.
 
     free(myFile);
 }

lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

void ClangASTSource::FindExternalLexicalDecls(
   }
 
   const DeclContext *original_decl_context =
       dyn_cast<DeclContext>(original_decl);
 
   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);
         if (const NamedDecl *context_named_decl =
                 dyn_cast<NamedDecl>(context_decl))
           log->Printf("  FELD[%d] Adding [to %sDecl %s] lexical %sDecl %s",
                       current_id, context_named_decl->getDeclKindName(),
                       context_named_decl->getNameAsString().c_str(),
                       decl->getDeclKindName(), ast_dumper.GetCString());
         else
           log->Printf("  FELD[%d] Adding lexical %sDecl %s", current_id,
                       decl->getDeclKindName(), ast_dumper.GetCString());
       }
 
       Decl *copied_decl = CopyDecl(decl);
 
       if (!copied_decl)
         continue;
 
       if (FieldDecl *copied_field = dyn_cast<FieldDecl>(copied_decl)) {
         QualType copied_field_type = copied_field->getType();
 
         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;
 }
 
 void ClangASTSource::FindExternalVisibleDecls(NameSearchContext &context) {
   assert(m_ast_context);