[lldb] Create a ASTImporterSharedState and pass it to all ASTImporterDelegates

[teemperor]

Summary

This patch is just creating an ASTImporterSharedState for each ASTContext we import into
and then pass it to all the different ASTImporter instances that import into that context.

See the ASTImporterSharedState documentation for the benefits of using the shared state.

[teemperor]

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging? Given that we most likely never want this to happen within LLDB, this seems like a straightforward change.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

[martong]

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h

269

If you want to get rid of noload_lookup in clang::ASTImporter then you have to pass the TranslationUnitDecl to the SharedState's constructor because only in that case would we create the lookup table.

[martong]

In D92848#2442609, @martong wrote:

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

Hmm, as far as I remember, in the past I tried this. And bumped into some issues, because during the traverse we had to load Decls with external storage and that introduced a new and uncontrolled import...

[teemperor]

In D92848#2442613, @martong wrote:

In D92848#2442609, @martong wrote:

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

Hmm, as far as I remember, in the past I tried this. And bumped into some issues, because during the traverse we had to load Decls with external storage and that introduced a new and uncontrolled import...

Ah, I assumed the default-constructed SharedState would have an empty table, but it has no table. That explains why this works with little modifications...

Anyway, I'm not sure if the workflow of first scanning the TU for all decls makes sense in LLDB. The idea is that we build the TU in the first place with the help of an ExternalSource and the ASTImporter, so once we have an actual TU to scan, we no longer use the ASTImporter.

However, every single TU in LLDB starts being pretty much empty (we do have some dummy declarations in them, but none of them have any name conflicts with declarations in any other AST we import from). So technically the 'empty' LookupTable is accurate.

[martong]

However, every single TU in LLDB starts being pretty much empty (we do have some dummy declarations in them, but none of them have any name conflicts with declarations in any other AST we import from). So technically the 'empty' LookupTable is accurate.

Okay, that makes sense. So, we could then try to build an empty lookup table in the default constructor of SharedState. This way we could get rid of noload_lookup entirely. And that would indeed result what you suggested at the beginning of our conversation (i.e. the ASTImporter would no longer consider non-imported declarations in the To context for merging). This could work.

[teemperor]

There area few failures once I actually create the lookup table, so I put this on my TODO list.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h

 #define LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_CLANGASTIMPORTER_H
 
 #include <map>
 #include <memory>
 #include <set>
 #include <vector>
 
 #include "clang/AST/ASTImporter.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/CompilerDeclContext.h"
public:
   /// certain interval.
   ///
   /// When intercepting a declaration import, the ASTImporterDelegate uses the
   /// CxxModuleHandler to replace any missing or malformed declarations with
   /// their counterpart from a C++ module.
   struct ASTImporterDelegate : public clang::ASTImporter {
     ASTImporterDelegate(ClangASTImporter &master, clang::ASTContext *target_ctx,
                         clang::ASTContext *source_ctx)
-        : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
-                             master.m_file_manager, true /*minimal*/),
+        : clang::ASTImporter(
+              *target_ctx, master.m_file_manager, *source_ctx,
+              master.m_file_manager, true /*minimal*/,
+              master.GetContextMetadata(target_ctx)->m_shared_state),
           m_master(master), m_source_ctx(source_ctx) {
       setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
     }
 
     /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
     /// and deattaches it at the end of the scope. Supports being used multiple
     /// times on the same ASTImporterDelegate instance in nested scopes.
     class CxxModuleScope {
private:
     /// The currently attached listener.
     NewDeclListener *m_new_decl_listener = nullptr;
   };
 
   typedef std::shared_ptr<ASTImporterDelegate> ImporterDelegateSP;
   typedef llvm::DenseMap<clang::ASTContext *, ImporterDelegateSP> DelegateMap;
   typedef llvm::DenseMap<const clang::NamespaceDecl *, NamespaceMapSP>
       NamespaceMetaMap;
+  typedef std::shared_ptr<clang::ASTImporterSharedState> SharedStateSP;
 
   class ASTContextMetadata {
     typedef llvm::DenseMap<const clang::Decl *, DeclOrigin> OriginMap;
 
   public:
-    ASTContextMetadata(clang::ASTContext *dst_ctx) : m_dst_ctx(dst_ctx) {}
+    ASTContextMetadata(clang::ASTContext *dst_ctx) : m_dst_ctx(dst_ctx) {
+      m_shared_state = std::make_shared<clang::ASTImporterSharedState>();
+    }
 
     clang::ASTContext *m_dst_ctx;
     DelegateMap m_delegates;
 
     NamespaceMetaMap m_namespace_maps;
     MapCompleter *m_map_completer = nullptr;
+    /// The shared state that should be used for all the ASTImporters that
+    /// import into the destination ASTContext.
+    SharedStateSP m_shared_state;
 
     /// Sets the DeclOrigin for the given Decl and overwrites any existing
     /// DeclOrigin.
     void setOrigin(const clang::Decl *decl, DeclOrigin origin) {
       m_origins[decl] = origin;
     }
 
     /// Removes any tracked DeclOrigin for the given decl.