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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thank you guys for the review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

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 &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().match(
+  ToTU, cxxRecordDecl(ha

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks a lot for fixing this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 210281.
martong marked 5 inline comments as done.
martong added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -611,10 +611,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);
@@ -639,21 +644,22 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(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(decl_context)->setMustBuildLookupTable();
+  }
+
   return;
 }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/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/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
+++ lldb/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: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5122,6 +5122,51 @@
   EXPECT_EQ(ToLSize, F

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

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:57
+# This expr command imports __sFILE with definition
+# (FILE is a typedef to __sFILE.)
+self.expect(

teemperor wrote:
> 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).
I'd rather keep this as-is, because I don't have enough confidence and 
experience with c modules (and with macOS).



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:67
+# Check that the AST log contains exactly one definition of __sFILE.
+f = open(log_file)
+log_lines = f.readlines()

teemperor wrote:
> 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?
Ok, I have removed the TestAST.py and added the extra logging and the check 
into TestCModules.py.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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.




Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:1
+"""Test that importing modules in C works as expected."""
+

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.



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:57
+# This expr command imports __sFILE with definition
+# (FILE is a typedef to __sFILE.)
+self.expect(

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).



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:67
+# Check that the AST log contains exactly one definition of __sFILE.
+f = open(log_file)
+log_lines = f.readlines()

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

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




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:620
 
 if (predicate(decl->getKind())) {
   if (log) {

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 209832.
martong added a comment.

- Rebase to master


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -611,6 +611,8 @@
   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;
@@ -639,21 +641,22 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(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(decl_context)->setMustBuildLookupTable();
+  }
+
   return;
 }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/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/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+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_CORRECT

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@shafik @jingham This is a polite Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 205093.
martong added a comment.

- Fix regression of TestFormatters.py


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -609,6 +609,8 @@
   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;
@@ -637,21 +639,22 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(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(decl_context)->setMustBuildLookupTable();
+  }
+
   return;
 }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/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/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+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",
+VARIABLE

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 200033.
martong added a comment.

- se -> so


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -637,18 +637,6 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(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);
 }
   }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/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/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+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)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set breakpoint 0 here.')
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
==

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 200031.
martong added a comment.

- Rebase to master
- Rebase to D62061 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -637,18 +637,6 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(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);
 }
   }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/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/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+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)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set breakpoint 0 here.')
Index: lldb/pack

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

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:922
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {

shafik wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

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




Comment at: clang/lib/AST/ASTImporter.cpp:1818
+  // The class will have DefinitionData, but no members.  Then,
+  // importDefinition is called from LLDB, which tries to get the members, se
+  // when we get here, the class already has the DefinitionData set, so we must

se -> so?



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:922
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping @shafik @teemperor


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c:8
 int b;
-} FILE;
+} MYFILE;
 

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.



Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:640
   }
-
-  DeclContext *decl_context_non_const =

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, 
llvm::SmallVectorImpl&) + 3917
11 
lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls(clang::DeclContext
 const*, llvm::function_ref, 
llvm::SmallVectorImpl&) + 102
12 clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext 
const*, llvm::SmallVectorImpl&) + 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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: shafik, teemperor, jingham, clayborg, a_sidorin.
Herald added subscribers: lldb-commits, cfe-commits, gamesh411, Szelethus, 
dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added projects: clang, LLDB.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Utility/Logging.h
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Utility/Logging.cpp

Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -918,6 +918,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+log_ast->Printf(" [ClangASTImporter][TUDecl: %p] Imported "
+"(%sDecl*)%p, named %s (from "
+"(Decl*)%p)",
+static_cast(to->getTranslationUnitDecl()),
+from->getDeclKindName(), static_cast(to),
+name_string.c_str(), static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+log_ast->Printf("%s", ast_string.c_str());
+  }
 }
   }
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -637,18 +637,6 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(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);
 }
   }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/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/packages/Python/lldbsuite/test/lang/c/ast/main.c
===