This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add Frame.read_register to Python API
- From: Tom Tromey <tromey at redhat dot com>
- To: Alexander Smundak <asmundak at google dot com>
- Cc: Eli Zaretskii <eliz at gnu dot org>, gdb-patches <gdb-patches at sourceware dot org>
- Date: Wed, 11 Jun 2014 13:11:29 -0600
- Subject: Re: [PATCH] Add Frame.read_register to Python API
- Authentication-results: sourceware.org; auth=none
- References: <CAHQ51u4gJRVFXNnUykc6EmCCFGd4O3+LJmpjFGMZ9AeQSvQCuQ at mail dot gmail dot com> <83oay128ca dot fsf at gnu dot org> <CAHQ51u4ugn--HzUS0r8sMyv14LcasL5vY_EUdOyJ1wXaX+WLpw at mail dot gmail dot com>
>>>>> "Alexander" == Alexander Smundak <asmundak@google.com> writes:
Thanks for your patch.
There's a few nits but nothing very serious.
Alexander> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
Alexander> index 4688783..14564eb 100644
Alexander> --- a/gdb/doc/python.texi
Alexander> +++ b/gdb/doc/python.texi
Alexander> @@ -2046,9 +2046,10 @@ class InlinedFrameDecorator(FrameDecorator):
Alexander> def __init__(self, fobj):
Alexander> super(InlinedFrameDecorator, self).__init__(fobj)
Alexander> + self.fobj = fobj
Alexander> def function(self):
Alexander> - frame = fobj.inferior_frame()
Alexander> + frame = self.fobj.inferior_frame()
Alexander> name = str(frame.name())
I think this is a nice fix but it seems unrelated to the patch at hand.
Alexander> @defun Frame.find_sal ()
Alexander> -Return the frame's symtab and line object.
Alexander> +Return the frame's @code{gdb.Symtab_and_line} object.
Likewise.
Alexander> +/* Implementation of gdb.Frame.read_register (self, register) -> gdb.Value.
Alexander> + Returns the value of a register in this frame. */
Alexander> +static PyObject *
Alexander> +frapy_read_register (PyObject *self, PyObject *args)
Alexander> +{
The gdb style requests a blank line between the intro comment and the
function.
Alexander> + struct frame_info *frame;
Alexander> + volatile struct gdb_exception except;
Alexander> + int regnum = -1;
Alexander> + struct value *val = NULL;
Alexander> + TRY_CATCH (except, RETURN_MASK_ALL)
Alexander> + {
... and also a blank line between variable declarations and code
(wherever this occurs -- there's a case in the patch in a nested block).
Alexander> + FRAPY_REQUIRE_VALID (self, frame);
Alexander> + if (!PyArg_ParseTuple (args, "i", ®num))
Alexander> + {
Alexander> + const char *regnum_str;
Alexander> + PyErr_Clear(); /* Clear PyArg_ParseTuple failure above. */
Alexander> + if (PyArg_ParseTuple (args, "s", ®num_str))
Alexander> + {
Alexander> + regnum = user_reg_map_name_to_regnum (get_frame_arch (frame),
Alexander> + regnum_str,
Alexander> + strlen (regnum_str));
Alexander> + }
Alexander> + }
I tend to think this would be clearer if the arguments were only parsed
once and then explicit type checks were applied to the resulting object.
Alexander> + return val ? value_to_value_object (val) : NULL;
gdb style requests an explicit NULL check, like "return val != NULL ? ...".
Alexander> +# On x86-64, PC is register 16.
Alexander> +gdb_test "python print ('result = %s' % ((f0.architecture().name() != 'i386:x86-64') or f0.read_register('pc') == f0.read_register(16)))" \
Alexander> + "True" \
Alexander> + "test Frame.read_register(regnum)"
A test that is arch-specific needs to be conditionalized somehow.
Tom