https://github.com/asavonic created https://github.com/llvm/llvm-project/pull/156446
DynamicLoaderWindowsDYLD uses pointers to mModules to maintain a map from modules to their addresses, but it does not need to keep "strong" references to them. Weak pointers should be enough, and would allow modules to be released elsewhere. Other DynamicLoader classes do not use shared pointers as well. For example, DynamicLoaderPOSIXDYLD has a similar map with weak pointers. Actually testing for modules being completely released can be tricky. The test here is just to illustrate the case where shared pointers kept modules in DynamicLoaderWindowsDYLD and prevented them from being released. The test executes the following sequence: 1. Create a target, load an executable and run it. 2. Remove one module from the target. The target should be the last actual use of the module, but we have another reference to it in the shared module cache. 3. Call MemoryPressureDetected to remove this last reference from the cache. 4. Replace the corresponding DLL file. LLDB memory maps DLLs, and this makes files read-only on Windows. Unless the modules are completely released (and therefore unmapped), (4) is going to fail with "access denied". However, the test does not trigger the bug completely - it passes with and without the change. >From 1ddf63f5018dce7db3a7e5a5ba43e7f1d3d9764c Mon Sep 17 00:00:00 2001 From: Andrew Savonichev <[email protected]> Date: Tue, 2 Sep 2025 17:41:13 +0900 Subject: [PATCH] [lldb] Use weak pointers instead of shared pointers in DynamicLoader DynamicLoaderWindowsDYLD uses pointers to Modules to maintain a map from modules to their addresses, but it does not need to keep "strong" references to them. Weak pointers should be enough, and would allow modules to be released elsewhere. Other DynamicLoader classes do not use shared pointers as well. For example, DynamicLoaderPOSIXDYLD has a similar map with weak pointers. Actually testing for modules being completely released can be tricky. The test here is just to illustrate the case where shared pointers kept modules in DynamicLoaderWindowsDYLD and prevented them from being released. The test executes the following sequence: 1. Create a target, load an executable and run it. 2. Remove one module from the target. The target should be the last actual use of the module, but we have another reference to it in the shared module cache. 3. Call MemoryPressureDetected to remove this last reference from the cache. 4. Replace the corresponding DLL file. LLDB memory maps DLLs, and this makes the files read-only on Windows. Unless the modules are completely released (and therefore unmapped), (4) is going to fail with "access denied". However, the test does not trigger the bug completely - it passes with and without the change. --- .../Windows-DYLD/DynamicLoaderWindowsDYLD.h | 3 +- .../API/windows/launch/replace-dll/Makefile | 1 + .../launch/replace-dll/TestReplaceDLL.py | 62 +++++++++++++++++++ .../test/API/windows/launch/replace-dll/bar.c | 1 + .../test/API/windows/launch/replace-dll/foo.c | 1 + .../API/windows/launch/replace-dll/test.c | 3 + 6 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 lldb/test/API/windows/launch/replace-dll/Makefile create mode 100644 lldb/test/API/windows/launch/replace-dll/TestReplaceDLL.py create mode 100644 lldb/test/API/windows/launch/replace-dll/bar.c create mode 100644 lldb/test/API/windows/launch/replace-dll/foo.c create mode 100644 lldb/test/API/windows/launch/replace-dll/test.c diff --git a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h index 42ea5aacecb40..8b1c3c3f467f4 100644 --- a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h +++ b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h @@ -45,7 +45,8 @@ class DynamicLoaderWindowsDYLD : public DynamicLoader { lldb::addr_t GetLoadAddress(lldb::ModuleSP executable); private: - std::map<lldb::ModuleSP, lldb::addr_t> m_loaded_modules; + std::map<lldb::ModuleWP, lldb::addr_t, std::owner_less<lldb::ModuleWP>> + m_loaded_modules; }; } // namespace lldb_private diff --git a/lldb/test/API/windows/launch/replace-dll/Makefile b/lldb/test/API/windows/launch/replace-dll/Makefile new file mode 100644 index 0000000000000..22f1051530f87 --- /dev/null +++ b/lldb/test/API/windows/launch/replace-dll/Makefile @@ -0,0 +1 @@ +include Makefile.rules diff --git a/lldb/test/API/windows/launch/replace-dll/TestReplaceDLL.py b/lldb/test/API/windows/launch/replace-dll/TestReplaceDLL.py new file mode 100644 index 0000000000000..afa97cf4afe50 --- /dev/null +++ b/lldb/test/API/windows/launch/replace-dll/TestReplaceDLL.py @@ -0,0 +1,62 @@ +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + +import gc +import os + + +class ReplaceDllTestCase(TestBase): + @skipUnlessWindows + def test(self): + """ + Test that LLDB unlocks module files once all references are released. + """ + + exe = self.getBuildArtifact("a.out") + foo = self.getBuildArtifact("foo.dll") + bar = self.getBuildArtifact("bar.dll") + + self.build( + dictionary={ + "DYLIB_NAME": "foo", + "DYLIB_C_SOURCES": "foo.c", + "C_SOURCES": "test.c", + } + ) + self.build( + dictionary={ + "DYLIB_ONLY": "YES", + "DYLIB_NAME": "bar", + "DYLIB_C_SOURCES": "bar.c", + } + ) + + target = self.dbg.CreateTarget(exe) + self.assertTrue(target, VALID_TARGET) + + shlib_names = ["foo"] + environment = self.registerSharedLibrariesWithTarget(target, shlib_names) + process = target.LaunchSimple( + None, environment, self.get_process_working_directory() + ) + self.assertEqual(process.GetExitStatus(), 42) + + module = next((m for m in target.modules if "foo" in m.file.basename), None) + self.assertIsNotNone(module) + self.assertEqual(module.file.fullpath, foo) + + target.RemoveModule(module) + del module + gc.collect() + + self.dbg.MemoryPressureDetected() + + os.remove(foo) + os.rename(bar, foo) + + process = target.LaunchSimple( + None, environment, self.get_process_working_directory() + ) + self.assertEqual(process.GetExitStatus(), 43) diff --git a/lldb/test/API/windows/launch/replace-dll/bar.c b/lldb/test/API/windows/launch/replace-dll/bar.c new file mode 100644 index 0000000000000..7ddde51234831 --- /dev/null +++ b/lldb/test/API/windows/launch/replace-dll/bar.c @@ -0,0 +1 @@ +__declspec(dllexport) int foo() { return 43; } diff --git a/lldb/test/API/windows/launch/replace-dll/foo.c b/lldb/test/API/windows/launch/replace-dll/foo.c new file mode 100644 index 0000000000000..b971fccc6ed9a --- /dev/null +++ b/lldb/test/API/windows/launch/replace-dll/foo.c @@ -0,0 +1 @@ +__declspec(dllexport) int foo() { return 42; } diff --git a/lldb/test/API/windows/launch/replace-dll/test.c b/lldb/test/API/windows/launch/replace-dll/test.c new file mode 100644 index 0000000000000..4c29f7852ae09 --- /dev/null +++ b/lldb/test/API/windows/launch/replace-dll/test.c @@ -0,0 +1,3 @@ +int foo(void); + +int main() { return foo(); } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
