Re: [PATCH] Add Frame.read_register to Python API

[Doug Evans <dje at google dot com>]

On Thu, Jul 24, 2014 at 9:41 AM, Doug Evans <dje@google.com> wrote:
> On Mon, Jul 14, 2014 at 9:13 AM, Alexander Smundak <asmundak@google.com> wrote:
>> Ping.
>> It has been a month since I have responded to the reviewer's comments.
>> Perhaps someone can take a look at this patch?
>>
>> On Mon, Jul 7, 2014 at 1:59 PM, Alexander Smundak <asmundak@google.com> wrote:
>>> Ping.
>>>
>>> On Mon, Jun 30, 2014 at 9:22 AM, Alexander Smundak <asmundak@google.com> wrote:
>>>> Ping
>>>>
>>>> On Mon, Jun 23, 2014 at 3:46 PM, Alexander Smundak <asmundak@google.com> wrote:
>>>>> Ping
>>>>>
>>>>> On Wed, Jun 18, 2014 at 10:18 AM, Alexander Smundak <asmundak@google.com> wrote:
>>>>>> Ping.
>>>>>>
>>>>>> On Wed, Jun 11, 2014 at 4:52 PM, Alexander Smundak <asmundak@google.com> wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> Should I mail these two as a single patch or as two separate patches?
>>>>>>>
>>>>>>>> Alexander> +      FRAPY_REQUIRE_VALID (self, frame);
>>>>>>>> Alexander> +      if (!PyArg_ParseTuple (args, "i", &regnum))
>>>>>>>> Alexander> +    {
>>>>>>>> Alexander> +      const char *regnum_str;
>>>>>>>> Alexander> +      PyErr_Clear();  /* Clear PyArg_ParseTuple failure above.  */
>>>>>>>> Alexander> +      if (PyArg_ParseTuple (args, "s", &regnum_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.
>>>>>>>
>>>>>>> Did that, and then started doubting whether it is really necessary to read
>>>>>>> a register by its (very arch-specific) number. The new version supports
>>>>>>> reading the register by the name. Another change is that it now throws
>>>>>>> an exception if the name is wrong.
>>>>>>>
>>>>>>>> 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.
>>>>>>> IMHO it's borderline arch-specific -- it is runnable on any platform,
>>>>>>> although it will not be testing much on any but x86-64. There hasn't
>>>>>>> been any arch-specific tests for Python so far, so I am not sure what to do.
>>>>>>>
>>>>>>> Here's the new version (style violations have been addressed, too):
>>>>>>>
>>>>>>> The ability to read registers is needed to use Frame Filter API to
>>>>>>> display the frames created by JIT compilers.
>>>>>>>
>>>>>>> gdb/Changelog
>>>>>>> 2014-06-11  Sasha Smundak  <asmundak@google.com>
>>>>>>>
>>>>>>> * python/py-frame.c (frapy_read_register): New function.
>>>>>>>
>>>>>>> 2014-06-11  Sasha Smundak  <asmundak@google.com>
>>>>>>>
>>>>>>> * python.texi (Frames in Python): Add read_register description.
>>>>>>>
>>>>>>> 2014-06-11  Sasha Smundak  <asmundak@google.com>
>>>>>>>
>>>>>>> * gdb.python/py-frame.exp: Test Frame.read_register.
>
> Hi.
>
> For myself, I don't like to step in once another reviewer has engaged the patch.
> [Not that I won't.  Only that I don't want to usurp someone else's
> review - it's hard to make sure one has sufficiently covered
> everything the other reviewer raised as issues.]
>
> Eli, I realize the doc parts are ok.  Thanks!
>
> Tom: Anything else that needs to be done?

Ping.

If I'm given the "OK" to take over review of this patch, that's cool.
I'd just rather not do so without an explicit handoff.