From 912bc231ab157fb05e988d8752ea348c3e65111f Mon Sep 17 00:00:00 2001 From: Tom de Vries Date: Tue, 24 Sep 2024 13:06:32 +0200 Subject: [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 [[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 [[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 (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 --- gdb/python/python-internal.h | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) (limited to 'gdb/python') diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h index 82680cd..759e305 100644 --- a/gdb/python/python-internal.h +++ b/gdb/python/python-internal.h @@ -934,19 +934,17 @@ private: /* 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 +[[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; -- cgit v1.1