Re: [PATCH 2/8] Python: Fix exception handling in py-record-btrace.c

[Yao Qi <qiyaoltc at gmail dot com>]

Tim Wiederhake <tim.wiederhake@intel.com> writes:

> GDB_PY_HANDLE_EXCEPTION does not handle all exceptions.  Replace with call to
> gdbpy_convert_exception.
>

I don't see why GDB_PY_HANDLE_EXCEPTION doesn't handle all exceptions.

GDB_PY_HANDLE_EXCEPTION is expanded to

    if (Exception.reason < 0)			\
      {						\
	gdbpy_convert_exception (Exception);	\
        return NULL;				\
      }

in CATCH block, .reason must be less than 0.

> 2017-04-13  Tim Wiederhake  <tim.wiederhake@intel.com>
>
> gdb/ChangeLog:
> 	python/py-record-btrace.c (btpy_insn_sal, btpy_insn_data,
> 	btpy_insn_decode, recpy_bt_goto): Handle all exceptions.
>
> ---
>  gdb/python/py-record-btrace.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
> index 0f8d7ef..9d79e2b 100644
> --- a/gdb/python/py-record-btrace.c
> +++ b/gdb/python/py-record-btrace.c
> @@ -227,7 +227,7 @@ btpy_insn_sal (PyObject *self, void *closure)
>      }
>    CATCH (except, RETURN_MASK_ALL)
>      {
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      gdbpy_convert_exception (except);

I don't see any functionality changed.

>      }
>    END_CATCH
>  
> @@ -320,10 +320,14 @@ btpy_insn_data (PyObject *self, void *closure)
>    CATCH (except, RETURN_MASK_ALL)
>      {
>        xfree (buffer);
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      buffer = NULL;
> +      gdbpy_convert_exception (except);
>      }
>    END_CATCH
>  
> +  if (buffer == NULL)
> +    return NULL;
> +

GDB_PY_HANDLE_EXCEPTION return NULL in CATCH block.  The changed code is
equivalent to the original one, no?

>    object = PyBytes_FromStringAndSize ((const char*) buffer, insn->size);
>    xfree (buffer);
>  
> @@ -348,6 +352,7 @@ btpy_insn_decode (PyObject *self, void *closure)
>    const struct btrace_insn *insn;
>    struct btrace_insn_iterator iter;
>    string_file strfile;
> +  int length = 0;
>  
>    BTPY_REQUIRE_VALID_INSN (obj, iter);
>  
> @@ -364,15 +369,16 @@ btpy_insn_decode (PyObject *self, void *closure)
>  
>    TRY
>      {
> -      gdb_print_insn (target_gdbarch (), insn->pc, &strfile, NULL);
> +      length = gdb_print_insn (target_gdbarch (), insn->pc, &strfile, NULL);
>      }
>    CATCH (except, RETURN_MASK_ALL)
>      {
>        gdbpy_convert_exception (except);
> -      return NULL;
>      }
>    END_CATCH
>  
> +  if (length == 0)
> +    return NULL;

This change doesn't match the changelog entry and commit log.  Why can't
we return NULL in CATCH?

>  
>    return PyBytes_FromString (strfile.string ().c_str ());
>  }
> @@ -871,6 +877,7 @@ recpy_bt_goto (PyObject *self, PyObject *args)
>  {
>    struct thread_info * const tinfo = find_thread_ptid (inferior_ptid);
>    const btpy_object *obj;
> +  PyObject *ret = NULL;
>  
>    if (tinfo == NULL || btrace_is_empty (tinfo))
>  	return PyErr_Format (gdbpy_gdb_error, _("Empty branch trace."));
> @@ -891,14 +898,17 @@ recpy_bt_goto (PyObject *self, PyObject *args)
>  	target_goto_record_end ();
>        else
>  	target_goto_record (obj->number);
> +
> +      Py_INCREF (Py_None);
> +      ret = Py_None;
>      }
>    CATCH (except, RETURN_MASK_ALL)
>      {
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      gdbpy_convert_exception (except);

Why can't we remove NULL here?

>      }
>    END_CATCH
>  
> -  Py_RETURN_NONE;
> +  return ret;
>  }

-- 
Yao (齐尧)