Remove 'if' from GDB_PY_HANDLE_EXCEPTION

  This removes the embedded 'if' from GDB_PY_HANDLE_EXCEPTION and
GDB_PY_SET_HANDLE_EXCEPTION.  I believe this 'if' was necessary with
the old gdb try/catch macros, but it no longer is: now these are only
ever called from a 'catch' block, where it's already known that an
exception was thrown.
---
 gdb/python/python-internal.h | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
  

On 12/22/23 13:44, Tom Tromey wrote:
> This removes the embedded 'if' from GDB_PY_HANDLE_EXCEPTION and
> GDB_PY_SET_HANDLE_EXCEPTION.  I believe this 'if' was necessary with
> the old gdb try/catch macros, but it no longer is: now these are only
> ever called from a 'catch' block, where it's already known that an
> exception was thrown.

In infpy_write_memory for instance, GDB_PY_HANDLE_EXCEPTION is not
called within the catch block.  Is it normal?

> ---
>  gdb/python/python-internal.h | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 14e15574685..fc8430ff35a 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -800,22 +800,16 @@ class gdbpy_gil
>     exception.  */
>  #define GDB_PY_HANDLE_EXCEPTION(Exception)	\
>    do {						\
> -    if (Exception.reason < 0)			\
> -      {						\
> -	gdbpy_convert_exception (Exception);	\
> -	return NULL;				\
> -      }						\
> +    gdbpy_convert_exception (Exception);	\
> +    return NULL;				\
>    } while (0)
>  
>  /* Use this after a TRY_EXCEPT to throw the appropriate Python

You could update those comments, TRY_EXCEPT is stale too.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> In infpy_write_memory for instance, GDB_PY_HANDLE_EXCEPTION is not
Simon> called within the catch block.  Is it normal?

Nope, oops.  Here's v2, that fixes this and also removes an unnecessary
return -- which is kind of the main motivation of this change, the 'if'
sometimes results in the compiler thinking the macro doesn't return,
whereas it does.

Tom

commit 0527427edccf5328449bf16be84d209e3ae750ff
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Dec 22 11:43:26 2023 -0700

    Remove 'if' from GDB_PY_HANDLE_EXCEPTION
    
    This removes the embedded 'if' from GDB_PY_HANDLE_EXCEPTION and
    GDB_PY_SET_HANDLE_EXCEPTION.  I believe this 'if' was necessary with
    the old gdb try/catch macros, but it no longer is: these should only
    ever be called from a 'catch' block, where it's already known that an
    exception was thrown.
    
    Simon pointed out, though, that in a few spots, these were in facts
    called outside of 'catch' blocks.  This patch cleans up these spots.
    I also found one spot where a redundant 'return nullptr' could be
    removed.

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 0e43a001cff..95782444c4a 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -589,7 +589,6 @@ bppy_set_condition (PyObject *self, PyObject *newvalue, void *closure)
   gdb::unique_xmalloc_ptr<char> exp_holder;
   const char *exp = NULL;
   gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
-  struct gdb_exception except;
 
   BPPY_SET_REQUIRE_VALID (self_bp);
 
@@ -615,11 +614,9 @@ bppy_set_condition (PyObject *self, PyObject *newvalue, void *closure)
     }
   catch (gdb_exception &ex)
     {
-      except = std::move (ex);
+      GDB_PY_SET_HANDLE_EXCEPTION (ex);
     }
 
-  GDB_PY_SET_HANDLE_EXCEPTION (except);
-
   return 0;
 }
 
@@ -657,7 +654,6 @@ static int
 bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure)
 {
   gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
-  struct gdb_exception except;
 
   BPPY_SET_REQUIRE_VALID (self_bp);
 
@@ -684,11 +680,9 @@ bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure)
     }
   catch (gdb_exception &ex)
     {
-      except = std::move (ex);
+      GDB_PY_SET_HANDLE_EXCEPTION (ex);
     }
 
-  GDB_PY_SET_HANDLE_EXCEPTION (except);
-
   return 0;
 }
 
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 834f7274d1e..5dcb17479f1 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -586,7 +586,6 @@ static PyObject *
 infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 {
   inferior_object *inf = (inferior_object *) self;
-  struct gdb_exception except;
   Py_ssize_t buf_len;
   const gdb_byte *buffer;
   CORE_ADDR addr, length;
@@ -625,11 +624,9 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
     }
   catch (gdb_exception &ex)
     {
-      except = std::move (ex);
+      GDB_PY_HANDLE_EXCEPTION (ex);
     }
 
-  GDB_PY_HANDLE_EXCEPTION (except);
-
   Py_RETURN_NONE;
 }
 
@@ -645,7 +642,6 @@ static PyObject *
 infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
 {
   inferior_object *inf = (inferior_object *) self;
-  struct gdb_exception except;
   CORE_ADDR start_addr, length;
   static const char *keywords[] = { "address", "length", "pattern", NULL };
   PyObject *start_addr_obj, *length_obj;
@@ -702,11 +698,9 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
     }
   catch (gdb_exception &ex)
     {
-      except = std::move (ex);
+      GDB_PY_HANDLE_EXCEPTION (ex);
     }
 
-  GDB_PY_HANDLE_EXCEPTION (except);
-
   if (found)
     return gdb_py_object_from_ulongest (found_addr).release ();
   else
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 65c4c6c12cd..995397e7f0d 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -214,7 +214,6 @@ thpy_get_ptid_string (PyObject *self, void *closure)
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
-      return nullptr;
     }
 }
 
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 151b8801b88..37695460717 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -1046,7 +1046,6 @@ get_field_type (PyObject *field)
 static PyObject *
 valpy_getitem (PyObject *self, PyObject *key)
 {
-  struct gdb_exception except;
   value_object *self_value = (value_object *) self;
   gdb::unique_xmalloc_ptr<char> field;
   struct type *base_class_type = NULL, *field_type = NULL;
@@ -1178,11 +1177,9 @@ valpy_getitem (PyObject *self, PyObject *key)
     }
   catch (gdb_exception &ex)
     {
-      except = std::move (ex);
+      GDB_PY_HANDLE_EXCEPTION (ex);
     }
 
-  GDB_PY_HANDLE_EXCEPTION (except);
-
   return result;
 }
 
@@ -1677,7 +1674,6 @@ valpy_absolute (PyObject *self)
 static int
 valpy_nonzero (PyObject *self)
 {
-  struct gdb_exception except;
   value_object *self_value = (value_object *) self;
   struct type *type;
   int nonzero = 0; /* Appease GCC warning.  */
