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

[Mike Frysinger <vapier at gentoo dot org>]

On 20 Oct 2015 09:28, Martin Galvan wrote:
> On Mon, Oct 19, 2015 at 8:03 PM, Mike Frysinger wrote:
> > On 09 Oct 2015 19:24, Martin Galvan wrote:
> >> +     # 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.

negative.  you can run strace to see:
$ cat Makefile 
all:
	@echo hi
	@# comment
$ strace -f -eexecve make
execve("/usr/bin/make", ["make"], [/* 74 vars */]) = 0
[pid 29566] execve("/bin/echo", ["echo", "hi"], [/* 79 vars */]) = 0
[pid 29567] execve("/bin/sh", ["/bin/sh", "-c", "# comment"], [/* 79 vars */]) = 0

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

yeah, -E was added a while ago for BSD compat but wasn't documented.  but since
POSIX has adopted -E, they've added it to the help/man/info pages in the latest
git version.  it's equiv to the -r flag.

although maybe using -r would be better since INSTALL:
	* GNU 'sed' 3.02 or newer
and the -E flag isn't in that version.  i don't think it was first available
until sed-4.2 which was released in 2009 (6 years ago).

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

it would be better to not install this at all and leave it up to distro peeps
until we can get a proper rule for it.  i don't think we need to block merging
until the install step is ironed out.

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

we've got scripts/pylint in the tree which uses the right rc file

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

make sure you're using pylint-1.4+ ... the 0.x releases are crufty now
-mike

Attachment: signature.asc
Description: Digital signature