[lldb] Add setting to specify (by name) which module's scripting resources can be auto-loaded - #188722
Conversation
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis is part of this RFC which is about turning the libc++ data-formatters into auto-loadable Python scripts. Eventually we want the Python data-formatters for This patch adds a setting ( Making this a setting also means a user can disable any auto-loading by clearing it. By default the setting is currently empty. Eventually we'll want it to contain AI Usage:
Patch is 27.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/188722.diff 11 Files Affected:
diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h
index 3d0776d95b539..7a20008634b28 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -275,9 +275,13 @@ class Platform : public PluginInterface {
///
/// Locating the file should happen only on the local computer or using the
/// current computers global settings.
- FileSpecList LocateExecutableScriptingResources(Target *target,
- Module &module,
- Stream &feedback_stream);
+ ///
+ /// Returns a pair of \c FileSpecList. The first element contains
+ /// scripts that are eligible to be auto-loaded. The second element
+ /// contains the non-auto loadable scripts.
+ std::pair<FileSpecList, FileSpecList>
+ LocateExecutableScriptingResources(Target *target, Module &module,
+ Stream &feedback_stream);
/// Locate the platform-specific scripting resource given a module
/// specification.
@@ -289,10 +293,16 @@ class Platform : public PluginInterface {
/// which gathers FileSpecs for executable scripts from
/// pre-configured "safe" auto-load paths.
///
+ /// Returns a pair of \c FileSpecList. The first element contains
+ /// scripts that are eligible to be auto-loaded. The second element
+ /// contains the non-auto loadable scripts.
+ ///
/// E.g., for Python it will look for a script at:
/// \c <safe-path>/<module-name>/<module-name>.py
- static FileSpecList LocateExecutableScriptingResourcesFromSafePaths(
- Stream &feedback_stream, FileSpec module_spec, const Target &target);
+ static std::pair<FileSpecList, FileSpecList>
+ LocateExecutableScriptingResourcesFromSafePaths(Stream &feedback_stream,
+ FileSpec module_spec,
+ const Target &target);
/// \param[in] module_spec
/// The ModuleSpec of a binary to find.
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 7907ed2f1a5f6..a9f602bb7873f 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -278,6 +278,10 @@ class TargetProperties : public Properties {
bool GetDebugUtilityExpression() const;
+ OptionValueDictionary *GetAutoLoadModules() const;
+
+ void SetAutoLoadModule(llvm::StringRef module_name, bool should_load);
+
private:
std::optional<bool>
GetExperimentalPropertyValue(size_t prop_idx,
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 3d33a5011fb5e..7f8192a5bff42 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1442,9 +1442,6 @@ bool Module::LoadScriptingResourceInTarget(Target *target, Status &error) {
LoadScriptFromSymFile should_load =
target->TargetProperties::GetLoadScriptFromSymbolFile();
- if (should_load == eLoadScriptFromSymFileFalse)
- return false;
-
Debugger &debugger = target->GetDebugger();
const ScriptLanguage script_language = debugger.GetScriptLanguage();
if (script_language == eScriptLanguageNone)
@@ -1464,18 +1461,28 @@ bool Module::LoadScriptingResourceInTarget(Target *target, Status &error) {
}
StreamString feedback_stream;
- FileSpecList file_specs = platform_sp->LocateExecutableScriptingResources(
- target, *this, feedback_stream);
+ const auto [auto_load_files, non_auto_load_files] =
+ platform_sp->LocateExecutableScriptingResources(target, *this,
+ feedback_stream);
if (!feedback_stream.Empty())
debugger.ReportWarning(feedback_stream.GetString().str(), debugger.GetID());
- const uint32_t num_specs = file_specs.GetSize();
- if (num_specs == 0)
- return true;
+ for (uint32_t i = 0; i < auto_load_files.GetSize(); ++i) {
+ FileSpec scripting_fspec(auto_load_files.GetFileSpecAtIndex(i));
+ if (!FileSystem::Instance().Exists(scripting_fspec))
+ continue;
+
+ if (!LoadScriptingModule(scripting_fspec, *script_interpreter, *target,
+ error))
+ return false;
+ }
+
+ if (should_load == eLoadScriptFromSymFileFalse)
+ return !auto_load_files.IsEmpty();
- for (uint32_t i = 0; i < num_specs; ++i) {
- FileSpec scripting_fspec(file_specs.GetFileSpecAtIndex(i));
+ for (uint32_t i = 0; i < non_auto_load_files.GetSize(); ++i) {
+ FileSpec scripting_fspec(non_auto_load_files.GetFileSpecAtIndex(i));
if (!FileSystem::Instance().Exists(scripting_fspec))
continue;
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 51ef8d7259d9a..db560769c1e34 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -24,6 +24,7 @@
#include "lldb/Host/Host.h"
#include "lldb/Host/HostInfo.h"
#include "lldb/Host/OptionParser.h"
+#include "lldb/Interpreter/OptionValueDictionary.h"
#include "lldb/Interpreter/OptionValueFileSpec.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Interpreter/Property.h"
@@ -157,7 +158,37 @@ Status Platform::GetFileWithUUID(const FileSpec &platform_file,
return Status();
}
-FileSpecList Platform::LocateExecutableScriptingResourcesFromSafePaths(
+// FIXME: move this into lldb-private-enumerations.h?
+enum class OptionalBool {
+ eYes,
+ eNo,
+ eDontKnow,
+};
+
+/// Returns \c OptionalBool::eYes if scripting resources associated with the
+/// specified module \c FileSpec can be automatically loaded. If a module is
+/// explicitly disallowed from being auto-loaded, returns \c OptionalBool::eNo.
+/// In all other cases, returns \c OptionalBool::eDontKnow.
+static OptionalBool CanAutoLoadModule(const FileSpec &module_fspec,
+ const Target &target) {
+ OptionValueDictionary *names = target.GetAutoLoadModules();
+ if (!names)
+ return OptionalBool::eDontKnow;
+
+ OptionValueSP value_sp =
+ names->GetValueForKey(module_fspec.GetFileNameStrippingExtension());
+ if (!value_sp)
+ return OptionalBool::eDontKnow;
+
+ auto maybe_can_load = value_sp->GetValueAs<bool>();
+ if (!maybe_can_load)
+ return OptionalBool::eDontKnow;
+
+ return *maybe_can_load ? OptionalBool::eYes : OptionalBool::eNo;
+}
+
+std::pair<FileSpecList, FileSpecList>
+Platform::LocateExecutableScriptingResourcesFromSafePaths(
Stream &feedback_stream, FileSpec module_spec, const Target &target) {
assert(module_spec);
assert(target.GetDebugger().GetScriptInterpreter());
@@ -172,7 +203,8 @@ FileSpecList Platform::LocateExecutableScriptingResourcesFromSafePaths(
->GetSanitizedScriptingModuleName(
module_spec.GetFileNameStrippingExtension().GetStringRef());
- FileSpecList file_list;
+ FileSpecList non_auto_load_files;
+ FileSpecList auto_load_files;
FileSpecList paths = Debugger::GetSafeAutoLoadPaths();
// Iterate in reverse so we consider the latest appended path first.
@@ -196,15 +228,20 @@ FileSpecList Platform::LocateExecutableScriptingResourcesFromSafePaths(
WarnIfInvalidUnsanitizedScriptExists(feedback_stream, sanitized_name,
orig_script_fspec, script_fspec);
- if (FileSystem::Instance().Exists(script_fspec))
- file_list.Append(script_fspec);
+ if (FileSystem::Instance().Exists(script_fspec)) {
+ OptionalBool can_auto_load = CanAutoLoadModule(module_spec, target);
+ if (can_auto_load == OptionalBool::eYes)
+ auto_load_files.Append(script_fspec);
+ else if (can_auto_load == OptionalBool::eDontKnow)
+ non_auto_load_files.Append(script_fspec);
+ }
// If we successfully found a directory in a safe auto-load path
// stop looking at any other paths.
break;
}
- return file_list;
+ return {auto_load_files, non_auto_load_files};
}
FileSpecList Platform::LocateExecutableScriptingResourcesForPlatform(
@@ -212,7 +249,7 @@ FileSpecList Platform::LocateExecutableScriptingResourcesForPlatform(
return {};
}
-FileSpecList
+std::pair<FileSpecList, FileSpecList>
Platform::LocateExecutableScriptingResources(Target *target, Module &module,
Stream &feedback_stream) {
if (!target)
@@ -222,7 +259,7 @@ Platform::LocateExecutableScriptingResources(Target *target, Module &module,
if (FileSpecList fspecs = LocateExecutableScriptingResourcesForPlatform(
target, module, feedback_stream);
!fspecs.IsEmpty())
- return fspecs;
+ return {{}, fspecs};
const FileSpec &module_spec = module.GetFileSpec();
if (!module_spec)
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 126a2b57ed4b4..cd0fe4e3eea58 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -5266,6 +5266,20 @@ void TargetProperties::SetDebugUtilityExpression(bool debug) {
SetPropertyAtIndex(idx, debug);
}
+OptionValueDictionary *TargetProperties::GetAutoLoadModules() const {
+ return m_collection_sp->GetPropertyAtIndexAsOptionValueDictionary(
+ ePropertyAutoLoadModules);
+}
+
+void TargetProperties::SetAutoLoadModule(llvm::StringRef module_name,
+ bool should_load) {
+ OptionValueDictionary *dict = GetAutoLoadModules();
+ if (!dict)
+ return;
+ dict->SetValueForKey(module_name,
+ std::make_shared<OptionValueBoolean>(should_load));
+}
+
// Target::TargetEventData
Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp)
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 2361314d506ac..d8ef0df70e7f8 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -220,6 +220,11 @@ let Definition = "target", Path = "target" in {
def ParallelModuleLoad: Property<"parallel-module-load", "Boolean">,
DefaultTrue,
Desc<"Enable loading of modules in parallel for the dynamic loader.">;
+ def AutoLoadModules
+ : Property<"auto-load-modules", "Dictionary">,
+ ElementType<"Boolean">,
+ Desc<"A list of module names and whether LLDB will auto-load scripting "
+ "resources for it from safe paths.">;
}
let Definition = "process_experimental", Path = "target.process.experimental" in {
diff --git a/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-false.test b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-false.test
new file mode 100644
index 0000000000000..2902df479979c
--- /dev/null
+++ b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-false.test
@@ -0,0 +1,25 @@
+# REQUIRES: python, asserts, !system-windows
+
+# Test that when a module is listed in target.auto-load-modules with 'false',
+# its scripting resources are NOT loaded even when target.load-script-from-symbol-file
+# is true.
+
+# RUN: split-file %s %t
+# RUN: %clang_host %t/main.c -o %t/TestModule.out
+# RUN: mkdir -p %t/safe-path/TestModule
+
+# RUN: cp %t/script.py %t/safe-path/TestModule/TestModule.py
+# RUN: %lldb -b \
+# RUN: -o 'settings set target.load-script-from-symbol-file true' \
+# RUN: -o 'settings append testing.safe-auto-load-paths %t/safe-path' \
+# RUN: -o 'settings set target.auto-load-modules TestModule=false' \
+# RUN: -o 'target create %t/TestModule.out' 2>&1 \
+# RUN: | FileCheck %s --implicit-check-not=AUTOLOAD_SUCCESS --implicit-check-not=warning
+
+#--- main.c
+int main() { return 0; }
+
+#--- script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("AUTOLOAD_SUCCESS", file=sys.stderr)
diff --git a/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-multiple.test b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-multiple.test
new file mode 100644
index 0000000000000..a9de987017641
--- /dev/null
+++ b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-multiple.test
@@ -0,0 +1,38 @@
+# REQUIRES: python, asserts, !system-windows
+
+# Test that multiple modules listed in target.auto-load-modules are all
+# auto-loaded.
+
+# RUN: split-file %s %t
+# RUN: %clang_host -shared %t/lib.c -o %t/libFoo.dylib
+# RUN: %clang_host %t/main.c -o %t/TestModule.out %t/libFoo.dylib
+# RUN: mkdir -p %t/safe-path/TestModule
+# RUN: mkdir -p %t/safe-path/libFoo
+
+# RUN: cp %t/main_script.py %t/safe-path/TestModule/TestModule.py
+# RUN: cp %t/lib_script.py %t/safe-path/libFoo/libFoo.py
+# RUN: %lldb -b \
+# RUN: -o 'settings set target.load-script-from-symbol-file false' \
+# RUN: -o 'settings append testing.safe-auto-load-paths %t/safe-path' \
+# RUN: -o 'settings set target.auto-load-modules TestModule=true libFoo=true' \
+# RUN: -o 'target create %t/TestModule.out' 2>&1 | FileCheck %s
+
+# CHECK-DAG: MAIN_AUTOLOAD_SUCCESS
+# CHECK-DAG: LIB_AUTOLOAD_SUCCESS
+
+#--- main.c
+extern int foo(void);
+int main() { return foo(); }
+
+#--- lib.c
+int foo(void) { return 0; }
+
+#--- main_script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("MAIN_AUTOLOAD_SUCCESS", file=sys.stderr)
+
+#--- lib_script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("LIB_AUTOLOAD_SUCCESS", file=sys.stderr)
diff --git a/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-not-in-dict.test b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-not-in-dict.test
new file mode 100644
index 0000000000000..cf9a80121c17a
--- /dev/null
+++ b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-not-in-dict.test
@@ -0,0 +1,29 @@
+# REQUIRES: python, asserts, !system-windows
+
+# Test that when a module is NOT in target.auto-load-modules, the existing
+# target.load-script-from-symbol-file setting controls whether scripts load.
+# With load-script-from-symbol-file=true and no dictionary entry, scripts
+# should still load normally.
+
+# RUN: split-file %s %t
+# RUN: %clang_host %t/main.c -o %t/TestModule.out
+# RUN: mkdir -p %t/safe-path/TestModule
+
+# RUN: cp %t/script.py %t/safe-path/TestModule/TestModule.py
+
+## A different module is in the dictionary; TestModule is not.
+# RUN: %lldb -b \
+# RUN: -o 'settings set target.load-script-from-symbol-file warn' \
+# RUN: -o 'settings append testing.safe-auto-load-paths %t/safe-path' \
+# RUN: -o 'settings set target.auto-load-modules SomeOtherModule=true' \
+# RUN: -o 'target create %t/TestModule.out' 2>&1 | FileCheck %s --implicit-check-not=AUTOLOAD_SUCCESS
+
+# CHECK: warning: 'TestModule' contains a debug script. To run this script in this debug session
+
+#--- main.c
+int main() { return 0; }
+
+#--- script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("AUTOLOAD_SUCCESS", file=sys.stderr)
diff --git a/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-true.test b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-true.test
new file mode 100644
index 0000000000000..87b09248b22f6
--- /dev/null
+++ b/lldb/test/Shell/Platform/AutoLoad/UNIX/auto-load-modules-true.test
@@ -0,0 +1,26 @@
+# REQUIRES: python, asserts, !system-windows
+
+# Test that when a module is listed in target.auto-load-modules with 'true',
+# its scripting resources are loaded even when target.load-script-from-symbol-file
+# is false.
+
+# RUN: split-file %s %t
+# RUN: %clang_host %t/main.c -o %t/TestModule.out
+# RUN: mkdir -p %t/safe-path/TestModule
+
+# RUN: cp %t/script.py %t/safe-path/TestModule/TestModule.py
+# RUN: %lldb -b \
+# RUN: -o 'settings set target.load-script-from-symbol-file false' \
+# RUN: -o 'settings append testing.safe-auto-load-paths %t/safe-path' \
+# RUN: -o 'settings set target.auto-load-modules TestModule=true' \
+# RUN: -o 'target create %t/TestModule.out' 2>&1 | FileCheck %s
+
+# CHECK: AUTOLOAD_SUCCESS
+
+#--- main.c
+int main() { return 0; }
+
+#--- script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("AUTOLOAD_SUCCESS", file=sys.stderr)
diff --git a/lldb/unittests/Platform/PlatformTest.cpp b/lldb/unittests/Platform/PlatformTest.cpp
index 3f46353e1bcb8..a703c8ed09f15 100644
--- a/lldb/unittests/Platform/PlatformTest.cpp
+++ b/lldb/unittests/Platform/PlatformTest.cpp
@@ -225,10 +225,11 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("TestModule.py", module_dir);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
ASSERT_EQ(file_specs.GetSize(), 0u);
}
@@ -252,10 +253,11 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("TestModule1.py", module_dir);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
ASSERT_EQ(file_specs.GetSize(), 0u);
}
@@ -281,10 +283,11 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("not_a_script.txt", module_dir);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
EXPECT_EQ(file_specs.GetSize(), 1u);
EXPECT_EQ(file_specs.GetFileSpecAtIndex(0).GetFilename(), "TestModule.py");
}
@@ -313,10 +316,11 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("TestModule.py", nested_dir);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
EXPECT_EQ(file_specs.GetSize(), 0u);
}
@@ -373,10 +377,12 @@ TEST_F(PlatformLocateSafePathTest,
FileSpec(path2));
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
+
// path1 was the last appended path with a matching directory.
EXPECT_EQ(file_specs.GetSize(), 1u);
EXPECT_TRUE(llvm::StringRef(file_specs.GetFileSpecAtIndex(0).GetPath())
@@ -391,7 +397,8 @@ TEST_F(PlatformLocateSafePathTest,
FileSpec(path3));
file_specs = Platform::LocateExecutableScriptingResourcesFromSafePaths(
- ss, module_fspec, *m_target_sp);
+ ss, module_fspec, *m_target_sp)
+ .second;
EXPECT_EQ(file_specs.GetSize(), 0u);
@@ -399,7 +406,8 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("TestModule.py", path3_module_dir);
file_specs = Platform::LocateExecutableScriptingResourcesFromSafePaths(
- ss, module_fspec, *m_target_sp);
+ ss, module_fspec, *m_target_sp)
+ .second;
EXPECT_EQ(file_specs.GetSize(), 1u);
EXPECT_TRUE(llvm::StringRef(file_specs.GetFileSpecAtIndex(0).GetPath())
@@ -428,10 +436,11 @@ TEST_F(PlatformLocateSafePathTest,
ASSERT_TRUE(orig_fspec);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
EXPECT_EQ(file_specs.GetSize(), 0u);
std::string expected = llvm::formatv(
@@ -465,10 +474,11 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("TestModule_1_1_1.py", module_dir);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
EXPECT_EQ(file_specs.GetSize(), 1u);
EXPECT_EQ(file_specs.GetFileSpecAtIndex(0).GetFilename(),
"TestModule_1_1_1.py");
@@ -500,10 +510,11 @@ TEST_F(PlatformLocateSafePathTest,
CreateFile("TestModule_1_1_1.py", module_dir);
StreamString ss;
- FileSpecList file_specs =
+ auto [auto_load_spces, file_specs] =
Platform::LocateExecutableScriptingResourcesFromSafePaths(
ss, module_fspec, *m_target_sp);
+ EXPECT_EQ(auto_load_spces.GetSize(), 0u);
EXPECT_EQ(file_specs.GetSize(), 1u);
...
[truncated]
|
|
Based on the description:
|
Yup that's the correct interaction. I agree this is getting a bit subtle, so I'll document it.
Agreed
Are you suggesting we make |
+1 |
|
|
||
| OptionValueDictionary *GetAutoLoadScriptsForModules() const; | ||
|
|
||
| void SetAutoLoadScriptsForModules(llvm::StringRef module_name, |
There was a problem hiding this comment.
This only sets the flag for a single module right ?
| void SetAutoLoadScriptsForModules(llvm::StringRef module_name, | |
| void SetAutoLoadScriptsForModule(llvm::StringRef module_name, |
JDevlieghere
left a comment
There was a problem hiding this comment.
The PR is almost what I had in mind. The only difference is that I was expecting the value to be the LoadScriptFromSymFile enum rather than a boolean. The idea being that we can keep adding different ways to determine what to load by default, and users can always rely on this new to specify overrides. I created #189444 so we have a concrete example of what I have in mind for that.
5e3d181 to
8a0efe9
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
…89696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for #188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In #188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
…ileSpec (#189696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm/llvm-project#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm/llvm-project#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
252e8a3 to
3c3e0fd
Compare
c6ec216 to
d8de450
Compare
…vm#189696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
e8e21a9 to
7e01930
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…urces can be auto-loaded This is part of [this RFC](https://discourse.llvm.org/t/rfc-lldb-moving-libc-data-formatters-out-of-lldb/89591) which is about turning the libc++ data-formatters into auto-loadable Python scripts. Eventually we want the Python data-formatters for `libc++` to be automatically loaded without requiring user opt-in (since that's how the builtin formatters have always worked and, in my opinion, we can't transition to an opt-in model if users have always had the data-formatters available). To do so we need a way to distinguish which modules we can *always* auto-load from safe-paths, and which require `target.load-script-from-symbol-file` to be set to `true`. This patch adds a setting (`target.auto-load-modules`) that is a dictionary from module-name to a boolean indicating whether the scripts for that module can be automatically loaded. Making this a setting also means a user can disable any auto-loading by clearing it. By default the setting is currently empty. Eventually we'll want it to contain `libc++.1=true` (and possibly other names which the `libc++` dylib can commonly have). **AI Usage**: * Used Claude to generate the unit-test cases and shell tests. Reviewed and cleaned them up myself.
945ab73 to
2c838b6
Compare
|
|
||
| LoadScriptFromSymFile GetLoadScriptFromSymbolFile() const; | ||
|
|
||
| void SetLoadScriptFromSymbolFile(LoadScriptFromSymFile load_style); |
There was a problem hiding this comment.
This is the global target-wide setting...
There was a problem hiding this comment.
Yup, i needed that for one of the unit-tests
| bool GetDebugUtilityExpression() const; | ||
|
|
||
| std::optional<LoadScriptFromSymFile> | ||
| GetAutoLoadScriptsForModule(llvm::StringRef module_name) const; |
There was a problem hiding this comment.
and these are fine-grained per-module ones?
There was a problem hiding this comment.
Correct. I'll add some comments
Co-authored-by: Adrian Prantl <adrian.prantl@gmail.com>
…vm#189696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script. (cherry picked from commit 89dec12)
…urces can be auto-loaded (llvm#188722) * Depends on: llvm#189696 This is part of [this RFC](https://discourse.llvm.org/t/rfc-lldb-moving-libc-data-formatters-out-of-lldb/89591) which is about turning the libc++ data-formatters into auto-loadable Python scripts. Eventually we want the Python data-formatters for `libc++` to be automatically loaded without requiring user opt-in (since that's how the builtin formatters have always worked and, in my opinion, we can't transition to an opt-in model if users have always had the data-formatters available). To do so we need a way to distinguish which modules we can *always* auto-load from safe-paths, and which require `target.load-script-from-symbol-file` to be set to `true`. This patch adds a setting (`target.auto-load-modules`) that is a dictionary from module-name to the `LoadScriptFromSymFile` enum, indicating how the scripts for that module are to be loaded. Making this a setting also means a user can disable any auto-loading by clearing it. By default the setting is currently empty. Eventually we'll want it to contain `libc++.1=true` (and possibly other names which the `libc++` dylib can commonly have). **Considerations**: * I thought about adding an `IsAutoLoadable` API to `FileSpec` and set it for those that can be auto-loaded. Then we wouldn't have to return two lists from `LocateExecutableScriptingResources`. I just felt like `FileSpec` was too high-level of a structure to hold such information so I opted for the `llvm::SmallDenseMap<FileSpec, LoadScriptFromSymFile>`. If people favour one approach over the other, I'm happy to reconsider **AI Usage**: * Used Claude to generate the unit-test cases and shell tests. Reviewed and cleaned them up myself. --------- Co-authored-by: Adrian Prantl <adrian.prantl@gmail.com> (cherry picked from commit e03b731)
…ileSpec (#189696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm/llvm-project#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm/llvm-project#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
…urces can be auto-loaded (llvm#188722) * Depends on: llvm#189696 This is part of [this RFC](https://discourse.llvm.org/t/rfc-lldb-moving-libc-data-formatters-out-of-lldb/89591) which is about turning the libc++ data-formatters into auto-loadable Python scripts. Eventually we want the Python data-formatters for `libc++` to be automatically loaded without requiring user opt-in (since that's how the builtin formatters have always worked and, in my opinion, we can't transition to an opt-in model if users have always had the data-formatters available). To do so we need a way to distinguish which modules we can *always* auto-load from safe-paths, and which require `target.load-script-from-symbol-file` to be set to `true`. This patch adds a setting (`target.auto-load-modules`) that is a dictionary from module-name to the `LoadScriptFromSymFile` enum, indicating how the scripts for that module are to be loaded. Making this a setting also means a user can disable any auto-loading by clearing it. By default the setting is currently empty. Eventually we'll want it to contain `libc++.1=true` (and possibly other names which the `libc++` dylib can commonly have). **Considerations**: * I thought about adding an `IsAutoLoadable` API to `FileSpec` and set it for those that can be auto-loaded. Then we wouldn't have to return two lists from `LocateExecutableScriptingResources`. I just felt like `FileSpec` was too high-level of a structure to hold such information so I opted for the `llvm::SmallDenseMap<FileSpec, LoadScriptFromSymFile>`. If people favour one approach over the other, I'm happy to reconsider **AI Usage**: * Used Claude to generate the unit-test cases and shell tests. Reviewed and cleaned them up myself. --------- Co-authored-by: Adrian Prantl <adrian.prantl@gmail.com>
…ileSpec (#189696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm/llvm-project#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm/llvm-project#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
…vm#189696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
…urces can be auto-loaded (llvm#188722) * Depends on: llvm#189696 This is part of [this RFC](https://discourse.llvm.org/t/rfc-lldb-moving-libc-data-formatters-out-of-lldb/89591) which is about turning the libc++ data-formatters into auto-loadable Python scripts. Eventually we want the Python data-formatters for `libc++` to be automatically loaded without requiring user opt-in (since that's how the builtin formatters have always worked and, in my opinion, we can't transition to an opt-in model if users have always had the data-formatters available). To do so we need a way to distinguish which modules we can *always* auto-load from safe-paths, and which require `target.load-script-from-symbol-file` to be set to `true`. This patch adds a setting (`target.auto-load-modules`) that is a dictionary from module-name to the `LoadScriptFromSymFile` enum, indicating how the scripts for that module are to be loaded. Making this a setting also means a user can disable any auto-loading by clearing it. By default the setting is currently empty. Eventually we'll want it to contain `libc++.1=true` (and possibly other names which the `libc++` dylib can commonly have). **Considerations**: * I thought about adding an `IsAutoLoadable` API to `FileSpec` and set it for those that can be auto-loaded. Then we wouldn't have to return two lists from `LocateExecutableScriptingResources`. I just felt like `FileSpec` was too high-level of a structure to hold such information so I opted for the `llvm::SmallDenseMap<FileSpec, LoadScriptFromSymFile>`. If people favour one approach over the other, I'm happy to reconsider **AI Usage**: * Used Claude to generate the unit-test cases and shell tests. Reviewed and cleaned them up myself. --------- Co-authored-by: Adrian Prantl <adrian.prantl@gmail.com>
…89696) This patch changes the `Platform::LocateXXX` to return a map from `FileSpec` to `LoadScriptFromSymFile` enum. This is needed for llvm/llvm-project#188722, where I intend to set `LoadScriptFromSymFile` per-module. By default the `Platform::LocateXXX` set the value to whatever the target's current `target.load-script-from-symbol-file` is set to. In llvm/llvm-project#188722 we'll allow overriding this per-target setting on a per-module basis. Drive-by: * Added logging when we fail to load a script.
This is part of this RFC which is about turning the libc++ data-formatters into auto-loadable Python scripts.
Eventually we want the Python data-formatters for
libc++to be automatically loaded without requiring user opt-in (since that's how the builtin formatters have always worked and, in my opinion, we can't transition to an opt-in model if users have always had the data-formatters available). To do so we need a way to distinguish which modules we can always auto-load from safe-paths, and which requiretarget.load-script-from-symbol-fileto be set totrue.This patch adds a setting (
target.auto-load-modules) that is a dictionary from module-name to theLoadScriptFromSymFileenum, indicating how the scripts for that module are to be loaded.Making this a setting also means a user can disable any auto-loading by clearing it. By default the setting is currently empty. Eventually we'll want it to contain
libc++.1=true(and possibly other names which thelibc++dylib can commonly have).Considerations:
IsAutoLoadableAPI toFileSpecand set it for those that can be auto-loaded. Then we wouldn't have to return two lists fromLocateExecutableScriptingResources. I just felt likeFileSpecwas too high-level of a structure to hold such information so I opted for thellvm::SmallDenseMap<FileSpec, LoadScriptFromSymFile>. If people favour one approach over the other, I'm happy to reconsiderAI Usage: