llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> Remove the plugin loading logic in the debugger and replaces it remaining uses with the one in the PluginManager. Doing this per debugger instance was not only redundant, as the filesystem scan had already happened earlier, the plugins found this way were never initialized nor used. I've unified the plugin loading logic into a new PluginManager::LoadPlugin() helper method, that's now shared by both the PluginManager's file system scan and the `plugin load` command. --- PS: I'm somewhat skeptical that this dynamic loading of plugins is used given how broken some of its part appear. However, I need something similar, so I'm trying to make it better rather than removing it altogether. --- Patch is 21.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/182568.diff 16 Files Affected: - (modified) lldb/include/lldb/Core/Debugger.h (+1-6) - (modified) lldb/include/lldb/Core/PluginManager.h (+4) - (modified) lldb/include/lldb/lldb-private-types.h (-9) - (modified) lldb/source/API/SystemInitializerFull.cpp (+1-41) - (modified) lldb/source/Commands/CommandObjectPlugin.cpp (+3-6) - (modified) lldb/source/Core/Debugger.cpp (+2-89) - (modified) lldb/source/Core/PluginManager.cpp (+41-39) - (modified) lldb/tools/lldb-test/SystemInitializerTest.cpp (+1-1) - (modified) lldb/unittests/Callback/TestBreakpointSetCallback.cpp (+1-1) - (modified) lldb/unittests/Core/DebuggerTest.cpp (+1-1) - (modified) lldb/unittests/Core/DiagnosticEventTest.cpp (+1-1) - (modified) lldb/unittests/Core/ProgressReportTest.cpp (+1-1) - (modified) lldb/unittests/Process/ProcessEventDataTest.cpp (+2-2) - (modified) lldb/unittests/Process/elf-core/ThreadElfCoreTest.cpp (+1-1) - (modified) lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp (+1-2) - (modified) lldb/unittests/ValueObject/DynamicValueObjectLocalBuffer.cpp (-2) ``````````diff diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h index a38caa7ac594e..e7e0be966b9e0 100644 --- a/lldb/include/lldb/Core/Debugger.h +++ b/lldb/include/lldb/Core/Debugger.h @@ -104,7 +104,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>, static lldb::TargetSP FindTargetWithProcess(Process *process); - static void Initialize(LoadPluginCallbackType load_plugin_callback); + static void Initialize(); static void Terminate(); @@ -393,8 +393,6 @@ class Debugger : public std::enable_shared_from_this<Debugger>, bool SetShowInlineDiagnostics(bool); - bool LoadPlugin(const FileSpec &spec, Status &error); - void RunIOHandlers(); bool IsForwardingEvents(); @@ -777,9 +775,6 @@ class Debugger : public std::enable_shared_from_this<Debugger>, llvm::StringMap<std::weak_ptr<LogHandler>> m_stream_handlers; std::shared_ptr<CallbackLogHandler> m_callback_handler_sp; const std::string m_instance_name; - static LoadPluginCallbackType g_load_plugin_callback; - typedef std::vector<llvm::sys::DynamicLibrary> LoadedPluginsList; - LoadedPluginsList m_loaded_plugins; HostThread m_event_handler_thread; HostThread m_io_handler_thread; Broadcaster m_sync_broadcaster; ///< Private debugger synchronization. diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h index 26680bf95fae6..366072cba15b4 100644 --- a/lldb/include/lldb/Core/PluginManager.h +++ b/lldb/include/lldb/Core/PluginManager.h @@ -88,6 +88,10 @@ class PluginManager { static void Terminate(); + /// Load a plugin from a specific file path. Returns true if the plugin was + /// successfully loaded and initialized. + static bool LoadPlugin(const FileSpec &spec); + // Support for enabling and disabling plugins. // Return the plugins that can be enabled or disabled by the user. diff --git a/lldb/include/lldb/lldb-private-types.h b/lldb/include/lldb/lldb-private-types.h index 185467e91bf62..7c8672b63f17f 100644 --- a/lldb/include/lldb/lldb-private-types.h +++ b/lldb/include/lldb/lldb-private-types.h @@ -16,12 +16,6 @@ #include <type_traits> -namespace llvm { -namespace sys { -class DynamicLibrary; -} -} - namespace lldb_private { class Platform; class ExecutionContext; @@ -29,9 +23,6 @@ class RegisterFlags; typedef llvm::SmallString<256> PathSmallString; -typedef llvm::sys::DynamicLibrary (*LoadPluginCallbackType)( - const lldb::DebuggerSP &debugger_sp, const FileSpec &spec, Status &error); - /// Every register is described in detail including its name, alternate name /// (optional), encoding, size in bytes and the default display format. struct RegisterInfo { diff --git a/lldb/source/API/SystemInitializerFull.cpp b/lldb/source/API/SystemInitializerFull.cpp index 4cf7dd149e023..2f78037fdc94c 100644 --- a/lldb/source/API/SystemInitializerFull.cpp +++ b/lldb/source/API/SystemInitializerFull.cpp @@ -8,7 +8,6 @@ #include "SystemInitializerFull.h" #include "lldb/API/SBCommandInterpreter.h" -#include "lldb/API/SBDebugger.h" #include "lldb/Core/Debugger.h" #include "lldb/Core/PluginManager.h" #include "lldb/Core/Progress.h" @@ -87,46 +86,7 @@ llvm::Error SystemInitializerFull::Initialize() { LLDB_LOG(GetLog(SystemLog::System), "{0}", GetVersion()); - auto LoadPlugin = [](const lldb::DebuggerSP &debugger_sp, - const FileSpec &spec, - Status &error) -> llvm::sys::DynamicLibrary { - llvm::sys::DynamicLibrary dynlib = - llvm::sys::DynamicLibrary::getPermanentLibrary(spec.GetPath().c_str()); - if (dynlib.isValid()) { - typedef bool (*LLDBCommandPluginInit)(lldb::SBDebugger debugger); - - lldb::SBDebugger debugger_sb(debugger_sp); - // This calls the bool lldb::PluginInitialize(lldb::SBDebugger debugger) - // function. - // TODO: mangle this differently for your system - on OSX, the first - // underscore needs to be removed and the second one stays - LLDBCommandPluginInit init_func = - (LLDBCommandPluginInit)(uintptr_t)dynlib.getAddressOfSymbol( - "_ZN4lldb16PluginInitializeENS_10SBDebuggerE"); - if (init_func) { - if (init_func(debugger_sb)) - return dynlib; - else - error = Status::FromErrorString( - "plug-in refused to load " - "(lldb::PluginInitialize(lldb::SBDebugger) " - "returned false)"); - } else { - error = Status::FromErrorString( - "plug-in is missing the required initialization: " - "lldb::PluginInitialize(lldb::SBDebugger)"); - } - } else { - if (FileSystem::Instance().Exists(spec)) - error = Status::FromErrorString( - "this file does not represent a loadable dylib"); - else - error = Status::FromErrorString("no such file"); - } - return llvm::sys::DynamicLibrary(); - }; - - Debugger::Initialize(LoadPlugin); + Debugger::Initialize(); return llvm::Error::success(); } diff --git a/lldb/source/Commands/CommandObjectPlugin.cpp b/lldb/source/Commands/CommandObjectPlugin.cpp index 093ebde5f5f2c..cc8a6f120e7b9 100644 --- a/lldb/source/Commands/CommandObjectPlugin.cpp +++ b/lldb/source/Commands/CommandObjectPlugin.cpp @@ -35,16 +35,13 @@ class CommandObjectPluginLoad : public CommandObjectParsed { return; } - Status error; - FileSpec dylib_fspec(command[0].ref()); FileSystem::Instance().Resolve(dylib_fspec); - if (GetDebugger().LoadPlugin(dylib_fspec, error)) + if (PluginManager::LoadPlugin(dylib_fspec)) result.SetStatus(eReturnStatusSuccessFinishResult); - else { - result.AppendError(error.AsCString()); - } + else + result.AppendError("failed to load plugin"); } }; diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index 12f8039da947e..b268a000de019 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -230,8 +230,6 @@ TestingProperties &TestingProperties::GetGlobalTestingProperties() { } #endif -LoadPluginCallbackType Debugger::g_load_plugin_callback = nullptr; - Status Debugger::SetPropertyValue(const ExecutionContext *exe_ctx, VarSetOperationType op, llvm::StringRef property_path, @@ -748,13 +746,12 @@ bool Debugger::SetShowInlineDiagnostics(bool b) { //} // -void Debugger::Initialize(LoadPluginCallbackType load_plugin_callback) { +void Debugger::Initialize() { assert(g_debugger_list_ptr == nullptr && "Debugger::Initialize called more than once!"); g_debugger_list_mutex_ptr = new std::recursive_mutex(); g_debugger_list_ptr = new DebuggerList(); g_thread_pool = new llvm::DefaultThreadPool(llvm::optimal_concurrency()); - g_load_plugin_callback = load_plugin_callback; } void Debugger::Terminate() { @@ -787,91 +784,7 @@ void Debugger::SettingsInitialize() { Target::SettingsInitialize(); } void Debugger::SettingsTerminate() { Target::SettingsTerminate(); } -bool Debugger::LoadPlugin(const FileSpec &spec, Status &error) { - if (g_load_plugin_callback) { - llvm::sys::DynamicLibrary dynlib = - g_load_plugin_callback(shared_from_this(), spec, error); - if (dynlib.isValid()) { - m_loaded_plugins.push_back(dynlib); - return true; - } - } else { - // The g_load_plugin_callback is registered in SBDebugger::Initialize() and - // if the public API layer isn't available (code is linking against all of - // the internal LLDB static libraries), then we can't load plugins - error = Status::FromErrorString("Public API layer is not available"); - } - return false; -} - -static FileSystem::EnumerateDirectoryResult -LoadPluginCallback(void *baton, llvm::sys::fs::file_type ft, - llvm::StringRef path) { - Status error; - - static constexpr llvm::StringLiteral g_dylibext(".dylib"); - static constexpr llvm::StringLiteral g_solibext(".so"); - - if (!baton) - return FileSystem::eEnumerateDirectoryResultQuit; - - Debugger *debugger = (Debugger *)baton; - - namespace fs = llvm::sys::fs; - // If we have a regular file, a symbolic link or unknown file type, try and - // process the file. We must handle unknown as sometimes the directory - // enumeration might be enumerating a file system that doesn't have correct - // file type information. - if (ft == fs::file_type::regular_file || ft == fs::file_type::symlink_file || - ft == fs::file_type::type_unknown) { - FileSpec plugin_file_spec(path); - FileSystem::Instance().Resolve(plugin_file_spec); - - if (plugin_file_spec.GetFileNameExtension() != g_dylibext && - plugin_file_spec.GetFileNameExtension() != g_solibext) { - return FileSystem::eEnumerateDirectoryResultNext; - } - - Status plugin_load_error; - debugger->LoadPlugin(plugin_file_spec, plugin_load_error); - - return FileSystem::eEnumerateDirectoryResultNext; - } else if (ft == fs::file_type::directory_file || - ft == fs::file_type::symlink_file || - ft == fs::file_type::type_unknown) { - // Try and recurse into anything that a directory or symbolic link. We must - // also do this for unknown as sometimes the directory enumeration might be - // enumerating a file system that doesn't have correct file type - // information. - return FileSystem::eEnumerateDirectoryResultEnter; - } - - return FileSystem::eEnumerateDirectoryResultNext; -} - void Debugger::InstanceInitialize() { - const bool find_directories = true; - const bool find_files = true; - const bool find_other = true; - char dir_path[PATH_MAX]; - if (FileSpec dir_spec = HostInfo::GetSystemPluginDir()) { - if (FileSystem::Instance().Exists(dir_spec) && - dir_spec.GetPath(dir_path, sizeof(dir_path))) { - FileSystem::Instance().EnumerateDirectory(dir_path, find_directories, - find_files, find_other, - LoadPluginCallback, this); - } - } - - if (FileSpec dir_spec = HostInfo::GetUserPluginDir()) { - if (FileSystem::Instance().Exists(dir_spec) && - dir_spec.GetPath(dir_path, sizeof(dir_path))) { - FileSystem::Instance().EnumerateDirectory(dir_path, find_directories, - find_files, find_other, - LoadPluginCallback, this); - } - } - PluginManager::DebuggerInitialize(*this); } @@ -1016,7 +929,7 @@ Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton) std::make_unique<CommandInterpreter>(*this, false)), m_io_handler_stack(), m_instance_name(llvm::formatv("debugger_{0}", GetID()).str()), - m_loaded_plugins(), m_event_handler_thread(), m_io_handler_thread(), + m_event_handler_thread(), m_io_handler_thread(), m_sync_broadcaster(nullptr, "lldb.debugger.sync"), m_broadcaster(m_broadcaster_manager_sp, GetStaticBroadcasterClass().str()), diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index c99157692aa45..fb065aa571d11 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -80,8 +80,6 @@ template <typename FPtrTy> static FPtrTy CastToFPtr(void *VPtr) { static FileSystem::EnumerateDirectoryResult LoadPluginCallback(void *baton, llvm::sys::fs::file_type ft, llvm::StringRef path) { - Status error; - namespace fs = llvm::sys::fs; // If we have a regular file, a symbolic link or unknown file type, try and // process the file. We must handle unknown as sometimes the directory @@ -91,43 +89,8 @@ LoadPluginCallback(void *baton, llvm::sys::fs::file_type ft, ft == fs::file_type::type_unknown) { FileSpec plugin_file_spec(path); FileSystem::Instance().Resolve(plugin_file_spec); - - if (PluginIsLoaded(plugin_file_spec)) - return FileSystem::eEnumerateDirectoryResultNext; - else { - PluginInfo plugin_info; - - std::string pluginLoadError; - plugin_info.library = llvm::sys::DynamicLibrary::getPermanentLibrary( - plugin_file_spec.GetPath().c_str(), &pluginLoadError); - if (plugin_info.library.isValid()) { - bool success = false; - plugin_info.plugin_init_callback = CastToFPtr<PluginInitCallback>( - plugin_info.library.getAddressOfSymbol("LLDBPluginInitialize")); - if (plugin_info.plugin_init_callback) { - // Call the plug-in "bool LLDBPluginInitialize(void)" function - success = plugin_info.plugin_init_callback(); - } - - if (success) { - // It is ok for the "LLDBPluginTerminate" symbol to be nullptr - plugin_info.plugin_term_callback = CastToFPtr<PluginTermCallback>( - plugin_info.library.getAddressOfSymbol("LLDBPluginTerminate")); - } else { - // The initialize function returned FALSE which means the plug-in - // might not be compatible, or might be too new or too old, or might - // not want to run on this machine. Set it to a default-constructed - // instance to invalidate it. - plugin_info = PluginInfo(); - } - - // Regardless of success or failure, cache the plug-in load in our - // plug-in info so we don't try to load it again and again. - SetPluginInfo(plugin_file_spec, plugin_info); - - return FileSystem::eEnumerateDirectoryResultNext; - } - } + PluginManager::LoadPlugin(plugin_file_spec); + return FileSystem::eEnumerateDirectoryResultNext; } if (ft == fs::file_type::directory_file || @@ -142,6 +105,45 @@ LoadPluginCallback(void *baton, llvm::sys::fs::file_type ft, return FileSystem::eEnumerateDirectoryResultNext; } +bool PluginManager::LoadPlugin(const FileSpec &plugin_file_spec) { + if (PluginIsLoaded(plugin_file_spec)) + return false; + + PluginInfo plugin_info; + + std::string pluginLoadError; + plugin_info.library = llvm::sys::DynamicLibrary::getPermanentLibrary( + plugin_file_spec.GetPath().c_str(), &pluginLoadError); + if (plugin_info.library.isValid()) { + bool success = false; + plugin_info.plugin_init_callback = CastToFPtr<PluginInitCallback>( + plugin_info.library.getAddressOfSymbol("LLDBPluginInitialize")); + if (plugin_info.plugin_init_callback) { + // Call the plug-in "bool LLDBPluginInitialize(void)" function + success = plugin_info.plugin_init_callback(); + } + + if (success) { + // It is ok for the "LLDBPluginTerminate" symbol to be nullptr + plugin_info.plugin_term_callback = CastToFPtr<PluginTermCallback>( + plugin_info.library.getAddressOfSymbol("LLDBPluginTerminate")); + } else { + // The initialize function returned FALSE which means the plug-in + // might not be compatible, or might be too new or too old, or might + // not want to run on this machine. Set it to a default-constructed + // instance to invalidate it. + plugin_info = PluginInfo(); + } + + // Regardless of success or failure, cache the plug-in load in our + // plug-in info so we don't try to load it again and again. + SetPluginInfo(plugin_file_spec, plugin_info); + + return success; + } + return false; +} + void PluginManager::Initialize() { const bool find_directories = true; const bool find_files = true; diff --git a/lldb/tools/lldb-test/SystemInitializerTest.cpp b/lldb/tools/lldb-test/SystemInitializerTest.cpp index 3478e5d8df994..924e3cd97166a 100644 --- a/lldb/tools/lldb-test/SystemInitializerTest.cpp +++ b/lldb/tools/lldb-test/SystemInitializerTest.cpp @@ -51,7 +51,7 @@ llvm::Error SystemInitializerTest::Initialize() { // Settings must be initialized AFTER PluginManager::Initialize is called. Debugger::SettingsInitialize(); - Debugger::Initialize(nullptr); + Debugger::Initialize(); return llvm::Error::success(); } diff --git a/lldb/unittests/Callback/TestBreakpointSetCallback.cpp b/lldb/unittests/Callback/TestBreakpointSetCallback.cpp index c29cac5d2e5e7..e4ea57f450b8d 100644 --- a/lldb/unittests/Callback/TestBreakpointSetCallback.cpp +++ b/lldb/unittests/Callback/TestBreakpointSetCallback.cpp @@ -47,7 +47,7 @@ class BreakpointSetCallbackTest : public ::testing::Test { protected: void SetUp() override { std::call_once(TestUtilities::g_debugger_initialize_flag, - []() { Debugger::Initialize(nullptr); }); + []() { Debugger::Initialize(); }); }; DebuggerSP m_debugger_sp; diff --git a/lldb/unittests/Core/DebuggerTest.cpp b/lldb/unittests/Core/DebuggerTest.cpp index 4dccd912c63ae..22c611eee8410 100644 --- a/lldb/unittests/Core/DebuggerTest.cpp +++ b/lldb/unittests/Core/DebuggerTest.cpp @@ -25,7 +25,7 @@ class DebuggerTest : public ::testing::Test { HostInfo::Initialize(); PlatformMacOSX::Initialize(); std::call_once(TestUtilities::g_debugger_initialize_flag, - []() { Debugger::Initialize(nullptr); }); + []() { Debugger::Initialize(); }); ArchSpec arch("x86_64-apple-macosx-"); Platform::SetHostPlatform( PlatformRemoteMacOSX::CreateInstance(true, &arch)); diff --git a/lldb/unittests/Core/DiagnosticEventTest.cpp b/lldb/unittests/Core/DiagnosticEventTest.cpp index 82e96c4abec83..c226d909921a0 100644 --- a/lldb/unittests/Core/DiagnosticEventTest.cpp +++ b/lldb/unittests/Core/DiagnosticEventTest.cpp @@ -34,7 +34,7 @@ class DiagnosticEventTest : public ::testing::Test { HostInfo::Initialize(); PlatformMacOSX::Initialize(); std::call_once(TestUtilities::g_debugger_initialize_flag, - []() { Debugger::Initialize(nullptr); }); + []() { Debugger::Initialize(); }); ArchSpec arch("x86_64-apple-macosx-"); Platform::SetHostPlatform( PlatformRemoteMacOSX::CreateInstance(true, &arch)); diff --git a/lldb/unittests/Core/ProgressReportTest.cpp b/lldb/unittests/Core/ProgressReportTest.cpp index 592e13a79e8d7..da58a25a51be1 100644 --- a/lldb/unittests/Core/ProgressReportTest.cpp +++ b/lldb/unittests/Core/ProgressReportTest.cpp @@ -52,7 +52,7 @@ class ProgressReportTest : public ::testing::Test { // here the usual way. void SetUp() override { std::call_once(TestUtilities::g_debugger_initialize_flag, - []() { Debugger::Initialize(nullptr); }); + []() { Debugger::Initialize(); }); }; DebuggerSP m_debugger_sp; diff --git a/lldb/unittests/Process/ProcessEventDataTest.cpp b/lldb/unittests/Process/ProcessEventDataTest.cpp index 88ea394bbb1e5..c51ec3e5b09f3 100644 --- a/lldb/unittests/Process/ProcessEventDataTest.cpp +++ b/lldb/unittests/Process/ProcessEventDataTest.cpp @@ -30,7 +30,7 @@ class ProcessEventDataTest : public ::testing::Test { HostInfo::Initialize(); PlatformMacOSX::Initialize(); std::call_once(TestUtilities::g_debugger_initialize_flag, - []() { Debugger::Initialize(nullptr); }); + []() { Debugger::Initialize(); }); } void TearDown() override { PlatformMacOSX::Terminate(); @@ -187,7 +187,7 @@ TEST_F(ProcessEventDataTest, DoOnRemoval) { ->m_should_stop_hit_count == 0; ASSERT_TRUE(result); } -#endif +#endif TEST_F(ProcessEventDataTest, ShouldStop) { ArchSpec arch("x86_64-apple-macosx-"); diff --git a/lldb/unittests/Process/elf-core/ThreadElfCoreTest.cpp b/lldb/unittests/Process/elf-core/ThreadElfCoreTest.cpp index 68919945198d4..aaefae770a970 100644 --- a/lldb/unittests/Process/elf-core/ThreadElfCoreTest.cpp +++ b/lldb/unittests/Process/elf-core/ThreadElfCoreTest.cpp @@ -39,7 +39,7 @@ struct ElfCoreTest : public testing::Test { HostInfo::Initialize(); platform_linux::PlatformLinux::Initialize(); std::call_once(TestUtilities::g_debugger_initialize_flag, - []() { Debugger::Initialize(nullptr); }); + []() { Debugger::Initialize(); }); } static void TearDownTestCase() { platform_linux::PlatformLinux::Terminate(); diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp index 6a753b6b33edf..586ead69ff137 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.c... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/182568 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
