Re: [RFC] [PATCH] Provide the ability to write the frame unwinder in Python

[Alexander Smundak <asmundak at google dot com>]

Here's another round.
I am not sure who will be the reviewer this time.

>>> Is it possible to see the code, and example usage, of a real-life use-case
>>> of this? That will help folks not familiar with this project to understand
>>> the problem we are trying to solve.
>
> I agree.
>
>> The full implementation of the combined sniffer/frame filter for OpenJDK
>> is about 2500 lines and will eventually become part of it. I am waiting for
>> this GDB patch to be reviewed before I can present it to be reviewed by
>> the JDK community :-)
I am going to publish it on openjdk.java.net site once the site's admin updates
my credentials.

> You could expand the testcase a little?
I have revised it really 'a little', i.e., there is more than one frame with
broken previous frame link, causing standard unwinders to fail, but I am
not sure it's enough. On the other hand, this is a test, making it too
complex would make it harder to debug.

> Please make errors a complete sentence with capitalization and
> punctuation.
Done.

> +
> +    def list_sniffers(self, title, sniffers, name_re):
> +        """Lists the sniffers whose name matches regexp.
> +
> +        Arguments:
> +            title: The line to print before the list.
> +            sniffers: The list of the sniffers.
> +            name_re: sniffer name filter.
> +        """
> +        if not sniffers:
> +            return
> +        print title
> +        for sniffer in sniffers:
> +            if name_re.match(sniffer.name):
> +                print("  %s%s" % (sniffer.name,
> +                                  "" if sniffer.enabled else "[disabled]"))
> +
> +    def invoke(self, arg, from_tty):
> +        locus_re, name_re = parse_sniffer_command_args(arg)
>
> I think parse_sniffer_command_args should error if the loci is not
> global, progspace or objectfile. Either that or the default to object
> file should be documented here, and in the parse_sniffer_command_args
> function.
Not sure what you mean -- locus can be either a literal "global",
a literal "progspace", or a regex to match object file name.

