Fix the managing of the session dictionary when you have nested wrappers

[jimingham]

Since the inner wrapper call might have removed one of the entries from the global dict that the outer wrapper ALSO was going to delete, make sure that we check that the key is still in the global dict before trying to act on it.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

Since the inner wrapper call might have removed one of the entries from the global dict that the outer wrapper ALSO was going to delete, make sure that we check that the key is still in the global dict before trying to act on it.

---

Full diff: https://github.com/llvm/llvm-project/pull/132846.diff

5 Files Affected:

• (modified) lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp (+9-5)
• (added) lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/Makefile (+4)
• (added) lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/TestNestedBreakpointCommands.py (+58)
• (added) lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/main.c (+16)
• (added) lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/make_bkpt_cmds.py (+28)

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index 00d01981c64ff..84297d0d31b4e 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -1267,6 +1267,8 @@ Status ScriptInterpreterPythonImpl::ExportFunctionDefinitionToInterpreter(
     StringList &function_def) {
   // Convert StringList to one long, newline delimited, const char *.
   std::string function_def_string(function_def.CopyList());
+  LLDB_LOG(GetLog(LLDBLog::Script), "Added Function:\n%s\n", 
+      function_def_string.c_str());
 
   Status error = ExecuteMultipleLines(
       function_def_string.c_str(), ExecuteScriptOptions().SetEnableIO(false));
@@ -1336,13 +1338,15 @@ Status ScriptInterpreterPythonImpl::GenerateFunction(const char *signature,
       "    for key in new_keys:"); // Iterate over all the keys from session
                                    // dict
   auto_generated_function.AppendString(
-      "        internal_dict[key] = global_dict[key]"); // Update session dict
-                                                        // values
+      "        if key in old_keys:"); // If key was originally in
+                                      // global dict
   auto_generated_function.AppendString(
-      "        if key not in old_keys:"); // If key was not originally in
-                                          // global dict
+      "            internal_dict[key] = global_dict[key]"); // Update it
   auto_generated_function.AppendString(
-      "            del global_dict[key]"); //  ...then remove key/value from
+      "        elif key in global_dict:"); // Then if it is still in the
+                                           // global dict
+  auto_generated_function.AppendString(
+      "            del global_dict[key]"); //  remove key/value from the
                                            //  global dict
   auto_generated_function.AppendString(
       "    return __return_val"); //  Return the user callback return value.
diff --git a/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/Makefile b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/Makefile
new file mode 100644
index 0000000000000..695335e068c0c
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/TestNestedBreakpointCommands.py b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/TestNestedBreakpointCommands.py
new file mode 100644
index 0000000000000..5dffce4b11678
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/TestNestedBreakpointCommands.py
@@ -0,0 +1,58 @@
+"""
+Test that a Python breakpoint callback defined in another Python
+breakpoint callback works properly. 
+"""
+
+
+import lldb
+import os
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestNestedBreakpointCommands(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_nested_commands(self):
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.c")
+        self.callback_module = "make_bkpt_cmds"
+        self.do_test()
+
+    def do_test(self):
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, "Set a breakpoint here", self.main_source_file
+        )
+
+        outer_bkpt = target.BreakpointCreateBySourceRegex(
+            "Set outer breakpoint here",
+            self.main_source_file
+        )
+        cmd_file_path = os.path.join(self.getSourceDir(), f"{self.callback_module}.py")
+        self.runCmd(f"command script import {cmd_file_path}")
+        outer_bkpt.SetScriptCallbackFunction(f"{self.callback_module}.outer_callback")
+
+        process.Continue()
+
+        self.assertEqual(thread.stop_reason, lldb.eStopReasonBreakpoint, "Right stop reason")
+
+        bkpt_no = thread.stop_reason_data[0]
+
+        # We made the callbacks record the new breakpoint ID and the number of
+        # times a callback ran in some globals in the target.  Find them now:
+        exec_module = target.FindModule(target.executable)
+        self.assertTrue(exec_module.IsValid(), "Found executable module")
+        var = exec_module.FindFirstGlobalVariable(target, "g_global")
+        self.assertSuccess(var.GetError(), "Found globals")
+        num_hits = var.GetChildAtIndex(1).GetValueAsUnsigned()
+        inner_id = var.GetChildAtIndex(2).GetValueAsUnsigned()
+
+        # Make sure they have the right values:
+        self.assertEqual(bkpt_no, inner_id, "Hit the right breakpoint")
+        self.assertEqual(num_hits, 2, "Right counter end value")
+        self.assertEqual(thread.frames[0].name, "main", "Got to main")
+
+        self.assertEqual(outer_bkpt.GetHitCount(), 1, "Hit outer breakpoint once")
+
+        inner_bkpt = target.FindBreakpointByID(inner_id)
+        self.assertEqual(inner_bkpt.GetHitCount(), 1, "Hit inner breakpoint once")
diff --git a/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/main.c b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/main.c
new file mode 100644
index 0000000000000..6425bd702a888
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/main.c
@@ -0,0 +1,16 @@
+#include <stdio.h>
+
+int g_global[3] = {0, 100000, 100000};
+
+void doSomething()
+{
+  g_global[0] = 1; // Set outer breakpoint here
+}
+
+int
+main()
+{
+  doSomething(); // Set a breakpoint here
+
+  return g_global[0];
+}
diff --git a/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/make_bkpt_cmds.py b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/make_bkpt_cmds.py
new file mode 100644
index 0000000000000..7616dc64acc9a
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/nested_breakpoint_commands/make_bkpt_cmds.py
@@ -0,0 +1,28 @@
+import lldb
+
+def set_globals(target, index, value):
+    exe_module = target.FindModule(target.executable)
+    var = exe_module.FindFirstGlobalVariable(target, "g_global")
+    child = var.GetChildAtIndex(index)
+    child.SetValueFromCString(str(value))
+
+def outer_callback(frame: lldb.SBFrame, bp_loc, internal_dict):
+    thread = frame.GetThread()
+
+    # address of the next frame
+    next_frame_pc = thread.get_thread_frames()[1].GetPC()
+   
+    target = thread.process.target
+    bp = target.BreakpointCreateByAddress(next_frame_pc)
+    bp.SetScriptCallbackFunction(f"{__name__}.nested_bp_callback")
+    set_globals(target, 1, 1)
+    set_globals(target, 2, bp.GetID())
+    
+    return False
+
+def nested_bp_callback(frame: lldb.SBFrame, bp_loc, extra_args, internal_dict):
+    target = frame.thread.process.target
+    set_globals(target, 1, 2)
+
+    return True
+

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[medismailben]

LGTM!