Re: [PATCH v2] Add pretty printers for the NPTL lock types

[Martin Galvan <martin dot galvan at tallertechnologies dot com>]

Hi Mike! Thanks for the feedback.

On Mon, Oct 19, 2015 at 8:03 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 09 Oct 2015 19:24, Martin Galvan wrote:
>> --- a/Makerules
>> +++ b/Makerules
>>
>> +ifdef gen-py-const-headers
>> +# This is a hack we use to generate .py files with constants for Python
>> +# pretty-printers. It works the same way as gen-as-const. See gen-py-const
>> +# for details on how the awk | gcc mechanism works.
>> +$(common-objpfx)%.py: $(..)scripts/gen-py-const.awk %.pysym $(common-before-compile)
>> +     $(AWK) -f $< $(filter %.pysym,$^)
>
> this duplicated line left in by accident for debugging ?  should delete it.

Indeed, it was there for debugging :) will remove it in v3.

>> +     echo '# GENERATED FILE, DO NOT EDIT\n' > $@
>> +     echo '# Constant definitions for the NPTL pretty printers.' >> $@
>> +     echo '# See gen-py-const.awk for details.\n' >> $@
>> +
>> +     # Replace "@name@SOME_NAME@value@SOME_VALUE@" strings from the output of
>> +     # gen-py-const.awk with "SOME_NAME = SOME_VALUE" strings.
>> +     # The '-n' option, combined with the '/p' command, makes sed output only the
>> +     # modified lines instead of the whole input file. The output is redirected
>> +     # to a .py file; we'll import it from printers.py to read the constants
>> +     # generated by gen-py-const.awk.
>> +     # The regex has two capturing groups, for SOME_NAME and SOME_VALUE
>> +     # respectively. Notice SOME_VALUE will start with a '$'; Make requires that
>> +     # we write '$' twice.
>
> these comments are at the shell level instead of makelevel which means they get
> printed to stdout.  would be best to change them to make comments (rather than
> just adding a @ prefix) so we don't waste time execing a shell to do nothing.

Raelly? While I did see them in the make output, I thought anything
that comes after a '#' is a regula Make comment.

>> +     sed -n 's/^.*@name@\([^@]\+\)@value@\$$\([0-9Xxa-fA-F-]\+\)@.*/\1 = \2/p' \
>
> imo we should use -E rather than doing \( \+ \) stuff so we get POSIX ERE
> behavior by default

Is -E a GNU sed flag? I'm not seeing it on the man page, though -r
seems to do the same thing.

> also, the use of a-f and such are dangerous.  use [:hexdigit:] instead like:
>         [[:hexdigit:]Xx-]

Will do. Perhaps the sed line used for gen-as-const should be changed
as well, since that's where I got it from. I'll change that one in a
future patch.

>> +# Install the Python pretty-printers.
>> +py-const = $(patsubst %.pysym,%.py,$(gen-py-const-headers))
>> +install-bin-script = $(py-const) nptl-printers.py
>
> why is this install-bin-script ?  we don't want to install these into /usr/bin/.
> gdb pretty printers usually go into /usr/share/gdb/auto-load/<fspath>.  since
> these are for the pthreads lib, you'd want like:
>         /usr/share/gdb/auto-load/lib/libpthread.so.0
> obviously the path & name will vary based on the install of libpthread.  we
> probably also want to add a configure flag for the gdb autoload path as i
> vaguely recall different distros using different base paths.

That'd be great. However, this would be best addressed by someone
who's more familiar with the glibc build process, and Make and
autoconf in general. I don't know how to fix this myself.

>> +++ b/nptl/nptl-printers.py
>
> at a high level, you should run this through pylint

I remember running pylint on this, but admittedly it was a long time ago :)

>> +'info pretty-printer' gdb command. Printers should trigger automatically when
>
> GNU style puts two spaces after the period

Ok.

> please put before all imports:
>         from __future__ import print_function

Ok.

>> +import sys
>> +import re
>> +import ctypes
>> +import gdb
>
> system modules should be sorted, then a blank line, then the gdb imports
>
> although ctypes looks unused so you should drop it:
> W: 37, 0: Unused import ctypes (unused-import)

This seems to be a remnant from a previous version of the printers.
Will drop it.

>> +class _IteratorP3(object):
>> +    """A simple Iterator class."""
>
> this docstring needs to explain what they're for.  glancing at their use, i
> don't see what they're good for.  if you need a real iterator, you could just
> use itertools.chain() and delete these two classes entirely.

Will look into this, although I recall writing these classes because I
saw something similar being used in the libstdc++ printers.

>> +class MutexPrinter(object):
>
> glancing at the diff classes, they seem to have  ommon behavior.  maybe create
> a local class and implement the common behavior there ?  at least the children
> func looks the same.

Will look into it.

>> +################################################################################
>
> i don't see much value in these lines.  just punt them.

Ok.

>> +        self.values.append(('Threads waiting for this condvar',
>> +                           self.nwaiters >> COND_NWAITERS_SHIFT))
>
> trailing indent is wrong:
> C:376, 0: Wrong continued indentation.
>                            self.nwaiters >> COND_NWAITERS_SHIFT))
>                            ^| (bad-continuation)

Did pylint report that? If so, how did you configure it? I'm asking
this so I can fix any other errors that may pop out.

>> +    def to_string(self):
>> +        """gdb API function.
>> +
>> +        This is called from gdb when we try to print a pthread_rwlockattr_t."""
>> ...
>> +    def children(self):
>> +        """gdb API function.
>> +
>> +        This is called from gdb when we try to print a pthread_rwlockattr_t."""
>
> both of these docstrings shouldn't be cuddled:
> C:557, 4: Closing triple quotes should not be cuddled (docstring-cuddled-quotes)
> C:557, 4: Trailing whitespace in docstring: offset:1: {} (docstring-trailing-whitespace)
> C:564, 4: Closing triple quotes should not be cuddled (docstring-cuddled-quotes)
> C:564, 4: Trailing whitespace in docstring: offset:1: {} (docstring-trailing-whitespace)
>
>> +        def __init__(self, name, regex, callable):
>
> is |callable| part of the gdb API ?  if not, you should rename it:
> W:610,40: Redefining built-in 'callable' (redefined-builtin)

Not really. Will rename it.

>> +            """
>> +            Initialize a pretty-printer.
>
> this should be on the first line:
> C:610, 8: First line should be a short summary (docstring-first-line)
>
>> +    def addSubprinter(self, name, regex, callable):
>
> same here wrt |callable|
>
>> +    printer.addSubprinter('pthread_rwlock_t', '^pthread_rwlock_t$', RWLockPrinter)
>
> line is too long:
> C:665, 0: Line too long (82/80) (line-too-long)
> -mike

Ok.

I'll be sending v3 after fixing these findings. It would be great if
anyone else wants to review this in the meanwhile. Thanks a lot!