Re: [PATCH v2 2/4] gdb/python: eval_python_command returns result of the evaluation

[Matthieu Longo <matthieu.longo@arm.com>]

On 15/05/2026 17:51, Tom Tromey wrote:
>>>>>> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> +++ b/gdbsupport/gdb_ref_ptr.h
>> @@ -179,6 +179,11 @@ class ref_ptr
>>       return m_obj;
>>     }
>   
>> +  operator bool () const noexcept
>> +  {
>> +    return m_obj != nullptr;
>> +  }
> 
> gdb is normally explicit instead, and since there aren't many call sites
> I think this operator should be dropped and those spots updated to check
> against nullptr.
> 
> Tom

Here is the updated diff.

(1)
   if (eval_python_command (...) == nullptr)
VS (2)
   if (! eval_python_command (...))

As you can see below, (1) is significantly more verbose than (2) and makes more difficult to respect 
the "80 characters per line" rule.

Matthieu

diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c
index e8e2c23547c..f3d87e0ae25 100644
--- a/gdb/python/py-gdb-readline.c
+++ b/gdb/python/py-gdb-readline.c
@@ -114,7 +114,7 @@ class GdbRemoveReadlineFinder(MetaPathFinder):\n\
  \n\
  sys.meta_path.insert(2, GdbRemoveReadlineFinder())\n\
  ";
-  if (eval_python_command (code, Py_file_input) == 0)
+  if (eval_python_command (code, Py_file_input) != nullptr)
      PyOS_ReadlineFunctionPointer = gdbpy_readline_wrapper;

    return 0;
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 82f3262ae07..a26e33c33a1 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -1342,8 +1342,8 @@ class gdbpy_memoizing_registry_storage
    gdb::unordered_map<val_type *, obj_type *> m_objects;
  };

-extern int eval_python_command (const char *command, int start_symbol,
-                               const char *filename = nullptr);
+extern gdbpy_ref<> eval_python_command
+  (const char *command, int start_symbol, const char *filename = nullptr);

  /* The following four functions are refcount-safe wrappers around
     Py_RETURN_{NONE,TRUE,FALSE,NOTIMPLEMENTED}.  */
diff --git a/gdb/python/python.c b/gdb/python/python.c
index fd254a340d8..2b266a56759 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -298,44 +298,43 @@ gdbpy_check_quit_flag (const struct extension_language_defn *extlang)
     Python start symbol, and does not automatically print the stack on
     errors.  FILENAME is used to set the file name in error messages;
     NULL means that this is evaluating a string, not the contents of a
-   file.  */
+   file.
+   Return the result of the evaluation on success, NULL on failure.  */

-int
+gdbpy_ref<>
  eval_python_command (const char *command, int start_symbol,
                      const char *filename)
  {
-  PyObject *m, *d;
-
-  m = PyImport_AddModule ("__main__");
-  if (m == NULL)
-    return -1;
+  gdbpy_borrowed_ref<> mainmod = PyImport_AddModule ("__main__");
+  if (mainmod == nullptr)
+    return nullptr;

-  d = PyModule_GetDict (m);
-  if (d == NULL)
-    return -1;
+  gdbpy_borrowed_ref<> globals = PyModule_GetDict (mainmod);
+  if (globals == nullptr)
+    return nullptr;

    bool file_set = false;
    if (filename != nullptr)
      {
        gdbpy_ref<> file = host_string_to_python_string ("__file__");
        if (file == nullptr)
-       return -1;
+       return nullptr;

        /* PyDict_GetItemWithError returns a borrowed reference.  */
-      PyObject *found = PyDict_GetItemWithError (d, file.get ());
+      PyObject *found = PyDict_GetItemWithError (globals, file.get ());
        if (found == nullptr)
         {
           if (PyErr_Occurred ())
-           return -1;
+           return nullptr;

           gdbpy_ref<> filename_obj = host_string_to_python_string (filename);
           if (filename_obj == nullptr)
-           return -1;
+           return nullptr;

-         if (PyDict_SetItem (d, file.get (), filename_obj.get ()) < 0)
-           return -1;
-         if (PyDict_SetItemString (d, "__cached__", Py_None) < 0)
-           return -1;
+         if (PyDict_SetItem (globals, file.get (), filename_obj.get ()) < 0)
+           return nullptr;
+         if (PyDict_SetItemString (globals, "__cached__", Py_None) < 0)
+           return nullptr;

           file_set = true;
         }
@@ -348,12 +347,12 @@ eval_python_command (const char *command, int start_symbol,
                                       : filename,
                                       start_symbol));

-  int result = -1;
+  gdbpy_ref<> eval_result;
    if (code != nullptr)
      {
-      gdbpy_ref<> eval_result (PyEval_EvalCode (code.get (), d, d));
-      if (eval_result != nullptr)
-       result = 0;
+      eval_result.reset (PyEval_EvalCode (code.get (), globals, globals));
+      if (eval_result == nullptr)
+       gdb_assert (PyErr_Occurred ());
      }

    if (file_set)
@@ -361,21 +360,21 @@ eval_python_command (const char *command, int start_symbol,
        /* If there's already an exception occurring, preserve it and
          restore it before returning from this function.  */
        std::optional<gdbpy_err_fetch> save_error;
-      if (result < 0)
+      if (eval_result == nullptr)
         save_error.emplace ();

        /* CPython also just ignores errors here.  These should be
          expected to be exceedingly rare anyway.  */
-      if (PyDict_DelItemString (d, "__file__") < 0)
+      if (PyDict_DelItemString (globals, "__file__") < 0)
         PyErr_Clear ();
-      if (PyDict_DelItemString (d, "__cached__") < 0)
+      if (PyDict_DelItemString (globals, "__cached__") < 0)
         PyErr_Clear ();

        if (save_error.has_value ())
         save_error->restore ();
      }

-  return result;
+  return eval_result;
  }

  /* Implementation of the gdb "python-interactive" command.  */
