[Lldb-commits] [PATCH] D72507: [lldb] Remove FieldDecl stealing hack by rerouting indirect imports to the original AST

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added reviewers: shafik, aprantl.
Herald added subscribers: lldb-commits, JDevlieghere.
Herald added a project: LLDB.
teemperor added a comment.

FWIW, this isn't really a finished patch yet as I actually wanted to write several unit tests for this, but as it fixes the gmodules failures on master I thought I might as well put it up now.


This is a port of D67803 <https://reviews.llvm.org/D67803> of preventing indirect importing to our scratch context when evaluating expressions.

D67803 <https://reviews.llvm.org/D67803> already has a pretty long explanation of how this works, but the idea is that instead
of importing declarations indirectly over the expression AST (i.e., Debug info AST -> Expression AST -> scratch AST)
we instead directly import the declaration from the debug info AST to the scratch AST.

The difference from D67803 <https://reviews.llvm.org/D67803> is that here we have to do this in the ASTImporterDelegate (which is our ASTImporter
subclass we use in LLDB). It has the same information as the ExternalASTMerger in D67803 <https://reviews.llvm.org/D67803> as it can access the
ClangASTImporter (which also keeps track of where Decls originally came from).

With this patch we can also delete the FieldDecl stealing hack in the ClangASTSource (this was only necessary as the
indirect imports caused the creation of duplicate Record declarations but we needed the fields in the Record decl
we originally found in the scratch ASTContext).

This also fixes the current gmodules failures where we fail to find std::vector fields after an indirect import
over the expression AST (where it seems even our FieldDecl stealing hack can't save us from).


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D72507

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Symbol/ClangASTImporter.cpp


Index: lldb/source/Symbol/ClangASTImporter.cpp
===================================================================
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -872,6 +872,39 @@
     }
   }
 
+  // Check which ASTContext this declaration originally came from.
+  DeclOrigin origin = m_master.GetDeclOrigin(From);
+  // If it originally came from the target ASTContext then we can just
+  // pretend that the original is the one we imported. This can happen for
+  // example when inspecting a persistent declaration from the scratch
+  // ASTContext (which will provide the declaration when parsing the
+  // expression and then we later try to copy the declaration back to the
+  // scratch ASTContext to store the result).
+  // Without this check we would ask the ASTImporter to import a declaration
+  // into the same ASTContext where it came from (which doesn't make a lot of
+  // sense).
+  if (origin.Valid() && origin.ctx == &getToContext()) {
+      RegisterImportedDecl(From, origin.decl);
+      return origin.decl;
+  }
+
+  // This declaration came originally from another ASTContext. Instead of
+  // copying our potentially incomplete 'From' Decl we instead go to the
+  // original ASTContext and copy the original to the target. This is not
+  // only faster than first completing our current decl and then copying it
+  // to the target, but it also prevents that indirectly copying the same
+  // declaration to the same target requires the ASTImporter to merge all
+  // the different decls that appear to come from different ASTContexts (even
+  // though all these different source ASTContexts just got a copy from
+  // one source AST).
+  if (origin.Valid()) {
+    auto R = m_master.CopyDecl(&getToContext(), origin.decl);
+    if (R) {
+      RegisterImportedDecl(From, R);
+      return R;
+    }
+  }
+
   return ASTImporter::ImportImpl(From);
 }
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -530,20 +530,6 @@
 
         m_ast_importer_sp->RequireCompleteType(copied_field_type);
       }
-      auto decl_context_non_const = const_cast<DeclContext *>(decl_context);
-
-      // The decl ended up in the wrong DeclContext. Let's fix that so
-      // the decl we copied will actually be found.
-      // FIXME: This is a horrible hack that shouldn't be necessary. However
-      // it seems our current setup sometimes fails to copy decls to the right
-      // place. See rdar://55129537.
-      if (copied_decl->getDeclContext() != decl_context) {
-        assert(copied_decl->getDeclContext()->containsDecl(copied_decl));
-        copied_decl->getDeclContext()->removeDecl(copied_decl);
-        copied_decl->setDeclContext(decl_context_non_const);
-        assert(!decl_context_non_const->containsDecl(copied_decl));
-        decl_context_non_const->addDeclInternal(copied_decl);
-      }
     } else {
       SkippedDecls = true;
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72507.237312.patch
Type: text/x-patch
Size: 3147 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200110/8d803028/attachment-0001.bin>