> +        if locus_re.match("global"):
> +            self.list_sniffers("global sniffers:", gdb.frame_sniffers,
> +                               name_re)
> +        if locus_re.match("progspace"):
> +            cp = gdb.current_progspace()
> +            self.list_sniffers("progspace %s sniffers:" % cp.filename,
> +                               cp.frame_sniffers, name_re)
> +        for objfile in gdb.objfiles():
>
> There seems no way *not* to parse object files for sniffers? Say I set
> the loci to "global", that's what I want. Given there could be 1000s
> of object files, I think this default is a bit odd? I might be
> misunderstanding though.
>From the point of view of the output, issuing `info sniffer global'
will yield confusing result if the current progspace happens to contain
an object file with filename 'global' exactly. IMHO it's not a big deal,
especially taking into account that object file names tend to be absolute
paths (the only one I've seen so far not having an absolute path has
filename "system-supplied DSO at 0x7ffff7ffa000".

As to the concerns about the performance: frankly, I've patterned
the code after pretty_printers.py in the same directory, thinking that
if the performance of the 'info sniffer global' will be on par with that
of 'info pretty-printer global'.

> +def do_enable_sniffer(arg, flag):
> +    """Enable/disable sniffer(s)."""
> +    (locus_re, name_re) = parse_sniffer_command_args(arg)
> +    total = 0
> +    if locus_re.match("global"):
> +        total += do_enable_sniffer1(gdb.frame_sniffers, name_re, flag)
> +    if locus_re.match("progspace"):
> +        total += do_enable_sniffer1(gdb.current_progspace().frame_sniffers,
> +                                    name_re, flag)
> +    for objfile in gdb.objfiles():
>
> Again, in an environment where there may be  many object files this seems
> a rather wasteful search if I set the loci to "global", and then we
> search all object files pointlessly?
Judging by the experiments dje@ asked me to conduct, the performance
impact is fairly small, while the effort to maintain the exact list of
the curently
enabled sniffers is non-trivial. I can conduct additional experiments with
thousands of object files to see if it's worth it.

> +        if locus_re.match(objfile.filename):
> +            total += do_enable_sniffer1(objfile.frame_sniffers, name_re,
> +                                        flag)
> +    print("%d sniffer%s %s" % (total, "" if total == 1 else "s",
> +                               "enabled" if flag else "disabled"))
>
>
> +def execute_sniffers(sniffer_info):
> +    """Internal function called from GDB to execute all sniffers.
> +
> +    Runs each currently enabled sniffer until it finds the one that can
> +    unwind given frame.
> +
> +    Arguments:
> +        sniffer_info: an instance of gdb.SnifferInfo.
> +    Returns:
> +        Unwind info or None. See the description of the value returned
> +        by Sniffer.__call__ below.
> +    """
> +    for objfile in gdb.objfiles():
> +        for sniffer in objfile.frame_sniffers:
> +            if sniffer.enabled:
> +                unwind_info = sniffer.__call__(sniffer_info)
> +                if unwind_info is not None:
> +                    return unwind_info
>
> I think we may need a priority system. We ran into this with frame
> filters. Allow me to explain a scenario, and how a user might solve
> it.
I don't think there is much common between frame filters and frame
sniffers. The output of a filter is passed to the next filter, whereas
a sniffer just returns the previous frame, I am not sure why the
output of one sniffer may be handed down to the next one.

> Say in the future I am debugging openjdk, and the RPM on my Fedora
> system installs a bunch of frame sniffers in the GDB auto-loading
> path. So they get auto-loaded when the openjdk inferior is
> loaded. Good, that's what I want. But what happens if I want to
> register my own frame sniffer that does additional stuff?
If a sniffer already knows how to unwind a frame, there is nothing
more to do, and there is no need to override this (unless you are
debugging the sniffer proper, but this IMHO is a corner case).
This is quite different from the frame filters behavior.

> --- /dev/null
> +++ b/gdb/python/lib/gdb/sniffer.py
> @@ -0,0 +1,113 @@
> +# Copyright (C) 2013-2015
>
> This and others, 2015 only.
Done.

> +
> +class Sniffer(object):
> +    """Base class (or a template) for frame sniffers written in Python.
> +
> +    A sniffer has a single method __call__ and the attributes described below.
> +
> +    Attributes:
> +        name: The name of the sniffer.
> +        enabled: A boolean indicating whether the sniffer is enabled.
>
>      priority: The priority order of the sniffers
> (If you go with the idea of a priority based system)
>
>
> +        Returns:
> +            (REG_DATA, FRAME_ID_REGNUMS) tuple, or None if this sniffer cannot
> +            unwind the frame.
> +            REG_DATA is a tuple containing the registers in the unwound frame.
> +            Each element in thr REG_DATA is a (REG_NUM, REG_VALUE) tuple,
> +            where REG_NUM is platform-specific register number, and REG_VALUE
> +            is register value (a gdb.Value instance).
>
> See below
>
> +            FRAME_ID_REGNUMS is a tuple specifying the registers used
> +            to construct the frame ID. For instance, on x86_64,
> +            FRAME_ID_REGNUMS is (7, 16), as 7 is the stack pointer
> +            register (RSP), and 16 is the instruction pointer (RIP).
> +            FRAME_ID_REGNUMS is a (SP), (SP, PC), or (SP, PC,
> SPECIAL)
>
> This seems like an internal detail exposed a little too much? Why not
> return an object with the attributes SP, PC and SPECIAL? These could
> be mapped from the Python API register methods talked of earlier in
> the previous emails.  These three registers are either returned (SP
> looks to be the mandatory one), or not. No additional registers other
> than the three listed will be included in FRAME_ID_REGNUMS?
>
> Does the actual number of the register matter here to the
> user/implementer?  You tell the user the register number for SP, PC
> and SPECIAL as you populate them in tuple order. Why make the user
> jump through hops to map values to those three register numbers from
> REG_DATA when you could do it for them? You could still return an
> attribute containing the tuple REG_DATA if they wanted to poke around
> the other registers. It seem like, though, we don't tell them what the
> other registers are, so only the three named are important.
>
> There could be a very good reason for this, but I cannot see it
> myself. What do you think?
>
I am struggling to describe platform-specific stuff in a platform-independent
manner. Perhaps a better approach would be to spell what a sniffer is
expected to return for each platform (... denotes optional (regnum,
regvalue) 2-tuples):

x86_64:
(((#RBP, $RBP), (#RSP, $RSP), (#RIP, $RIP), ...),
 (#RSP, #RIP))
where #RBP=6, #RSP=7, #RIP=16

x86:
(((#EBP, $EBP), (#ESP, $ESP), (#EIP, $EIP), ...),
 (#ESP, #EIP))
where #EBP=5, #ESP=4, #EIP=8

PPC64:
(((#R1, $R1), (#PC, $PC), ...),
 (#R1, #PC))
where #R1=1, #PC=64)

I don't think it's too much to ask unwinder's author to return such
tuples (BTW, it
also shows the for x86 the sniffer is expected to return xBP in
addition to xSP and xIP).
Do you think it will be better if the expected values were documented
per platform?

>
> +            tuple, where SP, PC, and SPECIAL are register numbers. Depending
> +            on the tuple, Python sniffer support code uses internal GDB
> +            functions to construct frame ID as follows:
> +            (SP)                  frame_id_build_wild (Value(SP))
> +            (SP, PC)              frame_id_build (Value(SP), Value(PC))
> +            (SP, PC, SPECIAL)     frame_id_build_special (Value(SP),
> +                                   Value(PC), Value(SPECIAL)
> +            The registers in the FRAME_ID_REGNUMS tuple should be among those
> +            returned by REG_DATA.
> +            The chapter "Stack Frames" in the GDB Internals Guide describes
> +            frame ID.
> +        """
> +        raise NotImplementedError("Sniffer __call__")
>
> There might be a super-class provided somewhere the implements the
> comments above. So comments  might apply there too.
Not sure what you mean here, please elaborate.

> +typedef struct
> +{
> +  struct frame_id frame_id;
> +  struct gdbarch *gdbarch;
> +  int reg_count;
> +  struct reg_info
> +  {
> +    int number;
> +    gdb_byte *data;
> +  } reg[];
> +} cached_frame_info;
>
> Each field in a struct needs to be documented.
Commented cached_frame_info fields.

>
> +sniffer_infopy_read_register (PyObject *self, PyObject *args)
> +{
> +  volatile struct gdb_exception except;
> +  int regnum;
> +  struct value *val = NULL;
> +
> +  if (!PyArg_ParseTuple (args, "i", &regnum))
> +    return NULL;
> +
> +  TRY_CATCH (except, RETURN_MASK_ALL)
> +    {
> +      /* Calling `value_of_register' will result in infinite recursion.
> +         Call `deprecated_frame_register_read' instead.  */
>
> Can you explain this comment in more detail?

>
> +      if (deprecated_frame_register_read (frame, regnum, buffer))
> +        val = value_from_contents (register_type (gdbarch, regnum), buffer);
> +      if (val == NULL)
> +        {
> +          char *error_text
> +              = xstrprintf (_("Cannot read register %d from frame"), regnum);
> +          PyErr_SetString (PyExc_ValueError, error_text);
> +          xfree (error_text);
>
> I believe in this case you are overwriting the GDB generated exception
> with a more generic error message. Don't do it.
I have changed the code to validate `regnum' by calling
`user_ret_map_regnum_to_name', but I am not sure which exception
you mean.

> +static PyObject *
> +frame_info_to_sniffer_info_object (struct frame_info *frame)
> +{
> +  sniffer_info_object *sniffer_info
> +      = PyObject_New (sniffer_info_object, &sniffer_info_object_type);
> +
> +  if (sniffer_info != NULL)
>
> This is a new object so you should unconditionally write the
> attribute?
Done.

> +static int
> +pyuw_parse_int (PyObject *pyo_int, int *valuep)
> +{
>
> There's an issue with Python 2.x and Python 3.x compatibility with
> longs. Please see py-value.c:1447 comment and resulting logic to
> ensure this works in both cases.
Done.

> +static int
> +pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
> +              void **cache_ptr)
> +...
> +  gdb_assert (*cache_ptr == NULL);
>
> Can you please document the assert, and the reason for it.
Deleted this assert.

>
> +  gdbarch = (void *)(self->unwind_data);
>
> This cast seems odd to me.
Indeed.

> +          if (data_size != TYPE_LENGTH (value_enclosing_type (value)))
> +            {
> +              error (_("The value of the register #%d returned by the "
> +                       "Python sniffer has unexpected size: %u instead "
> +                       "of %u"), reg->number,
> +                     (unsigned)(TYPE_LENGTH (value_enclosing_type (value))),
> +                     (unsigned)data_size);
> +            }
>
> Can you please document this assert reason.
I assume you mean the one on the line below:
> +          gdb_assert ((gdb_data_free + data_size) <= gdb_data_end);
It's an overflow check -- is it redundant?

The ChangeLog files remain unchanged since last round, and the new
patch is attached.

Attachment: patch4.diff
Description: Text document