[PATCH] Add Frame.read_register to Python API

[PATCH] Add Frame.read_register to Python API

[Alexander Smundak]

The ability to read registers is needed to use Frame Filter API to
display the frames created by JIT compilers.

gdb/Changelog
2014-06-09  Sasha Smundak  <asmundak@google.com>

* python/py-frame.c (frapy_read_register): New function.

2014-06-09  Sasha Smundak  <asmundak@google.com>

* python.texi (Writing a Frame Filter): Fix example code.
(Frames in Python): Add read_register description.
(Frames in Python): Fix reference in find_sal description.

2014-06-09  Sasha Smundak  <asmundak@google.com>

* gdb.python/py-frame.exp: Test Frame.read_register.

diff --git a/gdb/NEWS b/gdb/NEWS
index 1397e8b..b3b7a22 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,9 @@

 *** Changes since GDB 7.7

+* Python Scripting
+  Access frame registers
+
 * New command line options

 -D data-directory
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4688783..a7ff162 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2046,9 +2046,10 @@ class InlinedFrameDecorator(FrameDecorator):

     def __init__(self, fobj):
         super(InlinedFrameDecorator, self).__init__(fobj)
+        self.fobj = fobj

     def function(self):
-        frame = fobj.inferior_frame()
+        frame = self.fobj.inferior_frame()
         name = str(frame.name())

         if frame.type() == gdb.INLINE_FRAME:
@@ -3585,10 +3586,16 @@ Return the frame called by this frame.
 @end defun

 @defun Frame.find_sal ()
-Return the frame's symtab and line object.
+Return the frame's @code{gdb.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., 'rsp' or 'r1'), or a register number.
+Returns @code{Gdb.Value} object.
+@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..4375541 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,38 @@ 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;
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      FRAPY_REQUIRE_VALID (self, frame);
+      if (!PyArg_ParseTuple (args, "i", &regnum))
+ {
+  const char *regnum_str;
+  PyErr_Clear();  /* Clear PyArg_ParseTuple failure above.  */
+  if (PyArg_ParseTuple (args, "s", &regnum_str))
+    {
+      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);
+ }
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+  return val ? value_to_value_object (val) : NULL;
+}
+
 /* Implementation of gdb.Frame.block (self) -> gdb.Block.
    Returns the frame's code block.  */

@@ -674,6 +707,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\
+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..9c56d34 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -94,3 +94,10 @@ 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\]+" "test Frame.read_register(fp)"
+# 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()))" "True" "test Frame.read_register(pc)"
+# On x86-64, PC is register 16.
+gdb_test "python print ('result = %s' % ((f0.architecture().name() !=
'i386:x86-64') or f0.read_register('pc') == f0.read_register(16)))"
"True" "test Frame.read_register(regnum)"

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

[Eli Zaretskii]

> Date: Mon, 9 Jun 2014 12:15:26 -0700
> From: Alexander Smundak <asmundak@google.com>
> 
> 2014-06-09  Sasha Smundak  <asmundak@google.com>
> 
> * python.texi (Writing a Frame Filter): Fix example code.
> (Frames in Python): Add read_register description.
> (Frames in Python): Fix reference in find_sal description.

When 2 changes are in the same node, write them one after the other,
and have only one "(Node Name)" thing.

> +* Python Scripting
> +  Access frame registers

This is an incomplete sentence.  I guess you meant something like

  You can now access frame registers from Python scripts.

> +Return the value of @var{register} in this frame. The @var{register}
> +argument must be a string (e.g., 'rsp' or 'r1'), or a register number.
                                    ^^^^^^^^^^^^^
These should be in @code instead of quotes.

> +Returns @code{Gdb.Value} object.
          ^
"a" missing here.

The documentation parts are OK with these fixed.

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

[Alexander Smundak]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On Mon, Jun 9, 2014 at 12:25 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> When 2 changes are in the same node, write them one after the other,
> and have only one "(Node Name)" thing.
Done.

>> +* Python Scripting
>> +  Access frame registers
>
> This is an incomplete sentence.  I guess you meant something like
>   You can now access frame registers from Python scripts.
Done.

>> +Return the value of @var{register} in this frame. The @var{register}
>> +argument must be a string (e.g., 'rsp' or 'r1'), or a register number.
>                                     ^^^^^^^^^^^^^
> These should be in @code instead of quotes.
Added the code but retained the quotes, because it's string literal.

