[PATCH 1/2] [gdb/python] Handle normalized exception returned by PyErr_Fetch

[PATCH 1/2] [gdb/python] Handle normalized exception returned by PyErr_Fetch

[Tom de Vries]

With python 3.12 and test-case gdb.python/py-block.exp, before commit
a207f6b3a38 ("Rewrite "python" command exception handling"), we had:
...
(gdb) python print (block['nonexistent'])^M
Traceback (most recent call last):^M
  File "<string>", line 1, in <module>^M
KeyError: 'nonexistent'^M
Error while executing Python code.^M
(gdb) PASS: gdb.python/py-block.exp: check nonexistent variable
...
but after the commit we have:
...
(gdb) python print (block['nonexistent'])^M
Python Exception <class 'KeyError'>: 'nonexistent'^M
Error occurred in Python: 'nonexistent'^M
(gdb) FAIL: gdb.python/py-block.exp: check nonexistent variable
...

In contrast, with python 3.6 we have:
...
(gdb) python print (block['nonexistent'])^M
Python Exception <class 'KeyError'>: nonexistent^M
Error occurred in Python: nonexistent^M
(gdb) PASS: gdb.python/py-block.exp: check nonexistent variable
...

The change in the test-case is:
...
-gdb_test "python print (block\['nonexistent'\])" ".*KeyError: 'nonexistent'.*" \
+gdb_test "python print (block\['nonexistent'\])" ".*KeyError.*: nonexistent.*" \
          "check nonexistent variable"
...
which drops the single quotes around the nonexistent string, which matches the
output with python 3.6, but not with python 3.12.

The difference is caused by a difference in the result of PyErr_Fetch.

[ PyErr_Fetch is deprecated in python 3.12, this will be addressed in a
follow-up commit. ]

With python 3.6, we have PyErr_Fetch returning:
...
(gdb) p PyObject_Print(error_value, stderr, 0)
'nonexistent'$3 = 0
(gdb) p PyObject_Print(error_value, stderr, 1)
nonexistent$4 = 0
...
but with python 3.12, we have instead:
...
(gdb) p (int)PyObject_Print(error_value,stderr, 0)
KeyError('nonexistent')$3 = 0
(gdb) p (int)PyObject_Print(error_value,stderr, 1)
'nonexistent'$4 = 0
...

In more detail, with python 3.12 we get a normalized exception, so the
error_value is an object of class KeyError.  With python 3.6, error_value is
an unnormalized exception, meaning not of class KeyError.

Apparantly we rely here on PyErr_Fetch to return an unnormalized exception.

Fix this by pretending to have an unnormalized exception in
gdbpy_err_fetch::to_string.

Tested on aarch64-linux.

PR python/31425
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31425
---
 gdb/python/py-utils.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 9382eb62a5f..8eee33f89bb 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -196,7 +196,19 @@ gdbpy_err_fetch::to_string () const
      gdb.GdbError ("message").  */
 
   if (m_error_value.get () != nullptr && m_error_value.get () != Py_None)
-    return gdbpy_obj_to_string (m_error_value.get ());
+    {
+      if ((PyObject *)Py_TYPE (m_error_value.get ()) == m_error_type.get ())
+	{
+	  /* Detected a normalized exception.  */
+	  PyObject *args = PyException_GetArgs (m_error_value.get ());
+	  if (PyTuple_Size (args) == 1)
+	    {
+	      /* Pretend to be looking at an unnormalized exception.  */
+	      return gdbpy_obj_to_string (PyTuple_GetItem (args, 0));
+	    }
+	}
+      return gdbpy_obj_to_string (m_error_value.get ());
+    }
   else
     return gdbpy_obj_to_string (m_error_type.get ());
 }

base-commit: 1485a3fb63619cced99dd7a4a043cf01a0f423d9
-- 
2.35.3

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

[Tom de Vries]

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(+)

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
-- 
2.35.3

Re: [PATCH 1/2] [gdb/python] Handle normalized exception returned by PyErr_Fetch

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +	  /* Detected a normalized exception.  */
Tom> +	  PyObject *args = PyException_GetArgs (m_error_value.get ());

According to:

https://docs.python.org/3/c-api/exceptions.html

... PyException_GetArgs returns a new reference; so this code leaks
memory.

I think args should be a gdbpy_ref<> here.

Tom

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

[Tom Tromey]

>>>>> "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

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

[Tom de Vries]

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