Re: [PING 2][PATCH v3] Add pretty printers for the NPTL lock types

[Carlos O'Donell <carlos at redhat dot com>]

On 03/09/2016 03:01 PM, Martin Galvan wrote:
> As requested, I've written a series of test cases for the pretty
> printers. Each lock type (mutex, condvar and rwlock) has two test
> programs, one for itself and other for its related 'attributes'
> object. Each test program in turn has a PExpect-based Python script
> that drives gdb and compares its output to the expected printer's. I'm
> attaching the test cases so you guys can start reviewing them; in the
> meanwhile I'll dwell further into the build system stuff.

This is really awesome.

>From a high level I think the testing is coming along very nicely.

Things I really like:

* PExpect looks very cool. It looks like you can just drive the
  test easily by hand if you detect it's failing which is an
  important part of re-running the test with local changes
  while debugging.

* I like one *.c and one *.py file tied together for testing
  purposes. It makes it easy to see how they work together.
  We should keep this 1:1 relationship as much as we can.
  I really like how easy glibc's tests are to hack on.

One things I'd like to see:

* Test timeouts? What if the tests get stuck?
  - I don't see how you control the timeouts or time them to
    the existing TIMEOUTFACTOR or define default timeouts.


> A few things to consider while reviewing this:
> 
> * I use the 'GCC optimize' pargma to disable compiler optimizations.
> This is necessary because I rely on the C code structure being
> preserved when stepping through the programs. Things like aggressive
> instruction reordering or optimizing variables out will complicate the
> test scripts beyond my reach.

That's fine.

> * Each call to the pthread_* functions should be kept on a separate
> line. This is done so I can easily advance through the programs using
> the gdb 'next' command.

OK.

> * If you want to run the unit tests, I've included a small Makefile
> that will compile each program and run its corresponding script. You
> should have my printers (and its constants file) set to be loaded when
> gdb starts, e.g. by adding something like the following to .gdbinit:

OK.

> python
> import sys
> sys.path.insert(0, '/home/martin')
> end
> 
> source /home/martin/nptl-printers.py
> 
> I've also attached the newest version of the pretty printers and their
> constants. The last version I sent defined a Printer class which works
> exactly as gdb.RegexpCollectionPrettyPrinter except it could read
> through typedefs. I needed this because of a bug in the Python API
> which is fixed in the latest gdb. I also corrected a couple of
> mistakes and added the corresponding constants for the condvar's clock
> ID attribute.

Awesome. I haven't tried these yet, but I looked through the tests and
they all look pretty OK. I'm not going to nit pick the details right now,
but I think you're getting close to ready for a detailed review.

I think you need to answer the test timeout question though since it
becomes very important, particularly if your model in the PyExpect
gets out of sync with the test program.

-- 
Cheers,
Carlos.