>> +Returns @code{Gdb.Value} object.
>           ^
> "a" missing here.
Done.
Here it is again:

The ability to read registers is needed to use Frame Filter API to
display the frames created by JIT compilers.

gdb/Changelog
2014-06-09  Sasha Smundak  <asmundak@google.com>

* python/py-frame.c (frapy_read_register): New function.

2014-06-09  Sasha Smundak  <asmundak@google.com>

* python.texi (Writing a Frame Filter): Fix example code.
(Frames in Python): Fix reference in find_sal description.
Add read_register description.

2014-06-09  Sasha Smundak  <asmundak@google.com>

* gdb.python/py-frame.exp: Test Frame.read_register.

[-- Attachment #2: frame-read-register-patch-2.txt --]
[-- Type: text/plain, Size: 4495 bytes --]

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
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4688783..14564eb 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2046,9 +2046,10 @@ class InlinedFrameDecorator(FrameDecorator):
 
     def __init__(self, fobj):
         super(InlinedFrameDecorator, self).__init__(fobj)
+        self.fobj = fobj
 
     def function(self):
-        frame = fobj.inferior_frame()
+        frame = self.fobj.inferior_frame()
         name = str(frame.name())
 
         if frame.type() == gdb.INLINE_FRAME:
@@ -3585,10 +3586,16 @@ Return the frame called by this frame.
 @end defun
 
 @defun Frame.find_sal ()
-Return the frame's symtab and line object.
+Return the frame's @code{gdb.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{'rsp'} or @code{'r1'}), or a
+register number. Returns a @code{Gdb.Value} object.
+@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..4375541 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,38 @@ 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;
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      FRAPY_REQUIRE_VALID (self, frame);
+      if (!PyArg_ParseTuple (args, "i", &regnum))
+	{
+	  const char *regnum_str;
+	  PyErr_Clear();  /* Clear PyArg_ParseTuple failure above.  */
+	  if (PyArg_ParseTuple (args, "s", &regnum_str))
+	    {
+	      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);
+	}
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+  return val ? value_to_value_object (val) : NULL;
+}
+
 /* Implementation of gdb.Frame.block (self) -> gdb.Block.
    Returns the frame's code block.  */
 
@@ -674,6 +707,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\
+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..6034fff 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\]+" \
+  "test Frame.read_register(fp)"
+# 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()))" \
+  "True" \
+  "test Frame.read_register(pc)"
+# On x86-64, PC is register 16.
+gdb_test "python print ('result = %s' % ((f0.architecture().name() != 'i386:x86-64') or f0.read_register('pc') == f0.read_register(16)))" \
+  "True" \
+  "test Frame.read_register(regnum)"

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

[Tom Tromey]

>>>>> "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", &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.

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

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

[Alexander Smundak]

[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]

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

[-- Attachment #2: frame-read-register-patch-3.txt --]
[-- Type: text/plain, Size: 4078 bytes --]

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
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)
+    {
+      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\
+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\]+" \
+  "test Frame.read_register(fp)"
+# 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()))" \
+  "True" \
+  "test Frame.read_register(pc)"
+# 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" \
+  "test Frame.read_register(regnum)"

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

[Alexander Smundak]

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.

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

[Eli Zaretskii]

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

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

[Alexander Smundak]

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.

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

[Alexander Smundak]

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.

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

[Alexander Smundak]

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.

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

[Alexander Smundak]

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.

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

[Doug Evans]

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?

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

[Doug Evans]

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.

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

[Tom Tromey]

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

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

[Doug Evans]

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!

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

[Alexander Smundak]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

I've fixed the problems you pointed out, please take another look.
Sasha


The ability to read registers is needed to use Frame Filter API to
display the frames created by JIT compilers.

gdb/Changelog
2014-08-26  Sasha Smundak  <asmundak@google.com>

* python/py-frame.c (frapy_read_register): New function.

2014-08-26  Sasha Smundak  <asmundak@google.com>

* python.texi (Frames in Python): Add read_register description.

2014-08-26  Sasha Smundak  <asmundak@google.com>

* gdb.python/py-frame.exp: Test Frame.read_register.