@@ -384,7 +383,7 @@ static void
  python_interactive_command (const char *arg, int from_tty)
  {
    struct ui *ui = current_ui;
-  int err;
+  bool err;

    scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);

@@ -396,11 +395,11 @@ python_interactive_command (const char *arg, int from_tty)
      {
        std::string script = std::string (arg) + "\n";
        /* Py_single_input causes the result to be displayed.  */
-      err = eval_python_command (script.c_str (), Py_single_input);
+      err = (eval_python_command (script.c_str (), Py_single_input) == nullptr);
      }
    else
      {
-      err = PyRun_InteractiveLoop (ui->instream, "<stdin>");
+      err = PyRun_InteractiveLoop (ui->instream, "<stdin>") != 0;
        dont_repeat ();
      }

@@ -411,6 +410,7 @@ python_interactive_command (const char *arg, int from_tty)
  /* Like PyRun_SimpleFile, but if there is an exception, it is not
     automatically displayed.  FILE is the Python script to run named
     FILENAME.
+   Return true on success, false on failure.

     On Windows hosts few users would build Python themselves (this is no
     trivial task on this platform), and thus use binaries built by
@@ -421,11 +421,11 @@ python_interactive_command (const char *arg, int from_tty)
     A FILE * from one runtime does not necessarily operate correctly in
     the other runtime.  */

-static int
+static bool
  python_run_simple_file (FILE *file, const char *filename)
  {
    std::string contents = read_remainder_of_file (file);
-  return eval_python_command (contents.c_str (), Py_file_input, filename);
+  return eval_python_command (contents.c_str (), Py_file_input, filename) != nullptr;
  }

  /* Given a command_line, return a command string suitable for passing
@@ -459,8 +459,7 @@ gdbpy_eval_from_control_command (const struct extension_language_defn *extlang,
    gdbpy_enter enter_py;

    std::string script = compute_python_string (cmd->body_list_0.get ());
-  int ret = eval_python_command (script.c_str (), Py_file_input);
-  if (ret != 0)
+  if (eval_python_command (script.c_str (), Py_file_input) == nullptr)
      gdbpy_handle_exception ();
  }

@@ -476,8 +475,7 @@ python_command (const char *arg, int from_tty)
    arg = skip_spaces (arg);
    if (arg && *arg)
      {
-      int ret = eval_python_command (arg, Py_file_input);
-      if (ret != 0)
+      if (eval_python_command (arg, Py_file_input) == nullptr)
         gdbpy_handle_exception ();
      }
    else
@@ -1120,8 +1118,7 @@ gdbpy_source_script (const struct extension_language_defn *extlang,
                      FILE *file, const char *filename)
  {
    gdbpy_enter enter_py;
-  int result = python_run_simple_file (file, filename);
-  if (result != 0)
+  if (! python_run_simple_file (file, filename))
      gdbpy_handle_exception ();
  }

@@ -1827,8 +1824,7 @@ gdbpy_source_objfile_script (const struct extension_language_defn *extlang,
    scoped_restore restire_current_objfile
      = make_scoped_restore (&gdbpy_current_objfile, objfile);

-  int result = python_run_simple_file (file, filename);
-  if (result != 0)
+  if (! python_run_simple_file (file, filename))
      gdbpy_print_stack ();
  }

@@ -1850,8 +1846,7 @@ gdbpy_execute_objfile_script (const struct extension_language_defn *extlang,
    scoped_restore restire_current_objfile
      = make_scoped_restore (&gdbpy_current_objfile, objfile);

-  int ret = eval_python_command (script, Py_file_input);
-  if (ret != 0)
+  if (eval_python_command (script, Py_file_input) == nullptr)
      gdbpy_print_stack ();
  }

diff --git a/gdb/varobj.c b/gdb/varobj.c
index edd94ea4963..a8c991e50eb 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1365,20 +1365,13 @@ void
  varobj_set_visualizer (struct varobj *var, const char *visualizer)
  {
  #if HAVE_PYTHON
-  PyObject *mainmod;
-
    if (!gdb_python_initialized)
      return;

    gdbpy_enter_varobj enter_py (var);

-  mainmod = PyImport_AddModule ("__main__");
-  gdbpy_ref<> globals
-    = gdbpy_ref<>::new_reference (PyModule_GetDict (mainmod));
-  gdbpy_ref<> constructor (PyRun_String (visualizer, Py_eval_input,
-                                        globals.get (), globals.get ()));
-
-  if (constructor == NULL)
+  auto constructor = eval_python_command (visualizer, Py_eval_input);
+  if (constructor == nullptr)
      {
        gdbpy_print_stack ();
        error (_("Could not evaluate visualizer expression: %s"), visualizer);