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

[Mike Frysinger <vapier at gentoo dot org>]

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.

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

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

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

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

> +++ b/nptl/nptl-printers.py

at a high level, you should run this through pylint

> +'info pretty-printer' gdb command. Printers should trigger automatically when

GNU style puts two spaces after the period

> """

please put before all imports:
	from __future__ import print_function

> +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)


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

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

> +################################################################################

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

> +        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)

> +    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)

> +            """
> +            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

Attachment: signature.asc
Description: Digital signature