[lldb] Remove ClangASTImporter reference from Target

[xiaobai]

Summary

Target is one of the classes responsible for vending ClangASTImporter.
Target doesn't need to know anything about ClangASTImporter, so if we
instead have ClangPersistentVariables vend it, we can preserve
existing behavior while improving layering and removing dependencies
from non-plugins to plugins.

[teemperor]

I wish we could do this without a global map. Also the ClangASTImporter shouldn't have a dependency on Target (I'm actually surprised this compiles without an additional include).

I'm not sure where the perfect place for the Target's ClangASTImporter is, but putting it in the ClangPersistentVariables would solve your problem of getting it out of Target and doesn't need a global map. Also in general the ClangASTImporter of the Target is used for copying stuff to the scratch context, so having it in the ClangPersistentVariables makes some sense.

lldb/include/lldb/Symbol/ClangASTImporter.h

328

unrelated change

[clayborg]

Is an AST importer specific to a target? Can we just put it into the Clang AST type system subclass and create it lazily?

[xiaobai]

In D72946#1827301, @teemperor wrote:

I wish we could do this without a global map. Also the ClangASTImporter shouldn't have a dependency on Target (I'm actually surprised this compiles without an additional include).

I'm not sure where the perfect place for the Target's ClangASTImporter is, but putting it in the ClangPersistentVariables would solve your problem of getting it out of Target and doesn't need a global map. Also in general the ClangASTImporter of the Target is used for copying stuff to the scratch context, so having it in the ClangPersistentVariables makes some sense.

Oh, yes, I'm not a fan of the global map either. I agree that ClangASTImporter shouldn't depend on Target but I think that's better than the other way around. I also agree that putting it into ClangPersistentVariables makes some sense. I'll give it a shot and update this when I get the chance. Thanks for pointing that out!

[xiaobai]

Implement @teemperor's suggestion

[teemperor]

In D72946#1827372, @clayborg wrote:

Is an AST importer specific to a target? Can we just put it into the Clang AST type system subclass and create it lazily?

If ClangASTImporter was stateless I would agree. But as the ClangASTImporter stores and hands out ASTImporters, it has a *lot* of state. So in theory we should only have one in LLDB to not have different state that goes out of sync. That doesn't fully work as the modules and the target are independent, so having 1(target) + N(modules) is the best we can do I think.

Anyway, the current approach moves it into the ClangPersistentVariables (which are part of the TypeSystemClangForExpressions which is the one for the target-unique scratch ASTContext) so this LGTM.

[teemperor]

lldb/include/lldb/Symbol/TypeSystemClang.h

38 ↗

(On Diff #240001)

I would also just let this be part of the normal include list without the empty line (or wherever clang format decides to place this).

lldb/source/Plugins/Language/CPlusPlus/BlockPointer.cpp

22

I would just keep this in the include list above and let clang-format do its thing.

[xiaobai]

Sort headers via clang-format

[xiaobai]

Commit c4f6fbe971351273b19a4a819bf6ceae2b70b37e

lldb/include/lldb/Symbol/ClangASTImporter.h

public:
   bool CompleteAndFetchChildren(clang::QualType type);
 
   bool RequireCompleteType(clang::QualType type);
 
   void SetDeclOrigin(const clang::Decl *decl, clang::Decl *original_decl);
 
   ClangASTMetadata *GetDeclMetadata(const clang::Decl *decl);
 
+  static lldb::ClangASTImporterSP Get(Target *target);
+
   //
   // Namespace maps
   //
 
   typedef std::vector<std::pair<lldb::ModuleSP, CompilerDeclContext>>
       NamespaceMap;
   typedef std::shared_ptr<NamespaceMap> NamespaceMapSP;
 
public:
 
   clang::FileManager m_file_manager;
   typedef llvm::DenseMap<const clang::RecordDecl *, LayoutInfo>
       RecordDeclToLayoutMap;
 
   RecordDeclToLayoutMap m_record_decl_to_layout_map;
 };
 
+
 } // namespace lldb_private
 
 #endif // liblldb_ClangASTImporter_h_

lldb/include/lldb/Target/Target.h

public:
   // parameters have the same meaning as for the UtilityFunction constructor.
   // Returns a new-ed object which the caller owns.
 
   UtilityFunction *GetUtilityFunctionForLanguage(const char *expr,
                                                  lldb::LanguageType language,
                                                  const char *name,
                                                  Status &error);
 
-  lldb::ClangASTImporterSP GetClangASTImporter();
-
   // Install any files through the platform that need be to installed prior to
   // launching or attaching.
   Status Install(ProcessLaunchInfo *launch_info);
 
   bool ResolveFileAddress(lldb::addr_t load_addr, Address &so_addr);
 
   bool ResolveLoadAddress(lldb::addr_t load_addr, Address &so_addr,
                           uint32_t stop_id = SectionLoadHistory::eStopIDNow);