[-- Attachment #2: frame-read-register-patch-4.txt --]
[-- Type: text/plain, Size: 4245 bytes --]

diff --git a/gdb/NEWS b/gdb/NEWS
index d603cf7..46c6a87 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,9 @@
 
 *** Changes since GDB 7.8
 
+* Python Scripting
+  You can now access frame registers from Python scripts.
+
 * On resume, GDB now always passes the signal the program had stopped
   for to the thread the signal was sent to, even if the user changed
   threads before resuming.  Previously GDB would often (but not
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 120e147..88e9114 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,40 @@ 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)
+{
+  volatile struct gdb_exception except;
+  const char *regnum_str;
+  struct value *val = NULL;
+
+  if (!PyArg_ParseTuple (args, "s", &regnum_str))
+    return NULL;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      struct frame_info *frame;
+      int regnum = -1;
+
+      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 +709,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_name) -> gdb.Value\n\
+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..e47f340 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -94,3 +94,20 @@ 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' % (gdb.selected_frame ().read_register ('sp') == gdb.parse_and_eval ('\$sp')))" \
+  " = True" \
+  "test Frame.read_register(sp)"
+
+# PC value obtained via read_register is as expected.
+gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.pc()))" \
+  " = True" \
+  "test Frame.read_register(pc)"
+
+# On x86-64, PC is in $rip register.
+if {[istarget x86_64-*]} {
+    gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.read_register('rip')))" \
+	" = True" \
+	"test Frame.read_register(rip)"
+}

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

[Doug Evans]

Alexander Smundak writes:
 > I've fixed the problems you pointed out, please take another look.
 > Sasha
 > 
 > 
 > The ability to read registers is needed to use Frame Filter API to
 > display the frames created by JIT compilers.
 > 
 > gdb/Changelog
 > 2014-08-26  Sasha Smundak  <asmundak@google.com>
 > 
 > * python/py-frame.c (frapy_read_register): New function.
 > 
 > 2014-08-26  Sasha Smundak  <asmundak@google.com>
 > 
 > * python.texi (Frames in Python): Add read_register description.
 > 
 > 2014-08-26  Sasha Smundak  <asmundak@google.com>
 >
 > * gdb.python/py-frame.exp: Test Frame.read_register.

Hi.

Nit: See
https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages
for new guidelines on how commit messages should be formatted.
The Changelog part of it is a little different than what's
written above.
[I don't want to be excessively picky about such things
during review. Just want to make sure that when this gets checked
in the format is right: the above is missing directory names for
each section.]

 > diff --git a/gdb/NEWS b/gdb/NEWS
 > index d603cf7..46c6a87 100644
 > --- a/gdb/NEWS
 > +++ b/gdb/NEWS
 > @@ -3,6 +3,9 @@
 >  
 >  *** Changes since GDB 7.8
 >  
 > +* Python Scripting
 > +  You can now access frame registers from Python scripts.
 > +
 >  * On resume, GDB now always passes the signal the program had stopped
 >    for to the thread the signal was sent to, even if the user changed
 >    threads before resuming.  Previously GDB would often (but not
 > 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 120e147..88e9114 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,40 @@ 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)
 > +{
 > +  volatile struct gdb_exception except;
 > +  const char *regnum_str;
 > +  struct value *val = NULL;
 > +
 > +  if (!PyArg_ParseTuple (args, "s", &regnum_str))
 > +    return NULL;
 > +
 > +  TRY_CATCH (except, RETURN_MASK_ALL)
 > +    {
 > +      struct frame_info *frame;
 > +      int regnum = -1;

Nit: I suspect the "= -1" is no longer needed.

 > +
 > +      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 +709,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_name) -> gdb.Value\n\
 > +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..e47f340 100644
 > --- a/gdb/testsuite/gdb.python/py-frame.exp
 > +++ b/gdb/testsuite/gdb.python/py-frame.exp
 > @@ -94,3 +94,20 @@ 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' % (gdb.selected_frame ().read_register ('sp') == gdb.parse_and_eval ('\$sp')))" \
 > +  " = True" \
 > +  "test Frame.read_register(sp)"
 > +
 > +# PC value obtained via read_register is as expected.
 > +gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.pc()))" \
 > +  " = True" \
 > +  "test Frame.read_register(pc)"
 > +
 > +# On x86-64, PC is in $rip register.
 > +if {[istarget x86_64-*]} {
 > +    gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.read_register('rip')))" \
 > +	" = True" \
 > +	"test Frame.read_register(rip)"
 > +}

