This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH 2/8] Python: Fix exception handling in py-record-btrace.c
- From: "Wiederhake, Tim" <tim dot wiederhake at intel dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "Metzger, Markus T" <markus dot t dot metzger at intel dot com>, "brobecker at adacore dot com" <brobecker at adacore dot com>
- Date: Thu, 20 Apr 2017 13:43:17 +0000
- Subject: RE: [PATCH 2/8] Python: Fix exception handling in py-record-btrace.c
- Authentication-results: sourceware.org; auth=none
- Dlp-product: dlpe-windows
- Dlp-reaction: no-action
- Dlp-version: 10.0.102.7
- References: <1492095516-8650-1-git-send-email-tim.wiederhake@intel.com> <1492095516-8650-3-git-send-email-tim.wiederhake@intel.com> <86pog78mm6.fsf@gmail.com>
Hi Yao!
Thanks for your review!
> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: Thursday, April 20, 2017 11:17 AM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>; brobecker@adacore.com
> Subject: Re: [PATCH 2/8] Python: Fix exception handling in py-record-
> btrace.c
>
> 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.
Exception handling in GDB is a little bit unintuitive and I do not claim to grasp it fully. If there is a guarantee that Exception.reason < 0 will always hold true (as the comment in common-exceptions.h claims), then this patch could be dropped from the series.
> > 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.
This change was made for consistency and indeed, it does not change functionality.
> > }
> > 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?
This was the point that got me thinking. If we were to catch an exception here with .reason >= 0, it would not return from the function but continue execution after the catch-block. In this case we would end up with a use-after-xfree in the PyBytes_FromStringAndSize line.
> > 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?
I vaguely remember that GDB used to have some rules about whether or not returning from a function inside a try/catch block was allowed. To play it save I decided to pull that "return" out of the catch to abide to any rules that might still be in place in this regard.
> >
> > 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 [return?] NULL here?
Same as above, I changed it for consistency.
> > }
> > END_CATCH
> >
> > - Py_RETURN_NONE;
> > + return ret;
> > }
>
> --
> Yao (齐尧)
Tim
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928