protected:
   lldb::ProcessSP m_process_sp;
   lldb::SearchFilterSP m_search_filter_sp;
   PathMappingList m_image_search_paths;
   TypeSystemMap m_scratch_type_system_map;
 
   typedef std::map<lldb::LanguageType, lldb::REPLSP> REPLMap;
   REPLMap m_repl_map;
 
-  lldb::ClangASTImporterSP m_ast_importer_sp;
   lldb::ClangModulesDeclVendorUP m_clang_modules_decl_vendor_up;
 
   lldb::SourceManagerUP m_source_manager_up;
 
   typedef std::map<lldb::user_id_t, StopHookSP> StopHookCollection;
   StopHookCollection m_stop_hooks;
   lldb::user_id_t m_stop_hook_next_id;
   bool m_valid;

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

void ASTResultSynthesizer::CommitPersistentDecls() {
 
   auto *persistent_vars = llvm::cast<ClangPersistentVariables>(state);
   ClangASTContext *scratch_ctx = ClangASTContext::GetScratch(m_target);
 
   for (clang::NamedDecl *decl : m_decls) {
     StringRef name = decl->getName();
     ConstString name_cs(name.str().c_str());
 
-    Decl *D_scratch = m_target.GetClangASTImporter()->DeportDecl(
+    Decl *D_scratch = ClangASTImporter::Get(&m_target)->DeportDecl(
         &scratch_ctx->getASTContext(), decl);
 
     if (!D_scratch) {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
       if (log) {
         std::string s;
         llvm::raw_string_ostream ss(s);

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

 
 void ClangUserExpression::ClangUserExpressionHelper::ResetDeclMap(
     ExecutionContext &exe_ctx,
     Materializer::PersistentVariableDelegate &delegate,
     bool keep_result_in_memory,
     ValueObject *ctx_obj) {
   m_expr_decl_map_up.reset(new ClangExpressionDeclMap(
       keep_result_in_memory, &delegate, exe_ctx.GetTargetSP(),
-      exe_ctx.GetTargetRef().GetClangASTImporter(), ctx_obj));
+      ClangASTImporter::Get(exe_ctx.GetTargetPtr()), ctx_obj));
 }
 
 clang::ASTConsumer *
 ClangUserExpression::ClangUserExpressionHelper::ASTTransformer(
     clang::ASTConsumer *passthrough) {
   m_result_synthesizer_up.reset(
       new ASTResultSynthesizer(passthrough, m_top_level, m_target));
 

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

if (jit_error.Success()) {
     return false;
   }
 }
 
 void ClangUtilityFunction::ClangUtilityFunctionHelper::ResetDeclMap(
     ExecutionContext &exe_ctx, bool keep_result_in_memory) {
   m_expr_decl_map_up.reset(new ClangExpressionDeclMap(
       keep_result_in_memory, nullptr, exe_ctx.GetTargetSP(),
-      exe_ctx.GetTargetRef().GetClangASTImporter(), nullptr));
+      ClangASTImporter::Get(exe_ctx.GetTargetPtr()), nullptr));
 }

lldb/source/Plugins/Language/CPlusPlus/BlockPointer.cpp

 #include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/Target.h"
 
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
 namespace lldb_private {
 namespace formatters {
 
 class BlockPointerSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
 public:
BlockPointerSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
 
     ClangASTContext *clang_ast_context =
         llvm::dyn_cast<ClangASTContext>(&type_system_or_err.get());
 
     if (!clang_ast_context) {
       return;
     }
 
-    ClangASTImporterSP clang_ast_importer = target_sp->GetClangASTImporter();
+    ClangASTImporterSP clang_ast_importer =
+        ClangASTImporter::Get(target_sp.get());
 
     if (!clang_ast_importer) {
       return;
     }
 
     const char *const isa_name("__isa");
     const CompilerType isa_type =
         clang_ast_context->GetBasicType(lldb::eBasicTypeObjCClass);

lldb/source/Symbol/ClangASTContext.cpp

ClangASTContext::DeclContextGetClangASTContext(const CompilerDeclContext &dc) {
   return nullptr;
 }
 
 ClangASTContextForExpressions::ClangASTContextForExpressions(
     Target &target, llvm::Triple triple)
     : ClangASTContext(triple), m_target_wp(target.shared_from_this()),
       m_persistent_variables(new ClangPersistentVariables) {
   m_scratch_ast_source_up.reset(new ClangASTSource(
-      target.shared_from_this(), target.GetClangASTImporter()));
+      target.shared_from_this(), ClangASTImporter::Get(&target)));
   m_scratch_ast_source_up->InstallASTContext(*this);
   llvm::IntrusiveRefCntPtr<clang::ExternalASTSource> proxy_ast_source(
       m_scratch_ast_source_up->CreateProxy());
   SetExternalSource(proxy_ast_source);
 }
 
 void ClangASTContextForExpressions::Finalize() {
   ClangASTContext::Finalize();

lldb/source/Symbol/ClangASTImporter.cpp

 #include "clang/Sema/Sema.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <memory>
 
 using namespace lldb_private;
 using namespace clang;
 
+typedef std::map<Target *, lldb::ClangASTImporterSP> ClangASTImporterMap;
+
+static ClangASTImporterMap &GetImporterMap() {
+  static ClangASTImporterMap *g_map = nullptr;
+  static llvm::once_flag g_initialize;
+
+  llvm::call_once(g_initialize, [] { g_map = new ClangASTImporterMap(); });
+  return *g_map;
+}
+
+static std::mutex &GetImporterMapMutex() {
+  static std::mutex *g_mutex = nullptr;
+  static llvm::once_flag g_initialize;
+
+  llvm::call_once(g_initialize, [] { g_mutex = new std::mutex(); });
+
+  return *g_mutex;
+}
+
+lldb::ClangASTImporterSP ClangASTImporter::Get(Target *target) {
+  if (!target || !target->IsValid()) {
+    return lldb::ClangASTImporterSP();
+  }
+
+  std::lock_guard<std::mutex> guard(GetImporterMapMutex());
+  ClangASTImporterMap &map = GetImporterMap();
+  ClangASTImporterMap::iterator pos = map.find(target);
+  if (pos == map.end()) {
+    auto ast_importer_sp = std::make_shared<ClangASTImporter>();
+    map[target] = ast_importer_sp;
+    return ast_importer_sp;
+  }
+  return pos->second;
+}
+
 CompilerType ClangASTImporter::CopyType(ClangASTContext &dst_ast,
                                         const CompilerType &src_type) {
   clang::ASTContext &dst_clang_ast = dst_ast.getASTContext();
 
   ClangASTContext *src_ast =
       llvm::dyn_cast_or_null<ClangASTContext>(src_type.GetTypeSystem());
   if (!src_ast)
     return CompilerType();

lldb/source/Target/Target.cpp

 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionGroupWatchpoint.h"
 #include "lldb/Interpreter/OptionValues.h"
 #include "lldb/Interpreter/Property.h"
-#include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
Target::Target(Debugger &debugger, const ArchSpec &target_arch,
                const lldb::PlatformSP &platform_sp, bool is_dummy_target)
     : TargetProperties(this),
       Broadcaster(debugger.GetBroadcasterManager(),
                   Target::GetStaticBroadcasterClass().AsCString()),
       ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
       m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
       m_breakpoint_list(false), m_internal_breakpoint_list(true),
       m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
-      m_image_search_paths(ImageSearchPathsChanged, this), m_ast_importer_sp(),
+      m_image_search_paths(ImageSearchPathsChanged, this),
       m_source_manager_up(), m_stop_hooks(), m_stop_hook_next_id(0),
       m_valid(true), m_suppress_stop_hooks(false),
       m_is_dummy_target(is_dummy_target),
       m_stats_storage(static_cast<int>(StatisticKind::StatisticMax))
 
 {
   SetEventName(eBroadcastBitBreakpointChanged, "breakpoint-changed");
   SetEventName(eBroadcastBitModulesLoaded, "modules-loaded");
target->GetDebugger().GetErrorStream().Printf("%s\n",
                                                   feedback_stream.GetData());
 }
 
 void Target::ClearModules(bool delete_locations) {
   ModulesDidUnload(m_images, delete_locations);
   m_section_load_history.Clear();
   m_images.Clear();
   m_scratch_type_system_map.Clear();
-  m_ast_importer_sp.reset();
 }
 
 void Target::DidExec() {
   // When a process exec's we need to know about it so we can do some cleanup.
   m_breakpoint_list.RemoveInvalidLocations(m_arch.GetSpec());
   m_internal_breakpoint_list.RemoveInvalidLocations(m_arch.GetSpec());
 }
 
Target::GetUtilityFunctionForLanguage(const char *text,
   if (!utility_fn)
     error.SetErrorStringWithFormat(
         "Could not create an expression for language %s",
         Language::GetNameForLanguageType(language));
 
   return utility_fn;
 }
 
-ClangASTImporterSP Target::GetClangASTImporter() {
-  if (m_valid) {
-    if (!m_ast_importer_sp) {
-      m_ast_importer_sp = std::make_shared<ClangASTImporter>();
-    }
-    return m_ast_importer_sp;
-  }
-  return ClangASTImporterSP();
-}
-
 void Target::SettingsInitialize() { Process::SettingsInitialize(); }
 
 void Target::SettingsTerminate() { Process::SettingsTerminate(); }
 
 FileSpecList Target::GetDefaultExecutableSearchPaths() {
   TargetPropertiesSP properties_sp(Target::GetGlobalProperties());
   if (properties_sp)
     return properties_sp->GetExecutableSearchPaths();