LGTM with the above nits addressed.

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

[Sasha Smundak]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

Thanks.
Here it is again:

The ability to read registers is needed to use Frame Filter API to
display the frames created by JIT compilers.

gdb/ChangeLog:

2014-08-29  Sasha Smundak  <asmundak@google.com>

	* python/py-frame.c (frapy_read_register): New function.

gdb/doc/ChangeLog

2014-08-26  Sasha Smundak  <asmundak@google.com>

	* python.texi (Frames in Python): Add read_register description.

gdb/testsuite/ChangeLog:

2014-08-26  Sasha Smundak  <asmundak@google.com>

	* gdb.python/py-frame.exp: Test Frame.read_register.

[-- Attachment #2: frame-read-register-patch-5.txt --]
[-- Type: text/plain, Size: 4240 bytes --]

diff --git a/gdb/NEWS b/gdb/NEWS
index d603cf7..46c6a87 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,9 @@
 
 *** Changes since GDB 7.8
 
+* Python Scripting
+  You can now access frame registers from Python scripts.
+
 * On resume, GDB now always passes the signal the program had stopped
   for to the thread the signal was sent to, even if the user changed
   threads before resuming.  Previously GDB would often (but not
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 120e147..859d115 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,40 @@ 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)
+{
+  volatile struct gdb_exception except;
+  const char *regnum_str;
+  struct value *val = NULL;
+
+  if (!PyArg_ParseTuple (args, "s", &regnum_str))
+    return NULL;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      struct frame_info *frame;
+      int regnum;
+
+      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 +709,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_name) -> gdb.Value\n\
+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..e47f340 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -94,3 +94,20 @@ 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' % (gdb.selected_frame ().read_register ('sp') == gdb.parse_and_eval ('\$sp')))" \
+  " = True" \
+  "test Frame.read_register(sp)"
+
+# PC value obtained via read_register is as expected.
+gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.pc()))" \
+  " = True" \
+  "test Frame.read_register(pc)"
+
+# On x86-64, PC is in $rip register.
+if {[istarget x86_64-*]} {
+    gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.read_register('rip')))" \
+	" = True" \
+	"test Frame.read_register(rip)"
+}

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

[Doug Evans]

On Fri, Aug 29, 2014 at 4:32 PM, Sasha Smundak <asmundak@google.com> wrote:
> Thanks.
> Here it is again:
>
>
> The ability to read registers is needed to use Frame Filter API to
> display the frames created by JIT compilers.
>
> gdb/ChangeLog:
>
> 2014-08-29  Sasha Smundak  <asmundak@google.com>
>
>
>         * python/py-frame.c (frapy_read_register): New function.
>
> gdb/doc/ChangeLog
>
>
> 2014-08-26  Sasha Smundak  <asmundak@google.com>
>
>         * python.texi (Frames in Python): Add read_register description.
>
> gdb/testsuite/ChangeLog:
>
>
> 2014-08-26  Sasha Smundak  <asmundak@google.com>
>
>         * gdb.python/py-frame.exp: Test Frame.read_register.

LGTM.

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

[Doug Evans]

On Fri, Aug 29, 2014 at 4:36 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Aug 29, 2014 at 4:32 PM, Sasha Smundak <asmundak@google.com> wrote:
>> Thanks.
>> Here it is again:
>>
>>
>> The ability to read registers is needed to use Frame Filter API to
>> display the frames created by JIT compilers.
>>
>> gdb/ChangeLog:
>>
>> 2014-08-29  Sasha Smundak  <asmundak@google.com>
>>
>>
>>         * python/py-frame.c (frapy_read_register): New function.
>>
>> gdb/doc/ChangeLog
>>
>>
>> 2014-08-26  Sasha Smundak  <asmundak@google.com>
>>
>>         * python.texi (Frames in Python): Add read_register description.
>>
>> gdb/testsuite/ChangeLog:
>>
>>
>> 2014-08-26  Sasha Smundak  <asmundak@google.com>
>>
>>         * gdb.python/py-frame.exp: Test Frame.read_register.
>
> LGTM.

Hi.

fyi, I've committed this.

Thanks!