[1/4,gdb/python] Add gdbpy_handle_gdb_exception
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
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, T val>
[[nodiscard]] T
gdbpy_handle_gdb_exception (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_t, nullptr> (except);
}
...
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. ]
Compared to using GDB_PY_HANDLE_EXCEPTION, there is the burden now to specify
both return type and 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.
---
gdb/python/python-internal.h | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
base-commit: fbf8e4c35c292620f8d25096b4341c3b5caa90ee
Comments
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> The problems are that:
Tom> - the macro hides control flow, specifically the return statement, and
Tom> - the macro hides the return value.
Yeah. I think having the macro here was more convenient before we used
C++ exceptions, because back then there was also an embedded 'if'.
Tom> return gdbpy_handle_gdb_exception<nullptr_t, nullptr> (except);
This particular formulation is kind of wordy, IMO. The return value can
be deduced from the return type, because Python follows a few
conventions here -- there's no need to repeat this at every call point.
Instead this could use a per-type constant, similar to
gdbpy_method_format.
Unfortunately I don't think the type can be deduced from the 'return'
context ... that would be really convenient if it were possible.
Tom
On 8/27/24 20:40, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>
> Tom> The problems are that:
> Tom> - the macro hides control flow, specifically the return statement, and
> Tom> - the macro hides the return value.
>
> Yeah. I think having the macro here was more convenient before we used
> C++ exceptions, because back then there was also an embedded 'if'.
>
> Tom> return gdbpy_handle_gdb_exception<nullptr_t, nullptr> (except);
>
> This particular formulation is kind of wordy, IMO. The return value can
> be deduced from the return type, because Python follows a few
> conventions here -- there's no need to repeat this at every call point.
>
> Instead this could use a per-type constant, similar to
> gdbpy_method_format.
>
> Unfortunately I don't think the type can be deduced from the 'return'
> context ... that would be really convenient if it were possible.
I think deducing the return-value-indicating-failure from the return
type is fine, if we use that style everywhere else:
...
catch (const gdb_exception &except)
{
return gdbpy_handle_gdb_exception<int> (except);
}
if (PyErr_Occurred ())
return gdbpy_fail<int>;
...
Otherwise we have:
...
catch (const gdb_exception &except)
{
return gdbpy_handle_gdb_exception<int> (except);
}
if (PyErr_Occurred ())
return -1;
...
which makes consistency checking harder.
So, I've tried the other direction, deducing the return type from the
return value:
...
catch (const gdb_exception &except)
{
return gdbpy_handle_gdb_exception (-1, except);
}
if (PyErr_Occurred ())
return -1;
...
I've submitted this as a v2 (
https://sourceware.org/pipermail/gdb-patches/2024-August/211374.html ).
I hope I've managed to convince you, but perhaps this is a preference
thing, I'm not sure.
Anyway, if you object I can do a v3 using the
"gdbpy_handle_gdb_exception<int> (except)" style.
Thanks,
- Tom
@@ -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_t, 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<int, -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 the value to signal that an exception occurred.
+ Typically at the use site, that value will be returned immediately. */
+
+template<typename T, T val>
+[[nodiscard]] T
+gdbpy_handle_gdb_exception (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;