[lldb][Module] Only call LoadScriptingResourceInTarget via ModuleList

[Michael137]

This patch is motivated by #189943, where we would like to print the "these module scripts weren't loaded" warning for all modules batched together. I.e., we want to print the warning after all the script loading attempts, not from within each attempt.

To do so we want to hoist the ReportWarning calls in Module::LoadScriptingResourceInTarget out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use ModuleList::LoadScriptingResourceInTarget, which will be responsible for printing the warnings.

To avoid future accidental uses of Module::LoadScriptingResourceInTarget I moved the API into ModuleList and made it private.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch is motivated by #189943, where we would like to print the "these module scripts weren't loaded" warning for all modules batched together. I.e., we want to print the warning after all the script loading attempts, not from within each attempt.

To do so we want to hoist the ReportWarning calls in Module::LoadScriptingResourceInTarget out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use ModuleList::LoadScriptingResourceInTarget, which will be responsible for printing any warnings manually.

To avoid future accidental uses of Module::LoadScriptingResourceInTarget I moved the API into ModuleList and made it private.

---

Full diff: https://github.com/llvm/llvm-project/pull/190136.diff

6 Files Affected:

• (modified) lldb/include/lldb/Core/Module.h (-2)
• (modified) lldb/include/lldb/Core/ModuleList.h (+5)
• (modified) lldb/source/Commands/CommandObjectTarget.cpp (+4-10)
• (modified) lldb/source/Core/Module.cpp (-87)
• (modified) lldb/source/Core/ModuleList.cpp (+85-1)
• (modified) lldb/source/Target/Target.cpp (+5-14)

diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index cfe006629ae99..d33d1b1938ef4 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -510,8 +510,6 @@ class Module : public std::enable_shared_from_this<Module>,
   ///     \b true if it is, \b false otherwise.
   bool IsLoadedInTarget(Target *target);
 
-  bool LoadScriptingResourceInTarget(Target *target, Status &error);
-
   /// Get the number of compile units for this module.
   ///
   /// \return
