[binutils-gdb] [gdb/python] Add gdbpy_handle_gdb_exception

[Tom de Vries]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=912bc231ab157fb05e988d8752ea348c3e65111f

commit 912bc231ab157fb05e988d8752ea348c3e65111f
Author: Tom de Vries <tdevries@suse.de>
Date:   Tue Sep 24 13:06:32 2024 +0200

    [gdb/python] Add gdbpy_handle_gdb_exception
    
    I've recently committed two patches:
    - commit 2f8cd40c37a ("[gdb/python] Use GDB_PY_HANDLE_EXCEPTION more often")
    - commit fbf8e4c35c2 ("[gdb/python] Use GDB_PY_SET_HANDLE_EXCEPTION more often")
    which use the macros GDB_PY_HANDLE_EXCEPTION and GDB_PY_SET_HANDLE_EXCEPTION
    more often, with the goal of making things more consistent.
    
    Having done that, I wondered if a better approach could be possible.
    
    Consider GDB_PY_HANDLE_EXCEPTION:
    ...
     /* Use this in a 'catch' block to convert the exception to a Python
        exception and return nullptr.  */
     #define GDB_PY_HANDLE_EXCEPTION(Exception)     \
       do {                                         \
         gdbpy_convert_exception (Exception);       \
         return nullptr;                            \
       } while (0)
    ...
    
    The macro nicely codifies how python handles exceptions:
    - setting an error condition using some PyErr_Set* variant, and
    - returning a value implying that something went wrong
    presumably with the goal that using the macro will mean not accidentally:
    - forgetting to return on error, or
    - returning the wrong value on error.
    
    The problems are that:
    - the macro hides control flow, specifically the return statement, and
    - the macro hides the return value.
    
    For example, when reading somewhere:
    ...
      catch (const gdb_exception &except)
        {
          GDB_PY_HANDLE_EXCEPTION (except);
        }
    ...
    in order to understand what this does, you have to know that the macro
    returns, and that it returns nullptr.
    
    Add a template gdbpy_handle_gdb_exception:
    ...
    template<typename T>
    [[nodiscard]] T
    gdbpy_handle_gdb_exception (T val, const gdb_exception &e)
    {
      gdbpy_convert_exception (e);
      return val;
    }
    ...
    which can be used instead:
    ...
      catch (const gdb_exception &except)
        {
          return gdbpy_handle_gdb_exception (nullptr, except);
        }
    ...
    
    [ Initially I tried this:
    ...
    template<auto val>
    [[nodiscard]] auto
    gdbpy_handle_gdb_exception (const gdb_exception &e)
    {
      gdbpy_convert_exception (e);
      return val;
    }
    ...
    with which the usage is slightly better looking:
    ...
      catch (const gdb_exception &except)
        {
          return gdbpy_handle_gdb_exception<nullptr> (except);
        }
    ...
    but I ran into trouble with older gcc compilers. ]
    
    While still a single statement, we now have it clear:
    - that the statement returns,
    - what value the statement returns.
    
    [ FWIW, this could also be handled by say:
    ...
    -      GDB_PY_HANDLE_EXCEPTION (except);
    +      GDB_PY_HANDLE_EXCEPTION_AND_RETURN_VAL (except, nullptr);
    ...
    but I still didn't find the fact that it returns easy to spot.
    
    Alternatively, this is the simplest form we could use:
    ...
          return gdbpy_convert_exception (e), nullptr;
    ...
    but the pairing would not necessarily survive a copy/paste/edit cycle. ]
    
    Also note how making the value explicit makes it easier to check for
    consistency:
    ...
      catch (const gdb_exception &except)
        {
          return gdbpy_handle_gdb_exception (-1, except);
        }
    
      if (PyErr_Occurred ())
        return -1;
    ...
    given that we do use the explicit constants almost everywhere else.
    
    Compared to using GDB_PY_HANDLE_EXCEPTION, there is the burden now to specify
    the return value, but I assume that this will be generally copy-pasted and
    therefore present no problem.
    
    Also, there's no longer a guarantee that there's an immediate return, but I
    assume that nodiscard making sure that the return value is not silently
    ignored is sufficient mitigation.
    
    For now, re-implement GDB_PY_HANDLE_EXCEPTION and GDB_PY_SET_HANDLE_EXCEPTION
    in terms of gdbpy_handle_gdb_exception.
    
    Follow-up patches will eliminate the macros.
    
    No functional changes.
    
    Tested on x86_64-linux.
    
    Approved-By: Tom Tromey <tom@tromey.com>

Diff:
---
 gdb/python/python-internal.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 82680cdac0a..759e305d40d 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -934,19 +934,17 @@ class gdbpy_gil
 
 /* Use this in a 'catch' block to convert the exception to a Python
    exception and return nullptr.  */
-#define GDB_PY_HANDLE_EXCEPTION(Exception)	\
-  do {						\
-    gdbpy_convert_exception (Exception);	\
-    return nullptr;				\
+#define GDB_PY_HANDLE_EXCEPTION(Exception)				\
+  do {									\
+    return gdbpy_handle_gdb_exception (nullptr, Exception);		\
   } while (0)
 
 /* Use this in a 'catch' block to convert the exception to a Python
    exception and return -1.  */
-#define GDB_PY_SET_HANDLE_EXCEPTION(Exception)				\
-    do {								\
-      gdbpy_convert_exception (Exception);				\
-      return -1;							\
-    } while (0)
+#define GDB_PY_SET_HANDLE_EXCEPTION(Exception)			\
+  do {								\
+    return gdbpy_handle_gdb_exception (-1, Exception);	\
+  } while (0)
 
 int gdbpy_print_python_errors_p (void);
 void gdbpy_print_stack (void);
@@ -1013,6 +1011,18 @@ extern PyObject *gdbpy_gdberror_exc;
 extern void gdbpy_convert_exception (const struct gdb_exception &)
     CPYCHECKER_SETS_EXCEPTION;
 
+ /* Use this in a 'catch' block to convert the exception E to a Python
+    exception and return value VAL to signal that an exception occurred.
+    Typically at the use site, that value will be returned immediately.  */
+
+template<typename T>
+[[nodiscard]] T
+gdbpy_handle_gdb_exception (T val, const gdb_exception &e)
+{
+  gdbpy_convert_exception (e);
+  return val;
+}
+
 int get_addr_from_python (PyObject *obj, CORE_ADDR *addr)
     CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;