This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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!


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]