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

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGef239972614c: [lldb] Remove FieldDecl stealing hack by rerouting indirect imports to the… (authored by teemperor).

Repository:
  rG LLVM Github Monorepo

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

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.237382.patch
Type: text/x-patch
Size: 3147 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200110/750665bc/attachment.bin>