[lldb] Clear ModuleList shared modules in SBDebugger::Clear #147289
Conversation
|
@llvm/pr-subscribers-lldb Author: Andrew Savonichev (asavonic) ChangesShared modules are stored in a global However, when lldb is used as a library, we need a way to manage opened modules to avoid problems with file locks (on some systems) for modules that we no longer need. It should be possible to record all loaded modules and use The patch adds the following:
This new behaviour is turned off by default: the debugger does not release these shared modules until the process exits. Full diff: https://github.com/llvm/llvm-project/pull/147289.diff 10 Files Affected:
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 192fbee9c0c6d..6a2f76f2d5685 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -319,6 +319,8 @@ class LLDB_API SBDebugger {
bool SetShowInlineDiagnostics(bool);
+ bool SetClearSharedModules(bool);
+
bool SetUseSourceCache(bool use_source_cache);
bool GetUseSourceCache() const;
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 504f936fe317a..4cf7fc75bafd4 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -373,6 +373,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
bool SetShowInlineDiagnostics(bool);
+ bool GetClearSharedModules() const;
+
+ bool SetClearSharedModules(bool);
+
bool LoadPlugin(const FileSpec &spec, Status &error);
void RunIOHandlers();
diff --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h
index 909ee08f9ba62..587843dd05a4d 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -482,6 +482,9 @@ class ModuleList {
static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
+ /// Empty global cache of modules to release memory, file locks, etc.
+ static void ClearSharedModules();
+
/// Applies 'callback' to each module in this ModuleList.
/// If 'callback' returns false, iteration terminates.
/// The 'module_sp' passed to 'callback' is guaranteed to
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 603e306497841..221c02cfe66ed 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1466,6 +1466,12 @@ bool SBDebugger::SetShowInlineDiagnostics(bool value) {
return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
}
+bool SBDebugger::SetClearSharedModules(bool value) {
+ LLDB_INSTRUMENT_VA(this, value);
+
+ return (m_opaque_sp ? m_opaque_sp->SetClearSharedModules(value) : false);
+}
+
bool SBDebugger::SetUseSourceCache(bool value) {
LLDB_INSTRUMENT_VA(this, value);
diff --git a/lldb/source/Core/CoreProperties.td b/lldb/source/Core/CoreProperties.td
index 53dd333f045c9..1a6ba1a9af84e 100644
--- a/lldb/source/Core/CoreProperties.td
+++ b/lldb/source/Core/CoreProperties.td
@@ -268,4 +268,8 @@ let Definition = "debugger" in {
Global,
DefaultFalse,
Desc<"Controls whether diagnostics can refer directly to the command input, drawing arrows to it. If false, diagnostics will echo the input.">;
+ def ClearSharedModules: Property<"clear-shared-modules", "Boolean">,
+ Global,
+ DefaultFalse,
+ Desc<"Controls whether the debugger clears internal shared modules as it exits.">;
}
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ed674ee1275c7..fbd2f37960e19 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -700,6 +700,17 @@ bool Debugger::SetShowInlineDiagnostics(bool b) {
return SetPropertyAtIndex(idx, b);
}
+bool Debugger::GetClearSharedModules() const {
+ const uint32_t idx = ePropertyClearSharedModules;
+ return GetPropertyAtIndexAs<bool>(
+ idx, g_debugger_properties[idx].default_uint_value);
+}
+
+bool Debugger::SetClearSharedModules(bool b) {
+ const uint32_t idx = ePropertyClearSharedModules;
+ return SetPropertyAtIndex(idx, b);
+}
+
#pragma mark Debugger
// const DebuggerPropertiesSP &
@@ -1092,6 +1103,8 @@ void Debugger::Clear() {
StopIOHandlerThread();
StopEventHandlerThread();
m_listener_sp->Clear();
+ if (GetClearSharedModules())
+ ModuleList::ClearSharedModules();
for (TargetSP target_sp : m_target_list.Targets()) {
if (target_sp) {
if (ProcessSP process_sp = target_sp->GetProcessSP())
diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index d5ddf6e846112..81a63b7ec541e 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -1040,6 +1040,13 @@ bool ModuleList::RemoveSharedModuleIfOrphaned(const Module *module_ptr) {
return GetSharedModuleList().RemoveIfOrphaned(module_ptr);
}
+void ModuleList::ClearSharedModules() {
+ GetSharedModuleList().Clear();
+ Log *log = GetLog(LLDBLog::Modules);
+ if (log != nullptr)
+ LLDB_LOGF(log, "cleared shared modules");
+}
+
bool ModuleList::LoadScriptingResourcesInTarget(Target *target,
std::list<Status> &errors,
Stream &feedback_stream,
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index 43f45f330ee2a..7ff88eb252967 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -294,3 +294,34 @@ def test_version(self):
self.assertEqual(instance_str, class_str)
self.assertEqual(class_str, property_str)
+
+ def test_default_SetClearSharedModules(self):
+ """Check that that SBDebugger does not clear shared modules by
+ default.
+ """
+ messages = list()
+ self.dbg.SetLoggingCallback(messages.append)
+ self.runCmd("log enable lldb module")
+ self.dbg.Destroy(self.dbg)
+ self.assertFalse(any("cleared shared modules cache" in msg for msg in messages))
+
+ def test_enable_SetClearSharedModules(self):
+ """Check that SetClearSharedModule(true) clears shared module cache.
+ """
+ messages = list()
+ self.dbg.SetLoggingCallback(messages.append)
+ self.dbg.SetClearSharedModules(True)
+ self.runCmd("log enable lldb module")
+ self.dbg.Destroy(self.dbg)
+ self.assertTrue(any("cleared shared modules" in msg for msg in messages))
+
+ def test_enable_clear_shared_modules(self):
+ """Check that clear-shared-module setting is equivalent
+ to SetClearSharedModules(true).
+ """
+ messages = list()
+ self.dbg.SetLoggingCallback(messages.append)
+ self.runCmd("settings set clear-shared-modules true")
+ self.runCmd("log enable lldb module")
+ self.dbg.Destroy(self.dbg)
+ self.assertTrue(any("cleared shared modules" in msg for msg in messages))
diff --git a/lldb/unittests/Target/CMakeLists.txt b/lldb/unittests/Target/CMakeLists.txt
index 3169339ec699f..aeb552e22ac3f 100644
--- a/lldb/unittests/Target/CMakeLists.txt
+++ b/lldb/unittests/Target/CMakeLists.txt
@@ -12,6 +12,7 @@ add_lldb_unittest(TargetTests
RemoteAwarePlatformTest.cpp
StackFrameRecognizerTest.cpp
SummaryStatisticsTest.cpp
+ SharedModuleListTest.cpp
FindFileTest.cpp
LINK_COMPONENTS
diff --git a/lldb/unittests/Target/SharedModuleListTest.cpp b/lldb/unittests/Target/SharedModuleListTest.cpp
new file mode 100644
index 0000000000000..837f934f439c3
--- /dev/null
+++ b/lldb/unittests/Target/SharedModuleListTest.cpp
@@ -0,0 +1,209 @@
+//===---------- SharedModuleList.cpp --------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h"
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "Plugins/Platform/Android/PlatformAndroid.h"
+#include "Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h"
+#include "Plugins/SymbolFile/Symtab/SymbolFileSymtab.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Target.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::platform_android;
+using namespace lldb_private::platform_linux;
+using namespace lldb_private::breakpad;
+using namespace testing;
+
+namespace {
+
+constexpr llvm::StringLiteral k_process_plugin("mock-process-plugin");
+constexpr llvm::StringLiteral k_platform_dir("remote-android");
+constexpr llvm::StringLiteral k_cache_dir(".cache");
+constexpr llvm::StringLiteral k_module_file("AndroidModule.so");
+constexpr llvm::StringLiteral k_arch("aarch64-none-linux");
+constexpr llvm::StringLiteral
+ k_module_uuid("80008338-82A0-51E5-5922-C905D23890DA-BDDEFECC");
+const size_t k_module_size = 3784;
+
+FileSpec GetTestDir() {
+ const auto *info = UnitTest::GetInstance()->current_test_info();
+ FileSpec test_dir = HostInfo::GetProcessTempDir();
+ test_dir.AppendPathComponent(std::string(info->test_case_name()) + "-" +
+ info->name());
+ std::error_code ec = llvm::sys::fs::create_directory(test_dir.GetPath());
+ EXPECT_FALSE(ec);
+ return test_dir;
+}
+
+FileSpec GetRemotePath() {
+ FileSpec fs("/", FileSpec::Style::posix);
+ fs.AppendPathComponent("bin");
+ fs.AppendPathComponent(k_module_file);
+ return fs;
+}
+
+FileSpec GetUuidView(FileSpec spec) {
+ spec.AppendPathComponent(k_platform_dir);
+ spec.AppendPathComponent(k_cache_dir);
+ spec.AppendPathComponent(k_module_uuid);
+ spec.AppendPathComponent(k_module_file);
+ return spec;
+}
+
+FileSpec BuildCacheDir(const FileSpec &test_dir) {
+ FileSpec uuid_view = GetUuidView(test_dir);
+ std::error_code ec =
+ llvm::sys::fs::create_directories(uuid_view.GetDirectory().GetCString());
+ EXPECT_FALSE(ec);
+ ec = llvm::sys::fs::copy_file(GetInputFilePath(k_module_file),
+ uuid_view.GetPath().c_str());
+ EXPECT_FALSE(ec);
+ return uuid_view;
+}
+
+ModuleSpec GetTestModuleSpec() {
+ ModuleSpec module_spec(GetRemotePath(), ArchSpec(k_arch));
+ module_spec.GetUUID().SetFromStringRef(k_module_uuid);
+ module_spec.SetObjectSize(k_module_size);
+ return module_spec;
+}
+
+void CheckModule(const ModuleSP &module_sp) {
+ ASSERT_TRUE(module_sp);
+ ASSERT_EQ(module_sp->GetUUID().GetAsString(), k_module_uuid);
+ ASSERT_EQ(module_sp->GetObjectOffset(), 0U);
+ ASSERT_EQ(module_sp->GetPlatformFileSpec(), GetRemotePath());
+}
+
+class MockProcess : public Process {
+public:
+ MockProcess(TargetSP target_sp, ListenerSP listener_sp)
+ : Process(target_sp, listener_sp) {}
+
+ llvm::StringRef GetPluginName() override { return k_process_plugin; };
+
+ bool CanDebug(TargetSP target, bool plugin_specified_by_name) override {
+ return true;
+ }
+
+ Status DoDestroy() override { return Status(); }
+
+ void RefreshStateAfterStop() override {}
+
+ bool DoUpdateThreadList(ThreadList &old_thread_list,
+ ThreadList &new_thread_list) override {
+ return false;
+ }
+
+ size_t DoReadMemory(addr_t vm_addr, void *buf, size_t size,
+ Status &error) override {
+ return 0;
+ }
+
+ bool GetModuleSpec(const FileSpec &module_file_spec, const ArchSpec &arch,
+ ModuleSpec &module_spec) override {
+ module_spec = GetTestModuleSpec();
+ return true;
+ }
+};
+
+ProcessSP MockProcessCreateInstance(TargetSP target_sp, ListenerSP listener_sp,
+ const FileSpec *crash_file_path,
+ bool can_connect) {
+ return std::make_shared<MockProcess>(target_sp, listener_sp);
+}
+
+class SharedModuleListTest : public testing::Test {
+ SubsystemRAII<FileSystem, HostInfo, ObjectFileBreakpad, ObjectFileELF,
+ PlatformAndroid, PlatformLinux, SymbolFileBreakpad,
+ SymbolFileSymtab>
+ subsystems;
+
+public:
+ void SetUp() override {
+ m_test_dir = GetTestDir();
+
+ // Set module cache directory for PlatformAndroid.
+ PlatformAndroid::GetGlobalPlatformProperties().SetModuleCacheDirectory(
+ m_test_dir);
+
+ // Create Debugger.
+ ArchSpec host_arch("i386-pc-linux");
+ Platform::SetHostPlatform(
+ platform_linux::PlatformLinux::CreateInstance(true, &host_arch));
+ m_debugger_sp = Debugger::CreateInstance();
+ EXPECT_TRUE(m_debugger_sp);
+
+ // Create PlatformAndroid.
+ ArchSpec arch(k_arch);
+ m_platform_sp = PlatformAndroid::CreateInstance(true, &arch);
+ EXPECT_TRUE(m_platform_sp);
+
+ // Create Target.
+ m_debugger_sp->GetTargetList().CreateTarget(*m_debugger_sp, "", arch,
+ eLoadDependentsNo,
+ m_platform_sp, m_target_sp);
+ EXPECT_TRUE(m_target_sp);
+
+ // Create MockProcess.
+ PluginManager::RegisterPlugin(k_process_plugin, "",
+ MockProcessCreateInstance);
+ m_process_sp =
+ m_target_sp->CreateProcess(Listener::MakeListener("test-listener"),
+ k_process_plugin, /*crash_file=*/nullptr,
+ /*can_connect=*/true);
+ EXPECT_TRUE(m_process_sp);
+
+ m_module_spec = GetTestModuleSpec();
+ m_module_spec_without_uuid = ModuleSpec(GetRemotePath(), ArchSpec(k_arch));
+ }
+
+ void TearDown() override {
+ if (m_module_sp)
+ ModuleList::RemoveSharedModule(m_module_sp);
+ }
+
+protected:
+ FileSpec m_test_dir;
+ DebuggerSP m_debugger_sp;
+ PlatformSP m_platform_sp;
+ TargetSP m_target_sp;
+ ProcessSP m_process_sp;
+ ModuleSpec m_module_spec;
+ ModuleSpec m_module_spec_without_uuid;
+ ModuleSP m_module_sp;
+ int m_callback_call_count = 0;
+};
+
+} // namespace
+
+TEST_F(SharedModuleListTest, TestClear) {
+ FileSpec uuid_view = BuildCacheDir(m_test_dir);
+
+ m_module_sp = m_target_sp->GetOrCreateModule(m_module_spec, /*notify=*/false);
+ CheckModule(m_module_sp);
+ ASSERT_EQ(m_module_sp->GetFileSpec(), uuid_view);
+ ASSERT_FALSE(m_module_sp->GetSymbolFileFileSpec());
+
+ UUID uuid = m_module_sp->GetUUID();
+
+ // Check if the module is cached
+ ASSERT_TRUE(ModuleList::FindSharedModule(uuid));
+
+ // Clear cache and check that it is gone
+ ModuleList::ClearSharedModules();
+ ASSERT_FALSE(ModuleList::FindSharedModule(uuid));
+ m_module_sp = nullptr;
+}
|
|
✅ With the latest revision this PR passed the Python code formatter. |
Shared modules are stored in a global ModuleList cache, and it is
intentionally leaked to avoid doing cleanup when lldb exits.
However, when lldb is used as a library, we need a way to manage
opened modules to avoid problems with file locks (on some systems) for
modules that we no longer need.
It should be possible to record all loaded modules and use
ModuleList::RemoveSharedModule and RemoveOrphanSharedModules functions
to clear the cache, but these functions are not available in the API.
This approach is also way too complicated when we just need to cleanup
the library.
The patch adds the following:
- ModuleList::ClearSharedModules function to clear all shared
modules.
- SBDebugger::SetClearSharedModules(bool) API to enable clearing
during SBDebugger::Clear().
- `settings set clear-shared-modules true` command line option
that is equivalent to SetClearSharedModules(true).
This new behaviour is turned off by default: the debugger does not
release these shared modules until the process exits.
asavonic
force-pushed
the
lldb/shared-module-cache
branch
from
0737ec0 to
a01eb19
Compare
Fixed. |
Can you give a concrete example? This seems like its own issue that should be solved rather than worked around. I wouldn't expect us to need to keep files open for as long as a module exists.
This is exposed from the SB API through |
Right, it is a separate issue. I'll follow up later.
How should a proper debugger shutdown sequence look like?
If any module survives |
Oh, right. If we call |
Ideally,
Yes, if you destroyed all the debuggers and don't hold onto modules yourself, calling |
Aha, so it already kind-of works the way I was suggesting. Instead of forcefully deleting the modules, we ask nicely to release the ones that have no uses anymore. That seems very reasonable. I still wonder why we can't just boot the whole module cache but I'm sure there is a reason... |
The cache was added in 6f4d8af back in 2012. I guess It makes sense for a standalone debugger. I'll update the patch to use |
|
No one SBDebugger should force-clear the Shared Module Cache. That's a resource shared among all the SBDebuggers, and no SBDebugger can know what the other debugger's intentions are w.r.t. it. That seems wrong to me. The most an SBDebugger should do on Destroy or Clear is call MemoryPressureDetected or a similar lower-level API to clear all the modules that it is the only owner of. It should not clear the cache out from under other users. Even in its current reduced form I don't think this patch is a good idea. |
|
Updated the patch to use RemoveOrphanSharedModules instead of force-clearing the cache.
Agree.
The last revision should do that. The downside is that the program must release all shared pointers to modules before calling |
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
This patch is still trying attempting to clear the module list when clearing the debugger. As Jim said, that doesn't make sense. The module list is global and spans multiple debuggers.
Maybe I misunderstand the problem, but AFAIK all you need to achieve what you're trying to do is:
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ed674ee1275c..c6390e90f726 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -742,6 +742,8 @@ void Debugger::Terminate() {
g_debugger_list_ptr->clear();
}
}
+
+ ModuleList::RemoveOrphanSharedModules(/*mandatory=*/true);
}
void Debugger::SettingsInitialize() { Target::SettingsInitialize(); }
This is going to remove all the orphaned modules (= all modules if you're not holding on to any shared pointers, which is by design) when you terminate LLDB (i.e. the last thing you do with LLDB).
Do we need to tie this to any option, or we can clear these modules unconditionally? |
I don't think this needs to be an option if we go with the approach of only removing orphaned modules. |
|
Any Debugger that wants to can call MemoryPressureDetected or RemoveOrphanedSharedModules. That's always been true. But no Debugger should delete modules out from under any other. And appropriately there are no API's that actually do that. When Debugger::Terminate is called we should go through and close anything remaining on the module list. Note, we should have closed down all the SBDebuggers by the time we get to clearing the global module list in Terminate, so there really shouldn't be anything with a non-zero reference count on it. We test that in the test suite at the end of each test, and except for a few bugs, that's generally true. So the normal RemoveOrphanedSharedModules in Terminate should do the job, but since we're tearing down the library at that point, force removing the rest should be fine, and doesn't need to be controlled by a setting. Note however that the So if you want to really force that on exit there are NO modules in the shared module cache, I'd suggest adding another API like |
|
@jimingham, thanks a lot for the explanation! I think
I almost prepared a patch to add that, but tests highlighted another problem - there seems to be no way to recover from Thank you @jimingham and @JDevlieghere for your guidance here. |
4 participants
Shared modules are stored in a global
ModuleListcache, and it is intentionally leaked to avoid doing cleanup when lldb exits.However, when lldb is used as a library, we need a way to manage opened modules to avoid problems with file locks (on some systems) for modules that we no longer need.
It is possible to use
SBDebugger::MemoryPressureDetected(which callsModuleList::RemoveOrphanSharedModules) to clear all modules withuse_count == 1, but this has to be done afterSBDebugger::Destroy, because it may keep pointers to some modules.The patch adds the following:
If enabled,
SBDebugger::Destroynow callsModuleList::RemoveOrphanSharedModulesafter all its pointers are released.Note that if the program keeps pointers to modules elsewhere, they will not be released, and they will not be removed from the cache.
When a complete tear down is required, the program must ensure that all pointers are released before calling
SBDebugger::Destroy.SBDebugger::SetClearSharedModules(bool)API to enable this behavior.settings set clear-shared-modules truecommand line option that is equivalent toSetClearSharedModules(true).This new behaviour is turned off by default: the debugger does not release these shared modules until the process exits, or until
SBDebugger::MemoryPressureDetected.The option provides a clear way for library users to communicate that complete cleanup is required, without relying on side-effects of
MemoryPressureDetected.It also allows to evaluate impact of clearing modules on performance of standalone debuggers.