[v2,1/4,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.
---
 gdb/python/python-internal.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)


base-commit: 934fcc7c916ad2876515a099ee09fa40dc08b489
  

On 8/28/24 10:20, Tom de Vries wrote:
> 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.
> 

Ping.

Thanks,
- Tom

> Tested on x86_64-linux.
> ---
>   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 bf3ab67ea74..da357e71e8d 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;
>   
> 
> base-commit: 934fcc7c916ad2876515a099ee09fa40dc08b489
  

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

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

Tom> Ping.

Thanks for doing this.
These are all ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
  

On 9/23/24 20:06, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
>>> 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.
> 
> Tom> Ping.
> 
> Thanks for doing this.
> These are all ok.
> 
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks for the review, pushed.

- Tom