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

[Doug Evans <dje@google.com>]

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.