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

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

On Mon, Apr 11, 2016 at 7:16 PM, Roland McGrath <roland@hack.frob.com> wrote:
> You can certainly have some common infrastructure code for both the
> pretty-printer implementations and for their tests.  It might well be fine
> to have a subdir for that common infrastructure code.  But anything
> actually related to a particular type must reside in the subdir responsible
> for the definition of that type.

Does that include the unit tests? That would mean that the
pretty-printers Makefile has to go, since that's what I use for
testing. Perhaps I could try to split its contents between Makerules
and the nptl Makefile? Something like:

# Makerules
ifdef tests-pretty-printers

test-srcs := $(tests-pretty-printers)

tests-pretty-printers-dest := $(addprefix $(objpfx),$(tests-pretty-printers))
tests-pretty-printers-pp := $(addsuffix -pp,$(tests-pretty-printers-dest))

$(tests-pretty-printers-dest): $(tests-pretty-printers-libs)

ifeq ($(run-built-tests),yes)
tests-special += $(tests-pretty-printers-pp)
endif

.PHONY: $(tests-pretty-printers-pp)

$(tests-pretty-printers-pp): $(objpfx)%-pp: $(objpfx)% %.py test_common.py
    $(test-wrapper-env) $(PYTHON) $*.py $*.c $(objpfx)$*; \
    $(evaluate-test)

endif  # tests-pretty-printers

# nptl/Makefile
tests-pretty-printers := test-mutex-attributes test-mutex-printer \
             test-condvar-attributes test-condvar-printer \
             test-rwlock-attributes test-rwlock-printer

ifeq ($(build-shared),yes)
tests-pretty-printers-libs := $(shared-thread-library)
else
tests-pretty-printers-libs := $(static-thread-library)
endif

CFLAGS-test-mutex-attributes.c := -O0 -ggdb3 -DIS_IN_build
CFLAGS-test-mutex-printer.c := -O0 -ggdb3 -DIS_IN_build
CFLAGS-test-condvar-attributes.c := -O0 -ggdb3 -DIS_IN_build
CFLAGS-test-condvar-printer.c := -O0 -ggdb3 -DIS_IN_build
CFLAGS-test-rwlock-attributes.c := -O0 -ggdb3 -DIS_IN_build
CFLAGS-test-rwlock-printer.c := -O0 -ggdb3 -DIS_IN_build

But again, I don't know how (or even if) Makerules interacts with
'make check'. I don't know how this will affect cross-testing either.

> This suggests to me that the testing methodology is a poor choice.  I'd
> have to review what you've done in more detail to know what I think is the
> best approach.  I suspect that using "next" (or "step", etc.) in tests like
> this is just a bad idea altogether (as opposed to only using explicit
> breakpoints).  If it turns out that using "next" over an "if" is an
> important thing to be able to do, then put the complex condition into an
> inline or macro.

I don't know if using explicit breakpoints will make any difference,
but I'll give it a shot.

Still, I'd appreciate it if you took the time to properly review the
patch before suggesting such drastic changes. This took me quite a
while to make, so please don't dismiss it just because it has lines
longer than 80 chars. It would also be nice if we listened to what the
previous reviewers have to say about this (Carlos?).

On Tue, Apr 12, 2016 at 12:03 AM, Siddhesh Poyarekar
<sid@reserved-bit.com> wrote:
> So you're only suggesting moving nptl-printers.py to nptl, which seems
> fine given that it is specific to nptl.

If that's what Roland's asking, then fine for me. It's the unit tests
that concern me, since the Makefile does a great deal to run them
correctly. I'd rather *not* touch the Makefile, or mess with the build
system anymore.

> Actually the comments on those lines are quite inane and could be
> dropped altogether, which should take care of most of those long
> lines.

They're not. I use them to know where to set breakpoints from the test
scripts. It's similar to how testing is done for gdb itself.