596219 - add valgrind support to runtests

Status: RESOLVED FIXED

(Tamarin Graveyard :: Build Config, defect)

Description

[Tommy Reilly]

Be nice to have runtests run valgrind for you and:

a) send valgrind output to logfile
b) make valgrind exit with error when valgrind finds an error
c) copy errors to runtests log when it fails

so the upshot is people can do runtests.py -V or --valgrind and not have to know anything else about valgrind.

Comment 1

[Tommy Reilly]

Attachment: valgrind runtests enhancements

Comment 2

[Edwin Smith]

Comment on attachment 475075 [details] [diff] [review]
valgrind runtests enhancements

I wouldn't bother with the -V short-form option, just --valgrind is enough.  (-V is too overloaded).

Sending all output to /tmp/valgrind.txt will scramble log messages when you run tests in parallel, which is the normal way of running tests. (imagine 8 or 16 cores test runs interleaved).

Are the valgrind options correct?  I notice that it doesn't set the mac specific dsym flag, nor does it set '--valgrind-discard-translations', which is required for correct handling of JIT code pages.

If a user hasn't read this patch, how are they supposed to know the output is in /tmp/valgrind.txt?  OIC,  runtest.py reads and prints the output... okay.  hmm, but it doesnt delete /tmp/valgrind.txt afterwards.  should it clean up after itself?)  It might be better to save the valgrind output in a file distinguished by the test run that failed, or pid, or both, in the current dir instead of in /tmp.

Seems like the right direction, bug f- since the patch doesn't look finished.

Comment 3

[Tommy Reilly]

I usually leave off --dsymutil because its expensive and you only need to do it once.

VALGRIND_DISCARD_TRANSLATIONS is a client request not an option.

Good point on the log being trampled be multiple processes although there's no reason to delete since valgrind truncates it for every run.

Comment 4

[Tommy Reilly]

instead of dealing with the log file issue I'll just use -q which will make valgrind silent unless errors are encountered.

Comment 5

[Edwin Smith]

If errors are encontered in multiple processes, they will still get interleaved.  Also, there's no way to pass options to valgrind (stack trace depth is one i know is useful)

Comment 6

[Dan Schaffer]

Comment on attachment 475075 [details] [diff] [review]
valgrind runtests enhancements

here are a few suggestions:

you should cleanup the /tmp/valgrind.txt between running tests.  You can add code to the runtestPrep function in runtests.py.  Could add like: 
if self.valgrind and os.path.exists('/tmp/valgrind.txt'):
   os.unlink('/tmp/valgrind.txt')

Also when you do if self.valgrind: outputCalls.append( ... should you check if /tmp/valgrind.txt exists before reading it?  Is it an error if the file is not generated?

Comment 7

[Dan Schaffer]

(In reply to comment #2)
> Comment on attachment 475075 [details] [diff] [review]
> Sending all output to /tmp/valgrind.txt will scramble log messages when you run
> tests in parallel, which is the normal way of running tests. (imagine 8 or 16
> cores test runs interleaved).

good point.  We could use the thread name in the log file.  Instead of '/tmp/valgrind.txt'  we use like: 
'/tmp/valgrind-%s.txt' % (threading.currentThread().getName())

would produce logs names like /tmp/valgrind-thread-0.txt

Comment 8

[Tommy Reilly]

(In reply to comment #5)
> If errors are encontered in multiple processes, they will still get
> interleaved.  Also, there's no way to pass options to valgrind (stack trace
> depth is one i know is useful)

Hmm, I presumed that runtests directs the output into a thread local python buffer or something, if runtests can keep avmshell output separate it shouldn't know the difference between a normal shell invocation and a valgrind -q invocation, no?

Comment 9

[Tommy Reilly]

see valgrind-qe.patch in main bug