diff --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h
index e147eeac61952..bea88a6cf1f51 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -559,6 +559,11 @@ class ModuleList {
   /// An orphaned module that lives only in the ModuleList has a count of 1.
   static constexpr long kUseCountModuleListOrphaned = 1;
 
+private:
+  static bool LoadScriptingResourceInTargetForModule(Module &module,
+                                                     Target &target,
+                                                     Status &error);
+
 public:
   typedef LockingAdaptedIterable<std::recursive_mutex, collection>
       ModuleIterable;
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index dccb7256b1cb7..288655f79f442 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -4400,16 +4400,10 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
 
           // Make sure we load any scripting resources that may be embedded
           // in the debug info files in case the platform supports that.
-          Status error;
-          module_sp->LoadScriptingResourceInTarget(target, error);
-          if (error.Fail() && error.AsCString())
-            result.AppendWarningWithFormat(
-                "unable to load scripting data for module %s - error "
-                "reported was %s",
-                module_sp->GetFileSpec()
-                    .GetFileNameStrippingExtension()
-                    .GetCString(),
-                error.AsCString());
+          std::list<Status> errors;
+          module_list.LoadScriptingResourcesInTarget(target, errors);
+          for (const auto &err : errors)
+            result.AppendWarning(err.AsCString());
 
           flush = true;
           result.SetStatus(eReturnStatusSuccessFinishResult);
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 808c8a347e9b2..439d53e4be4d2 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1419,93 +1419,6 @@ bool Module::IsLoadedInTarget(Target *target) {
   return false;
 }
 
-static bool LoadScriptingModule(const FileSpec &scripting_fspec,
-                                ScriptInterpreter &script_interpreter,
-                                Target &target, Status &error) {
-  assert(scripting_fspec);
-
-  StreamString scripting_stream;
-  scripting_fspec.Dump(scripting_stream.AsRawOstream());
-  LoadScriptOptions options;
-  return script_interpreter.LoadScriptingModule(
-      scripting_stream.GetData(), options, error,
-      /*module_sp*/ nullptr, /*extra_path*/ {}, target.shared_from_this());
-}
-
-bool Module::LoadScriptingResourceInTarget(Target *target, Status &error) {
-  Log *log = GetLog(LLDBLog::Modules);
-
-  if (!target) {
-    error = Status::FromErrorString("invalid destination Target");
-    return false;
-  }
-
-  Debugger &debugger = target->GetDebugger();
-  const ScriptLanguage script_language = debugger.GetScriptLanguage();
-  if (script_language == eScriptLanguageNone)
-    return true;
-
-  ScriptInterpreter *script_interpreter = debugger.GetScriptInterpreter();
-  if (!script_interpreter) {
-    error = Status::FromErrorString("invalid ScriptInterpreter");
-    return false;
-  }
-
-  PlatformSP platform_sp(target->GetPlatform());
-
-  if (!platform_sp) {
-    error = Status::FromErrorString("invalid Platform");
-    return false;
-  }
-
-  StreamString feedback_stream;
-  llvm::SmallDenseMap<FileSpec, LoadScriptFromSymFile> file_specs =
-      platform_sp->LocateExecutableScriptingResources(target, *this,
-                                                      feedback_stream);
-
-  if (!feedback_stream.Empty())
-    debugger.ReportWarning(feedback_stream.GetString().str(), debugger.GetID());
-
-  for (const auto &[scripting_fspec, load_style] : file_specs) {
-    if (load_style == eLoadScriptFromSymFileFalse)
-      continue;
-
-    if (!FileSystem::Instance().Exists(scripting_fspec))
-      continue;
-
-    if (load_style == eLoadScriptFromSymFileWarn) {
-      // clang-format off
-      debugger.ReportWarning(
-          llvm::formatv(
-R"('{0}' contains a debug script. To run this script in this debug session:
-
-    command script import "{1}"
-
-To run all discovered debug scripts in this session:
-
-    settings set target.load-script-from-symbol-file true
-)",
-              GetFileSpec().GetFileNameStrippingExtension(),
-              scripting_fspec.GetPath()),
-          debugger.GetID());
-      // clang-format on
-
-      return false;
-    }
-
-    LLDB_LOG(log, "Auto-loading {0}", scripting_fspec.GetPath());
-
-    if (!LoadScriptingModule(scripting_fspec, *script_interpreter, *target,
-                             error)) {
-      LLDB_LOG(log, "Failed to load '{0}'. Remaining scripts won't be loaded.",
-               scripting_fspec.GetPath());
-      return false;
-    }
-  }
-
-  return true;
-}
-
 bool Module::SetArchitecture(const ArchSpec &new_arch) {
   if (!m_arch.IsValid()) {
     m_arch = new_arch;
diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index b54d787de3512..b8980f6a02c50 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -1332,6 +1332,90 @@ bool ModuleList::RemoveSharedModuleIfOrphaned(const ModuleWP module_wp) {
   return GetSharedModuleList().RemoveIfOrphaned(module_wp);
 }
 
+static bool LoadScriptingModule(const FileSpec &scripting_fspec,
+                                ScriptInterpreter &script_interpreter,
+                                Target &target, Status &error) {
+  assert(scripting_fspec);
+
+  StreamString scripting_stream;
+  scripting_fspec.Dump(scripting_stream.AsRawOstream());
+  LoadScriptOptions options;
+  return script_interpreter.LoadScriptingModule(
+      scripting_stream.GetData(), options, error,
+      /*module_sp*/ nullptr, /*extra_path*/ {}, target.shared_from_this());
+}
+
+bool ModuleList::LoadScriptingResourceInTargetForModule(Module &module,
+                                                        Target &target,
+                                                        Status &error) {
+  Log *log = GetLog(LLDBLog::Modules);
+
+  Debugger &debugger = target.GetDebugger();
+  const ScriptLanguage script_language = debugger.GetScriptLanguage();
+  if (script_language == eScriptLanguageNone)
+    return true;
+
+  ScriptInterpreter *script_interpreter = debugger.GetScriptInterpreter();
+  if (!script_interpreter) {
+    error = Status::FromErrorString("invalid ScriptInterpreter");
+    return false;
+  }
+
+  PlatformSP platform_sp(target.GetPlatform());
+
+  if (!platform_sp) {
+    error = Status::FromErrorString("invalid Platform");
+    return false;
+  }
+
+  StreamString feedback_stream;
+  llvm::SmallDenseMap<FileSpec, LoadScriptFromSymFile> file_specs =
+      platform_sp->LocateExecutableScriptingResources(&target, module,
+                                                      feedback_stream);
+
+  if (!feedback_stream.Empty())
+    debugger.ReportWarning(feedback_stream.GetString().str(), debugger.GetID());
+
+  for (const auto &[scripting_fspec, load_style] : file_specs) {
+    if (load_style == eLoadScriptFromSymFileFalse)
+      continue;
+
+    if (!FileSystem::Instance().Exists(scripting_fspec))
+      continue;
+
+    if (load_style == eLoadScriptFromSymFileWarn) {
+      // clang-format off
+      debugger.ReportWarning(
+          llvm::formatv(
+R"('{0}' contains a debug script. To run this script in this debug session:
+
+    command script import "{1}"
+
+To run all discovered debug scripts in this session:
+
+    settings set target.load-script-from-symbol-file true
+)",
+              module.GetFileSpec().GetFileNameStrippingExtension(),
+              scripting_fspec.GetPath()),
+          debugger.GetID());
+      // clang-format on
+
+      return false;
+    }
+
+    LLDB_LOG(log, "Auto-loading {0}", scripting_fspec.GetPath());
+
+    if (!LoadScriptingModule(scripting_fspec, *script_interpreter, target,
+                             error)) {
+      LLDB_LOG(log, "Failed to load '{0}'. Remaining scripts won't be loaded.",
+               scripting_fspec.GetPath());
+      return false;
+    }
+  }
+
+  return true;
+}
+
 bool ModuleList::LoadScriptingResourcesInTarget(Target *target,
                                                 std::list<Status> &errors,
                                                 bool continue_on_error) {
@@ -1347,7 +1431,7 @@ bool ModuleList::LoadScriptingResourcesInTarget(Target *target,
   for (auto module : tmp_module_list.ModulesNoLocking()) {
     if (module) {
       Status error;
-      if (!module->LoadScriptingResourceInTarget(target, error)) {
+      if (!LoadScriptingResourceInTargetForModule(*module, *target, error)) {
         if (error.Fail() && error.AsCString()) {
           error = Status::FromErrorStringWithFormat(
               "unable to load scripting data for "
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 2a3832225f9b7..896425e42464d 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1542,19 +1542,6 @@ Module *Target::GetExecutableModulePointer() {
   return GetExecutableModule().get();
 }
 
-static void LoadScriptingResourceForModule(const ModuleSP &module_sp,
-                                           Target *target) {
-  Status error;
-  if (module_sp && !module_sp->LoadScriptingResourceInTarget(target, error)) {
-    if (error.AsCString())
-      target->GetDebugger().GetAsyncErrorStream()->Printf(
-          "unable to load scripting data for module %s - error reported was "
-          "%s\n",
-          module_sp->GetFileSpec().GetFileNameStrippingExtension().GetCString(),
-          error.AsCString());
-  }
-}
-
 void Target::ClearModules(bool delete_locations) {
   ModulesDidUnload(m_images, delete_locations);
   m_section_load_history.Clear();
@@ -1857,9 +1844,13 @@ void Target::ModulesDidLoad(ModuleList &module_list) {
 
   const size_t num_images = module_list.GetSize();
   if (m_valid && num_images) {
+    std::list<Status> errors;
+    module_list.LoadScriptingResourcesInTarget(this, errors);
+    for (const auto &err : errors)
+      GetDebugger().GetAsyncErrorStream()->PutCString(err.AsCString());
+
     for (size_t idx = 0; idx < num_images; ++idx) {
       ModuleSP module_sp(module_list.GetModuleAtIndex(idx));
-      LoadScriptingResourceForModule(module_sp, this);
       LoadTypeSummariesForModule(module_sp);
       LoadFormattersForModule(module_sp);
     }

[JDevlieghere]

A few small nits but overall LGTM

[JDevlieghere]

Suggested change

	/*module_sp*/ nullptr, /*extra_path*/ {}, target.shared_from_this());
	/*module_sp=*/nullptr, /*extra_path=*/{}, target.shared_from_this());

[Michael137]

Heh clang-format likes the space there.

See latest commit

[JDevlieghere]

Can this return an llvm::Error?

[Michael137]

That would be better, agreed. Though I'll do that in a separate PR because it's not as straightforward as hoped (mainly because the API we call also populate Status).

[JDevlieghere]

Suggested change

	PlatformSP platform_sp(target.GetPlatform());
	if (!platform_sp) {
	PlatformSP platform_sp = target.GetPlatform();
	if (!platform_sp) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.