[lldb][api-test] Add API test for SBCommandInterpreter::CommandOverrideCallback #94518

chelcassanova · 2024-06-05T18:33:08Z

SBCommandInterpreter::CommandOverrideCallback was not being exposed to the Python API has no coverage in the
API test suite, so this commits exposes and adds a test for it. Doing this involves also adding a typemap for the callback used for this function so that it matches the functionality of other callback functions that are exposed to Python.

llvmbot · 2024-06-05T18:33:42Z

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

SBCommandInterpreter::CommandOverrideCallback was not being exposed to the Python API has no coverage in the
API test suite, so this commits exposes and adds a test for it. Doing this involves also adding a typemap for the callback used for this function so that it matches the functionality of other callback functions that are exposed to Python.

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

4 Files Affected:

(modified) lldb/bindings/python/python-typemaps.swig (+26-1)
(modified) lldb/bindings/python/python-wrapper.swig (+13)
(modified) lldb/include/lldb/API/SBCommandInterpreter.h (-2)
(added) lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py (+30)

diff --git a/lldb/bindings/python/python-typemaps.swig b/lldb/bindings/python/python-typemaps.swig
index 8d4b740e5f35c..1bf93c59fbc7b 100644
--- a/lldb/bindings/python/python-typemaps.swig
+++ b/lldb/bindings/python/python-typemaps.swig
@@ -427,7 +427,6 @@ template <> bool SetNumberFromPyObject<double>(double &number, PyObject *obj) {
   free($1);
 }
 