@@ -1697,14 +1693,12 @@ valpy_nonzero (PyObject *self)
     }
   catch (gdb_exception &ex)
     {
-      except = std::move (ex);
+      /* This is not documented in the Python documentation, but if
+	 this function fails, return -1 as slot_nb_nonzero does (the
+	 default Python nonzero function).  */
+      GDB_PY_SET_HANDLE_EXCEPTION (ex);
     }
 
-  /* This is not documented in the Python documentation, but if this
-     function fails, return -1 as slot_nb_nonzero does (the default
-     Python nonzero function).  */
-  GDB_PY_SET_HANDLE_EXCEPTION (except);
-
   return nonzero;
 }
 
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 7c05007cbab..98ed3f8b8cb 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -800,26 +800,20 @@ class gdbpy_gil
   PyGILState_STATE m_state;
 };
 
-/* Use this after a TRY_EXCEPT to throw the appropriate Python
-   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 {						\
-    if (Exception.reason < 0)			\
-      {						\
-	gdbpy_convert_exception (Exception);	\
-	return NULL;				\
-      }						\
+    gdbpy_convert_exception (Exception);	\
+    return nullptr;				\
   } while (0)
 
-/* Use this after a TRY_EXCEPT to throw the appropriate Python
-   exception.  This macro is for use inside setter functions.  */
+/* 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 {								\
-      if (Exception.reason < 0)						\
-	{								\
-	  gdbpy_convert_exception (Exception);				\
-	  return -1;							\
-	}								\
+      gdbpy_convert_exception (Exception);				\
+      return -1;							\
     } while (0)
 
 int gdbpy_print_python_errors_p (void);
  

Simon> In infpy_write_memory for instance, GDB_PY_HANDLE_EXCEPTION is not
Simon> called within the catch block.  Is it normal?

> Nope, oops.  Here's v2, that fixes this and also removes an unnecessary
> return -- which is kind of the main motivation of this change, the 'if'
> sometimes results in the compiler thinking the macro doesn't return,
> whereas it does.

>     Remove 'if' from GDB_PY_HANDLE_EXCEPTION
    
>     This removes the embedded 'if' from GDB_PY_HANDLE_EXCEPTION and
>     GDB_PY_SET_HANDLE_EXCEPTION.  I believe this 'if' was necessary with
>     the old gdb try/catch macros, but it no longer is: these should only
>     ever be called from a 'catch' block, where it's already known that an
>     exception was thrown.
    
>     Simon pointed out, though, that in a few spots, these were in facts
>     called outside of 'catch' blocks.  This patch cleans up these spots.
>     I also found one spot where a redundant 'return nullptr' could be
>     removed.

I'm checking this in.

Tom