[Lldb-commits] [lldb] Fix up the TestGlobalModuleCache showing we succeed mostly. (PR #84099)
via lldb-commits
lldb-commits at lists.llvm.org
Tue Mar 5 16:49:28 PST 2024
https://github.com/jimingham created https://github.com/llvm/llvm-project/pull/84099
The point behind this test was that we were noticing when running under
a more complicated app than the driver (e.g. Xcode) we were seeing Modules
get stranded when doing incremental debugging.
These tests were to see if any of the slightly not normal but iterative development-like
actions would of themselves strand modules. I couldn't come up with a case where
they did, but a couple of test were failing because I didn't do them quite right.
In particular, "two targets one debugger" was failing because SBTarget.Clear
doesn't actually clear the target, just the SBTarget SP. So I needed to actually
delete the target for this one to work.
I also added a "two targets two debuggers" which also successfully deletes
all its modules by the time it is done.
Given that it looks like it's hard to get lldb itself to strand modules, this is likely
just an error on the IDE side. But also, Modules are expensive objects and once
we formally get rid of them by evicting them from the Shared Module Cache, it
would be good to actively Finalize them so they can't sit around taking up resources.
So I also add a couple test that show that an errant Python reference to
this will keep modules that lldb can no longer use alive. I made them fail
and xfailed them. But when we teach lldb to Finalize Modules, then we can
check at the end of the test that the modules we held onto have been finalized.
>From 44ef8137093c0d2e417cf09e71e4f81699d6d100 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Tue, 5 Mar 2024 12:49:11 -0800
Subject: [PATCH 1/2] Make the two targets one debugger and two targets two
debuggers tests pass. The two targets one debugger test was failing because I
incorrectly thought SBTarget.Clear did more than reset that SBTargets
internal shared pointer, which in this case didn't do anything, since there
are lots of other shared pointer references to the target. Calling "target
delete" fixed that failure.
The two debuggers two targets test was failing because the test wasn't right.
Note, what this shows is that actually deleting Modules from the shared module
cache happens in the normal course of shutting down lldb. But it is quite
easy to defeat this, for instance by holding another shared pointer to the
main Module. Then it will never go away.
---
.../TestGlobalModuleCache.py | 57 +++++++++++--------
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py b/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py
index 0942dcd655b750..500efb5fe594af 100644
--- a/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py
+++ b/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py
@@ -19,7 +19,7 @@ class GlobalModuleCacheTestCase(TestBase):
def check_counter_var(self, thread, value):
frame = thread.frames[0]
var = frame.FindVariable("counter")
- self.assertTrue(var.GetError().Success(), "Got counter variable")
+ self.assertSuccess(var.GetError(), "Didn't get counter variable")
self.assertEqual(var.GetValueAsUnsigned(), value, "This was one-print")
def copy_to_main(self, src, dst):
@@ -28,12 +28,13 @@ def copy_to_main(self, src, dst):
time.sleep(2)
try:
# Make sure dst is writeable before trying to write to it.
- subprocess.run(
- ["chmod", "777", dst],
- stdin=None,
- capture_output=False,
- encoding="utf-8",
- )
+ if os.path.exists(dst):
+ subprocess.run(
+ ["chmod", "777", dst],
+ stdin=None,
+ capture_output=False,
+ encoding="utf-8",
+ )
shutil.copy(src, dst)
except:
self.fail(f"Could not copy {src} to {dst}")
@@ -49,19 +50,16 @@ def copy_to_main(self, src, dst):
def test_OneTargetOneDebugger(self):
self.do_test(True, True)
- # This behaves as implemented but that behavior is not desirable.
- # This test tests for the desired behavior as an expected fail.
+
@skipIfWindows
- @expectedFailureAll
@skipIf(oslist=["linux"], archs=["arm", "aarch64"])
def test_TwoTargetsOneDebugger(self):
self.do_test(False, True)
@skipIfWindows
- @expectedFailureAll
@skipIf(oslist=["linux"], archs=["arm", "aarch64"])
- def test_OneTargetTwoDebuggers(self):
- self.do_test(True, False)
+ def test_TwoTargetsTwoDebuggers(self):
+ self.do_test(False, False)
def do_test(self, one_target, one_debugger):
# Make sure that if we have one target, and we run, then
@@ -94,7 +92,6 @@ def do_test(self, one_target, one_debugger):
process.Kill()
# Now copy two-print.c over main.c, rebuild, and rerun:
- # os.unlink(target.GetExecutable().fullpath)
self.copy_to_main(two_print_path, main_c_path)
self.build(dictionary={"C_SOURCES": main_c_path, "EXE": "a.out"})
@@ -109,15 +106,17 @@ def do_test(self, one_target, one_debugger):
self, "return counter;", main_filespec
)
else:
- if one_target:
+ if not one_target:
new_debugger = lldb.SBDebugger().Create()
+ new_debugger.SetAsync(False)
self.old_debugger = self.dbg
self.dbg = new_debugger
def cleanupDebugger(self):
- lldb.SBDebugger.Destroy(self.dbg)
- self.dbg = self.old_debugger
- self.old_debugger = None
+ if self.old_debugger != None:
+ lldb.SBDebugger.Destroy(self.dbg)
+ self.dbg = self.old_debugger
+ self.old_debugger = None
self.addTearDownHook(cleanupDebugger)
(target2, process2, thread, bkpt) = lldbutil.run_to_source_breakpoint(
@@ -129,19 +128,31 @@ def cleanupDebugger(self):
# If we made two targets, destroy the first one, that should free up the
# unreachable Modules:
+
+ # The original debugger is the one that owns the first target:
if not one_target:
- target.Clear()
+ dbg = None
+ if one_debugger:
+ dbg = self.dbg
+ else:
+ dbg = self.old_debugger
+ dbg.HandleCommand(f"target delete {dbg.GetIndexOfTarget(target)}")
- num_a_dot_out_entries = 1
# For dSYM's there will be two lines of output, one for the a.out and one
- # for the dSYM.
+ # for the dSYM, and no .o entries:
+ num_a_dot_out_entries = 1
+ num_main_dot_o_entries = 1
if debug_style == "dsym":
num_a_dot_out_entries += 1
+ num_main_dot_o_entries -= 1
error = self.check_image_list_result(num_a_dot_out_entries, 1)
# Even if this fails, MemoryPressureDetected should fix this.
+ if self.TraceOn():
+ print("*** Calling MemoryPressureDetected")
lldb.SBDebugger.MemoryPressureDetected()
- error_after_mpd = self.check_image_list_result(num_a_dot_out_entries, 1)
+ error_after_mpd = self.check_image_list_result(num_a_dot_out_entries,
+ num_main_dot_o_entries)
fail_msg = ""
if error != "":
fail_msg = "Error before MPD: " + error
@@ -158,7 +169,7 @@ def check_image_list_result(self, num_a_dot_out, num_main_dot_o):
# failing.
image_cmd_result = lldb.SBCommandReturnObject()
interp = self.dbg.GetCommandInterpreter()
- interp.HandleCommand("image list -g", image_cmd_result)
+ interp.HandleCommand("image list -g -r -u -h -f -S", image_cmd_result)
if self.TraceOn():
print(f"Expected: a.out: {num_a_dot_out} main.o: {num_main_dot_o}")
print(image_cmd_result)
>From 29866d5a183097348cb4d5b2bf3ec66f2fc0ccc4 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Tue, 5 Mar 2024 16:32:40 -0800
Subject: [PATCH 2/2] Fix up the TestGlobalModuleCache showing we succeed
mostly.
The two targets one debugger and two debuggers two target tests
were failing because the test wasn't quite right. Fix that here.
Also add a couple test that show that an errant Python reference to
this will keep modules that lldb can no longer use alive.
This last bit is in preparation for getting lldb to Finalize Modules
it can no longer use so that they don't hold onto resources that aren't
useable.
---
.../TestGlobalModuleCache.py | 41 ++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py b/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py
index 500efb5fe594af..a1b4ec4f9ef405 100644
--- a/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py
+++ b/lldb/test/API/python_api/global_module_cache/TestGlobalModuleCache.py
@@ -56,16 +56,36 @@ def test_OneTargetOneDebugger(self):
def test_TwoTargetsOneDebugger(self):
self.do_test(False, True)
+ @expectedFailureAll() # An external reference keeps modules alive
+ @skipIfWindows
+ @skipIf(oslist=["linux"], archs=["arm", "aarch64"])
+ def test_TwoTargetsOneDebuggerWithPin(self):
+ self.do_test(False, True, True)
+
@skipIfWindows
@skipIf(oslist=["linux"], archs=["arm", "aarch64"])
def test_TwoTargetsTwoDebuggers(self):
self.do_test(False, False)
- def do_test(self, one_target, one_debugger):
+ @expectedFailureAll() # An external reference keeps modules alive
+ #@skipIfWindows
+ #@skipIf(oslist=["linux"], archs=["arm", "aarch64"])
+ def test_TwoTargetsTwoDebuggersWithPin(self):
+ self.do_test(False, False, True)
+
+ def do_test(self, one_target, one_debugger, use_pinning_module=False):
# Make sure that if we have one target, and we run, then
# change the binary and rerun, the binary (and any .o files
# if using dwarf in .o file debugging) get removed from the
# shared module cache. They are no longer reachable.
+ # If use_pinning_module is true, we make another SBModule that holds
+ # a reference to the a.out, and will keep it alive.
+ # At present, those tests fail. But they show how easy it is to
+ # strand a module so it doesn't go away. We really should add a
+ # Finalize to Modules so when lldb removes them from the
+ # shared module cache, we delete the expensive parts of the Module
+ # and mark it no longer Valid. The we can test that that has
+ # happened.
debug_style = self.getDebugInfo()
# Before we do anything, clear the global module cache so we don't
@@ -87,6 +107,16 @@ def do_test(self, one_target, one_debugger):
self, "return counter;", main_filespec
)
+ self.pinning_module = None
+ if use_pinning_module:
+ self.pinning_module = target.FindModule(target.executable)
+ self.assertTrue(self.pinning_module.IsValid(), "Valid pinning module")
+ def cleanupPinningModule(self):
+ if self.pinning_module:
+ self.pinning_module.Clear()
+ self.pinning_module = None
+ self.addTearDownHook(cleanupPinningModule)
+
# Make sure we ran the version we intended here:
self.check_counter_var(thread, 1)
process.Kill()
@@ -96,6 +126,8 @@ def do_test(self, one_target, one_debugger):
self.build(dictionary={"C_SOURCES": main_c_path, "EXE": "a.out"})
error = lldb.SBError()
+
+ target2 = None
if one_debugger:
if one_target:
(_, process, thread, _) = lldbutil.run_to_breakpoint_do_run(
@@ -117,6 +149,8 @@ def cleanupDebugger(self):
lldb.SBDebugger.Destroy(self.dbg)
self.dbg = self.old_debugger
self.old_debugger = None
+ # The testsuite teardown asserts if we haven't deleted all
+ # modules by the time we exit, so we have to clean this up.
self.addTearDownHook(cleanupDebugger)
(target2, process2, thread, bkpt) = lldbutil.run_to_source_breakpoint(
@@ -146,6 +180,11 @@ def cleanupDebugger(self):
num_a_dot_out_entries += 1
num_main_dot_o_entries -= 1
+ # We shouldn't need the process anymore, that target holds onto the old
+ # image list, so get rid of it now:
+ if target2 and target2.process.IsValid():
+ target2.process.Kill()
+
error = self.check_image_list_result(num_a_dot_out_entries, 1)
# Even if this fails, MemoryPressureDetected should fix this.
if self.TraceOn():
More information about the lldb-commits
mailing list