-
 // For Log::LogOutputCallback
 %typemap(in) (lldb::LogOutputCallback log_callback, void *baton) {
   if (!($input == Py_None ||
@@ -476,6 +475,32 @@ template <> bool SetNumberFromPyObject<double>(double &number, PyObject *obj) {
   $1 = $1 || PyCallable_Check(reinterpret_cast<PyObject *>($input));
 }
 
+%typemap(in) (lldb::CommandOverrideCallback callback, void *baton) {
+  if (!($input == Py_None ||
+        PyCallable_Check(reinterpret_cast<PyObject *>($input)))) {
+    PyErr_SetString(PyExc_TypeError, "Need a callable object or None!");
+    SWIG_fail;
+  }
+
+  // FIXME (filcab): We can't currently check if our callback is already
+  // LLDBSwigPythonCallPythonSBDebuggerTerminateCallback (to DECREF the previous
+  // baton) nor can we just remove all traces of a callback, if we want to
+  // revert to a file logging mechanism.
+
+  // Don't lose the callback reference
+  Py_INCREF($input);
+  $1 = LLDBSwigPythonCallPythonSBCommandInterpreterSetCommandOverrideCallback;
+  $2 = $input;
+}
+
+%typemap(typecheck) (lldb::CommandOverrideCallback callback,
+
+                                  void *baton) {
+
+  $1 = $input == Py_None;
+  $1 = $1 || PyCallable_Check(reinterpret_cast<PyObject *>($input));
+}
+
 %typemap(in) lldb::FileSP {
   PythonFile py_file(PyRefType::Borrowed, $input);
   if (!py_file) {
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 1370afc885d43..0a332cb0a2ff4 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -1099,6 +1099,19 @@ static void LLDBSwigPythonCallPythonSBDebuggerTerminateCallback(lldb::user_id_t
   }
 }
 
+static bool LLDBSwigPythonCallPythonSBCommandInterpreterSetCommandOverrideCallback(void *baton, const char **argv) {
+  bool b = false;
+  if (baton != Py_None) {
+    SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+    PyObject *result = PyObject_CallFunction(
+      reinterpret_cast<PyObject *>(baton), const_cast<char *>("s"), argv); // WRONG!!!!!
+    b = result ? PyObject_IsTrue(result) : false;
+    Py_XDECREF(result);
+    SWIG_PYTHON_THREAD_END_BLOCK;
+  }
+  return b;
+}
+
 static SBError LLDBSwigPythonCallLocateModuleCallback(
     void *callback_baton, const SBModuleSpec &module_spec_sb,
     SBFileSpec &module_file_spec_sb, SBFileSpec &symbol_file_spec_sb) {
diff --git a/lldb/include/lldb/API/SBCommandInterpreter.h b/lldb/include/lldb/API/SBCommandInterpreter.h
index 8ac36344b3a79..084b6d9adb703 100644
--- a/lldb/include/lldb/API/SBCommandInterpreter.h
+++ b/lldb/include/lldb/API/SBCommandInterpreter.h
@@ -265,11 +265,9 @@ class SBCommandInterpreter {
   // Catch commands before they execute by registering a callback that will get
   // called when the command gets executed. This allows GUI or command line
   // interfaces to intercept a command and stop it from happening
-#ifndef SWIG
   bool SetCommandOverrideCallback(const char *command_name,
                                   lldb::CommandOverrideCallback callback,
                                   void *baton);
-#endif
 
   /// Return true if the command interpreter is the active IO handler.
   ///
diff --git a/lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py b/lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py
new file mode 100644
index 0000000000000..067f37f88096d
--- /dev/null
+++ b/lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py
@@ -0,0 +1,30 @@
+from typing_extensions import override
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class CommandOverrideCallback(TestBase):
+    def setUp(self):
+        TestBase.setUp(self)
+        self.line = line_number("main.c", "Hello world.")
+
+    def test_command_override_callback(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Create a target by the debugger.
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Retrieve the associated command interpreter from our debugger.
+        ci = self.dbg.GetCommandInterpreter()
+        self.assertTrue(ci, VALID_COMMAND_INTERPRETER)
+
+        override_string = "what"
+        def foo(string):
+            nonlocal override_string
+            override_string = string
+            return False
+
+        self.assertTrue(ci.SetCommandOverrideCallback("breakpoint", foo))

github-actions · 2024-06-05T18:36:10Z

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

medismailben · 2024-06-05T19:50:16Z

lldb/bindings/python/python-wrapper.swig

+  if (baton != Py_None) {
+    SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+    PyObject *result = PyObject_CallFunction(
+      reinterpret_cast<PyObject *>(baton), const_cast<char *>("s"), argv); // WRONG!!!!!


You need to convert your char** into a PyList of PyString before passing it here I think

Yes, forgot to do this here when I cleaned this up and you can see the comment from that 😅

medismailben · 2024-06-05T19:50:49Z

lldb/bindings/python/python-typemaps.swig

+    SWIG_fail;
+  }
+
+  // FIXME (filcab): We can't currently check if our callback is already


I'd say remove the FIXME entirely, it shouldn't apply to this.
As a side note, I think in the LLVM project we generally discourage the pattern of FIXME (name) and TODO (name). Maybe we should go clean up the original instance too? :p

I'm not opposed to that, this same message appears for the other 2 callbacks.

bulbazord · 2024-06-06T17:52:40Z

lldb/bindings/python/python-typemaps.swig

+    SWIG_fail;
+  }
+
+  // FIXME (filcab): We can't currently check if our callback is already


I'd say remove the FIXME entirely, it shouldn't apply to this.
As a side note, I think in the LLVM project we generally discourage the pattern of FIXME (name) and TODO (name). Maybe we should go clean up the original instance too? :p

bulbazord · 2024-06-06T17:52:55Z

lldb/bindings/python/python-typemaps.swig

+}
+
+%typemap(typecheck) (lldb::CommandOverrideCallback callback,
+


stray line?

Yup, I kind of wish that SWIG had a formatter like clang-format.

bulbazord · 2024-06-06T17:54:07Z

lldb/bindings/python/python-wrapper.swig

@@ -1099,6 +1099,19 @@ static void LLDBSwigPythonCallPythonSBDebuggerTerminateCallback(lldb::user_id_t
  }
 }

+static bool LLDBSwigPythonCallPythonSBCommandInterpreterSetCommandOverrideCallback(void *baton, const char **argv) {
+  bool b = false;


Can you change the name b to something with more meaning? Maybe something like ret_val?

bulbazord · 2024-06-06T17:56:23Z

lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py

+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Create a target by the debugger.


I don't think this comment is helpful because it's telling me what the code is doing, not why it's doing it. I'd suggest removing it because pretty much every test does this.

bulbazord · 2024-06-06T17:56:48Z

lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py

+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Retrieve the associated command interpreter from our debugger.


Same for this comment.

JDevlieghere · 2024-06-07T21:01:28Z

lldb/bindings/python/python-typemaps.swig

+    SWIG_fail;
+  }
+
+  // Don't lose the callback reference


Nit: missing period.

JDevlieghere · 2024-06-07T21:02:31Z

lldb/test/API/python_api/interpreter/TestCommandOverrideCallback.py

+        def foo(*command_args):
+            pass
+
+        self.assertTrue(ci.SetCommandOverrideCallback("breakpoint set", foo))
+        self.expect("breakpoint set -n main", substrs=["Breakpoint"])


This doesn't test the baton or the fact that the function was called. You should have the foo callback modify a variable or something and verify that the function has been called.

SBCommandInterpreter::CommandOverrideCallback `SBCommandInterpreter::CommandOverrideCallback` was not being exposed to the Python API has no coverage in the API test suite, so this commits exposes and adds a test for it. Doing this involves also adding a typemap for the callback used for this function so that it matches the functionality of other callback functions that are exposed to Python.

JDevlieghere

LGTM

medismailben

LGTM!

…ndOverrideCallback (#94518)" This reverts commit 6fb6eba. This test breaks due to an incorrect import in the test.

…ndOverrideCallback (llvm#94518)" This reverts commit 7cff05a. The API test that was added erroneously imports a module that isn't needed and wouldn't be found which causes a test failures. This reversion removes that import.

…#95181) …andOverrideCallback (#94518)" This reverts commit 7cff05a. The API test that was added erroneously imports a module that isn't needed and wouldn't be found which causes a test failures. This reversion removes that import.

…llvm#95181) …andOverrideCallback (llvm#94518)" This reverts commit 7cff05a. The API test that was added erroneously imports a module that isn't needed and wouldn't be found which causes a test failures. This reversion removes that import. (cherry picked from commit 3d86eeb)

chelcassanova requested review from bulbazord and medismailben June 5, 2024 18:33

chelcassanova requested a review from JDevlieghere as a code owner June 5, 2024 18:33

llvmbot added the lldb label Jun 5, 2024

chelcassanova force-pushed the add-commandoverridecallback-api-test branch from 90602ed to e4ba69a Compare June 5, 2024 19:49

medismailben requested changes Jun 5, 2024

View reviewed changes

bulbazord reviewed Jun 6, 2024

View reviewed changes

chelcassanova force-pushed the add-commandoverridecallback-api-test branch from e4ba69a to 44e8cab Compare June 7, 2024 20:36

JDevlieghere requested changes Jun 7, 2024

View reviewed changes

chelcassanova force-pushed the add-commandoverridecallback-api-test branch from 44e8cab to a721558 Compare June 7, 2024 21:51

chelcassanova force-pushed the add-commandoverridecallback-api-test branch from a721558 to 4e40f07 Compare June 7, 2024 21:52

medismailben self-requested a review June 11, 2024 21:52

chelcassanova requested a review from JDevlieghere June 11, 2024 21:53

JDevlieghere approved these changes Jun 11, 2024

View reviewed changes

medismailben approved these changes Jun 11, 2024

View reviewed changes

chelcassanova merged commit 6fb6eba into llvm:main Jun 11, 2024
5 checks passed

chelcassanova added a commit that referenced this pull request Jun 11, 2024

Revert "[lldb][api-test] Add API test for SBCommandInterpreter::Comma…

7cff05a

…ndOverrideCallback (#94518)" This reverts commit 6fb6eba. This test breaks due to an incorrect import in the test.

chelcassanova mentioned this pull request Jun 11, 2024

Reland "[lldb][api-test] Add API test for SBCommandInterpreter::Comm… #95181

Merged

chelcassanova mentioned this pull request Jun 12, 2024

Reland "[lldb][api-test] Add API test for SBCommandInterpreter::Comm… swiftlang/llvm-project#8884

Merged

		}

		%typemap(typecheck) (lldb::CommandOverrideCallback callback,

[lldb][api-test] Add API test for SBCommandInterpreter::CommandOverrideCallback #94518

[lldb][api-test] Add API test for SBCommandInterpreter::CommandOverrideCallback #94518

Uh oh!

Conversation

chelcassanova commented Jun 5, 2024

Uh oh!

llvmbot commented Jun 5, 2024

Uh oh!

github-actions bot commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 5, 2024 •

edited

Loading