Add Frame.read_register to Python API

  > 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.
  

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.
  

> Date: Wed, 18 Jun 2014 10:18:47 -0700
> From: Alexander Smundak <asmundak@google.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches <gdb-patches@sourceware.org>
> 
> Ping.

I already approved the documentation parts, with the 2 minor changes I
mentioned.

Thanks.
  

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.
  

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.
  

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.
  

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.
  

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?
  

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.
  

Doug> Ping.

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

I don't plan to look at it.

Tom
  

Alexander Smundak writes:
 > > 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?

Two separate patches.
Thanks!

 > > 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.

Yeah, I think passing register names is the way to go.
At least for now.  If it ever proves useful we can extend the API
to also accept integers, but let's leave them out for now.

IWBN to export an API to go from register name to number and back,
but that's a topic for another patch, and nothing you need to
work on for the task at hand (better java backtraces).

 > > 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.

The gdb testsuite has a canonical way to perform this test.

See, e.g., testsuite/gdb.dwarf2/dw2-restrict.exp:

if {![istarget x86_64-*] || ![is_lp64_target]} {
    return 0
}

We should probably just use something like that here.

 > 
 > 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.
 > diff --git a/gdb/NEWS b/gdb/NEWS
 > index 1397e8b..3db458b 100644
 > --- a/gdb/NEWS
 > +++ b/gdb/NEWS
 > @@ -3,6 +3,9 @@
 >  
 >  *** Changes since GDB 7.7
 >  
 > +* Python Scripting
 > +  You can now access frame registers from Python scripts.
 > +
 >  * New command line options
 >  
 >  -D data-directory

This will need to be updated to move to "Changes since GDB 7.8".

 > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
 > index 4688783..3cb6bf8 100644
 > --- a/gdb/doc/python.texi
 > +++ b/gdb/doc/python.texi
 > @@ -3589,6 +3589,13 @@ Return the frame's symtab and line object.
 >  @xref{Symbol Tables In Python}.
 >  @end defun
 >  
 > +@defun Frame.read_register (register)
 > +Return the value of @var{register} in this frame.  The @var{register}
 > +argument must be a string (e.g., @code{'sp'} or @code{'rax'}).
 > +Returns a @code{Gdb.Value} object.  Throws an exception if @var{register}
 > +does not exist.
 > +@end defun
 > +
 >  @defun Frame.read_var (variable @r{[}, block@r{]})
 >  Return the value of @var{variable} in this frame.  If the optional
 >  argument @var{block} is provided, search for the variable from that
 > diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
 > index 77077d3..73bffb1 100644
 > --- a/gdb/python/py-frame.c
 > +++ b/gdb/python/py-frame.c
 > @@ -28,6 +28,7 @@
 >  #include "python-internal.h"
 >  #include "symfile.h"
 >  #include "objfiles.h"
 > +#include "user-regs.h"
 >  
 >  typedef struct {
 >    PyObject_HEAD
 > @@ -235,6 +236,39 @@ frapy_pc (PyObject *self, PyObject *args)
 >    return gdb_py_long_from_ulongest (pc);
 >  }
 >  
 > +/* Implementation of gdb.Frame.read_register (self, register) -> gdb.Value.
 > +   Returns the value of a register in this frame.  */
 > +
 > +static PyObject *
 > +frapy_read_register (PyObject *self, PyObject *args)
 > +{
 > +  struct frame_info *frame;
 > +  volatile struct gdb_exception except;
 > +  int regnum = -1;
 > +  struct value *val = NULL;
 > +  const char *regnum_str;
 > +
 > +  if (!PyArg_ParseTuple (args, "s", &regnum_str))
 > +    return NULL;
 > +
 > +  TRY_CATCH (except, RETURN_MASK_ALL)
 > +    {

Can you move the definitions of frame, regnum, regnum_str to here?

 > +      FRAPY_REQUIRE_VALID (self, frame);
 > +
 > +      regnum = user_reg_map_name_to_regnum (get_frame_arch (frame),
 > +                                            regnum_str,
 > +                                            strlen (regnum_str));
 > +      if (regnum >= 0)
 > +        val = value_of_register (regnum, frame);
 > +
 > +      if (val == NULL)
 > +        PyErr_SetString (PyExc_ValueError, _("Unknown register."));
 > +    }
 > +  GDB_PY_HANDLE_EXCEPTION (except);
 > +
 > +  return val == NULL ? NULL : value_to_value_object (val);
 > +}
 > +
 >  /* Implementation of gdb.Frame.block (self) -> gdb.Block.
 >     Returns the frame's code block.  */
 >  
 > @@ -674,6 +708,9 @@ Return the reason why it's not possible to find frames older than this." },
 >    { "pc", frapy_pc, METH_NOARGS,
 >      "pc () -> Long.\n\
 >  Return the frame's resume address." },
 > +  { "read_register", frapy_read_register, METH_VARARGS,
 > +    "read_register (register) -> gdb.Value\n\

Change (register) to (register_name).

 > +Return the value of the register in the frame." },
 >    { "block", frapy_block, METH_NOARGS,
 >      "block () -> gdb.Block.\n\
 >  Return the frame's code block." },
 > diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp
 > index 3517824..9eeebba 100644
 > --- a/gdb/testsuite/gdb.python/py-frame.exp
 > +++ b/gdb/testsuite/gdb.python/py-frame.exp
 > @@ -94,3 +94,16 @@ gdb_test "python print ('result = %s' % f0.read_var ('variable_which_surely_does
 >  gdb_test "python print ('result = %s' % f0.read_var ('a'))" " = 1" "test Frame.read_var - success"
 >  
 >  gdb_test "python print ('result = %s' % (gdb.selected_frame () == f1))" " = True" "test gdb.selected_frame"
 > +
 > +# Can read SP register.
 > +gdb_test "python print ('result = %s' % f0.read_register('sp'))" \
 > +  " = 0x\[0-9a-fA-F\]+" \

Change the test to:

gdb_test "python print ('result = %s' % (f0.read_register('sp') == gdb.parse_and_eval('$sp')))" \
  " = True" \

 > +  "test Frame.read_register(fp)"

Typo. fp -> sp

Also, insert a blank line between each of these tests.

 > +# PC value obtained via read_register is as expected.
 > +gdb_test "python print ('result = %s' % (f0.read_register('pc').cast(gdb.lookup_type('unsigned long')) == f0.pc()))" \

It's odd that one side needs the cast and not the other.
Ah, the result of f0.pc() is a python integer, not a gdb.Value object.
Still, I tried it here and I think the cast is unnecessary.

 > +  "True" \

For consistency with the rest of the file replace this with " = True".

 > +  "test Frame.read_register(pc)"

blank line here

 > +# On x86-64, PC is in $rip register.
 > +gdb_test "python print ('result = %s' % ((f0.architecture().name() != 'i386:x86-64') or f0.read_register('pc') == f0.read_register('rip')))" \
 > +  "True" \

For consistency with the rest of the file replace this with " = True".

 > +  "test Frame.read_register(regnum)"

Change regnum -> rip.

Plus, wrap this in

if {[istarget x86_64-*] && [is_lp64_target]} {
  ...
}

and then remove the f0.architecture test.

Thanks!