diff mbox series

[2/2,gdb/python] Handle deprecation of PyErr_{Fetch, Restore} in 3.12

Message ID 20240304160347.31182-2-tdevries@suse.de
State Superseded
Headers
Series [1/2,gdb/python] Handle normalized exception returned by PyErr_Fetch |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Testing failed
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Testing failed

Commit Message

Tom de Vries March 4, 2024, 4:03 p.m. UTC 
  Starting python version 3.12, PyErr_Fetch and PyErr_Restore are deprecated.

Use PyErr_GetRaisedException and PyErr_SetRaisedException instead, for
python >= 3.12.

This causes a regression in gdb.python/py-mi-cmd.exp:
...
-pycmd exp^M
^error,msg="<class 'gdb.GdbError'>"^M
(gdb) ^M
FAIL: gdb.python/py-mi-cmd.exp: -pycmd exp (unexpected output)
...
where a gdbError with missing message goes undetected because it's in
unnormalized form.

Fix this by detecting the unnormalized from in gdbpy_err_fetch::to_string.

Tested on aarch64-linux.
---
 gdb/python/py-utils.c        |  7 +++++++
 gdb/python/python-internal.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

Comments

Tom Tromey March 4, 2024, 6:48 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +  if (type_matches (gdbpy_gdberror_exc) && m_error_value.get () == nullptr)
Tom> +    {
Tom> +      /* We have an unnormalized gdb.GdbError with missing message, make sure
Tom> +	 gdbpy_handle_exception flags it as such.*/
Tom> +      return nullptr;
Tom> +    }

The to_string doc comment says:

  If the result is NULL a python error occurred, the
     caller must clear it.  */

.. is this true for this null return?  It doesn't seem so to me but I
may not really understand what's happening here.

Tom> +    PyObject *args = PyException_GetArgs (exc);

I think there's a memory leak here.

I don't really know much about the Python 3.12 change here, but are all
the other fields of gdbpy_err_fetch even needed?

Tom>    gdbpy_ref<> m_error_type, m_error_value, m_error_traceback;
Tom> +#if PY_VERSION_HEX >= 0x030c0000
Tom> +  gdbpy_ref<> m_exc;
Tom> +#endif

... like is there still code that really needs anything other than m_exc?

Tom
Tom de Vries March 8, 2024, 3:36 p.m. UTC | #2 
On 3/4/24 19:48, Tom Tromey wrote:
> I don't really know much about the Python 3.12 change here, but are all
> the other fields of gdbpy_err_fetch even needed?
> 
> Tom>    gdbpy_ref<> m_error_type, m_error_value, m_error_traceback;
> Tom> +#if PY_VERSION_HEX >= 0x030c0000
> Tom> +  gdbpy_ref<> m_exc;
> Tom> +#endif
> 
> ... like is there still code that really needs anything other than m_exc?

Thanks for the reviews.

I started out following up on the review comments, but at the end of 
that process I was not satisfied with the result.

I realized that I'm making this harder than necessary by trying to mimic 
unnormalized behaviour in the normalized case, and it's much better to 
standardize on the normalized version, since that's also what 
PyErr_GetRaisedException gets us.

So, I've submitted a v2 that takes that approach, and indeed only uses 
m_exc in the 3.12 case.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 8eee33f89bb..23afb9be353 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -195,6 +195,13 @@  gdbpy_err_fetch::to_string () const
      Using str (aka PyObject_Str) will fetch the error message from
      gdb.GdbError ("message").  */
 
+  if (type_matches (gdbpy_gdberror_exc) && m_error_value.get () == nullptr)
+    {
+      /* We have an unnormalized gdb.GdbError with missing message, make sure
+	 gdbpy_handle_exception flags it as such.*/
+      return nullptr;
+    }
+
   if (m_error_value.get () != nullptr && m_error_value.get () != Py_None)
     {
       if ((PyObject *)Py_TYPE (m_error_value.get ()) == m_error_type.get ())
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index c68aff5340e..40084d76502 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -640,12 +640,36 @@  class gdbpy_err_fetch
 
   gdbpy_err_fetch ()
   {
+#if PY_VERSION_HEX < 0x030c0000
     PyObject *error_type, *error_value, *error_traceback;
 
     PyErr_Fetch (&error_type, &error_value, &error_traceback);
     m_error_type.reset (error_type);
     m_error_value.reset (error_value);
     m_error_traceback.reset (error_traceback);
+#else
+    /* PyErr_Fetch is deprecated in python 3.12, use PyErr_GetRaisedException
+       instead.  */
+    PyObject *exc = PyErr_GetRaisedException ();
+    m_exc.reset (exc);
+    if (exc == nullptr)
+      {
+	m_error_type.reset (nullptr);
+	m_error_value.reset (nullptr);
+	m_error_traceback.reset (nullptr);
+	return;
+      }
+
+    m_error_type = gdbpy_ref<>::new_reference ((PyObject *)Py_TYPE (exc));
+
+    PyObject *args = PyException_GetArgs (exc);
+    if (PyTuple_Size (args) >= 1)
+      m_error_value = gdbpy_ref<>::new_reference (PyTuple_GetItem (args, 0));
+    else
+      m_error_value.reset (nullptr);
+
+    m_error_traceback.reset (PyException_GetTraceback (exc));
+#endif
   }
 
   /* Call PyErr_Restore using the values stashed in this object.
@@ -654,9 +678,18 @@  class gdbpy_err_fetch
 
   void restore ()
   {
+#if PY_VERSION_HEX < 0x030c0000
     PyErr_Restore (m_error_type.release (),
 		   m_error_value.release (),
 		   m_error_traceback.release ());
+#else
+    /* PyErr_Restore is deprecated in python 3.12, use PyErr_SetRaisedException
+       instead.  */
+    m_error_type.reset (nullptr);
+    m_error_value.reset (nullptr);
+    m_error_traceback.reset (nullptr);
+    PyErr_SetRaisedException (m_exc.release ());
+#endif
   }
 
   /* Return the string representation of the exception represented by
@@ -688,6 +721,9 @@  class gdbpy_err_fetch
 private:
 
   gdbpy_ref<> m_error_type, m_error_value, m_error_traceback;
+#if PY_VERSION_HEX >= 0x030c0000
+  gdbpy_ref<> m_exc;
+#endif
 };
 
 /* Called before entering the Python interpreter to install the