add a valgrind option to "make test"

Attached is a patch which adds a valgrind option to "make test". The
patch can be merged as-is, but there are a few things I'm unsure about:

o Why the original "rm $OUTPUT" if something goes wrong?
o Right now I replace "clang" with "valgrind ... clang". Should we
introduce a "%clang" instead? That has pros and cons.

Also, I haven't touched Makefile.parallel. That should be trivial, though.

Thanks,
Sam

valgrind-make-test.patch (1.8 KB)

I've tweaked this a bit to better handle tests that fail both with
incorrect data and valgrind errors.

Sam

valgrind-make-test-v2.patch (1.91 KB)

Sam Bishop wrote:

I've tweaked this a bit to better handle tests that fail both with
incorrect data and valgrind errors.

Sam

This is great, Sam! The linux folks will love it.

In the last days I have tweaked a lot on "make test", but now it should
be stable enough for you to get this in. My apologies not coming back to you
earlier.

Please find my comments below:

Index: test/TestRunner.sh

--- test/TestRunner.sh (revision 48417)
+++ test/TestRunner.sh (working copy)
@@ -39,18 +39,38 @@
    exit 1
)

+# Run clang under Valgrind, if asked to by the user.
+if [ -n "$VG" ]; then
+ CLANG="valgrind --leak-check=full -q --log-file-exactly=$OUTPUT.vg clang"
+else
+ CLANG="clang"
+fi
+

I guess this is ok.

SCRIPT=$OUTPUT.script
-grep 'RUN:' $FILENAME | sed "s|^.*RUN:\(.*\)$|\1|g;s|%s|$SUBST|g;s|%llvmgcc|llvm-gcc -emit-llvm|g;s|%llvmgxx|llvm-g++ -emit-llvm|g;s|%prcontext|prcontext.tcl|g" > $SCRIPT
+grep 'RUN:' $FILENAME | \
+ sed -e "s|^.*RUN:\(.*\)$|\1|g" \
+ -e "s|%s|$SUBST|g" \
+ -e "s|%llvmgcc|llvm-gcc -emit-llvm|g" \
+ -e "s|%llvmgxx|llvm-g++ -emit-llvm|g" \
+ -e "s|%prcontext|prcontext.tcl|g" \
+ -e "s|clang|$CLANG|g" > $SCRIPT

nice cleanup. meanwhile I have added a new %t substitution, please add that too.

grep -q XFAIL $FILENAME && (printf "XFAILED '$TESTNAME': "; grep XFAIL $FILENAME)

-/bin/sh $SCRIPT > $OUTPUT 2>&1 || (
+/bin/sh $SCRIPT > $OUTPUT 2>&1
+STATUS=$?
+if [ $STATUS -ne 0 -o -n "$VG" -a -s $OUTPUT.vg ]; then
   echo "******************** TEST '$TESTNAME' FAILED! ********************"
- echo "Command: "
+ echo "Command:"
   cat $SCRIPT
- echo "Output:"
- cat $OUTPUT
- rm $OUTPUT
+ if [ $STATUS -ne 0 ]; then
+ echo "Incorrect Output:"
+ cat $OUTPUT
+ fi
+ if [ -n "$VG" -a -s $OUTPUT.vg ]; then
+ echo "Valgrind Errors:"
+ cat $OUTPUT.vg
+ fi
   echo "******************** TEST '$TESTNAME' FAILED! ********************"
-)
+fi

OK. I guess seeing "Incorrect Output:" instead of "Output:" will disturb nobody.

Index: test/Makefile

--- test/Makefile (revision 48417)
+++ test/Makefile (working copy)
@@ -9,6 +9,6 @@
endif

all::
- PATH=$$PATH:$(ToolDir):$(LLVM_SRC_ROOT)/test/Scripts \
+ PATH=$$PATH:$(ToolDir):$(LLVM_SRC_ROOT)/test/Scripts VG=${VG} \
           find $(TESTDIRS) \( -name '*.c' -or -name '*.cpp' -or -name '*.m' \) \
         -print -exec ./TestRunner.sh {} \;

Why do you use wiggly brackets in ${VG}. The file uses parentheses, let's stick to them.

Btw. test/Makefile is obsolete and will go away soon. Please apply the
change to test/Makefile.parallel too.

Can you resubmit your updated patch? I would gladly check this in.

Cheers,

  Gabor

Hi, Gabor.

Sam Bishop wrote:

...

In the last days I have tweaked a lot on "make test", but now it should
be stable enough for you to get this in. My apologies not coming back to
you earlier.

No problem. And thank you for the review!

SCRIPT=$OUTPUT.script
-grep 'RUN:' $FILENAME | sed
"s|^.*RUN:\(.*\)$|\1|g;s|%s|$SUBST|g;s|%llvmgcc|llvm-gcc
-emit-llvm|g;s|%llvmgxx|llvm-g++
-emit-llvm|g;s|%prcontext|prcontext.tcl|g" > $SCRIPT
+grep 'RUN:' $FILENAME | \
+ sed -e "s|^.*RUN:\(.*\)$|\1|g" \
+ -e "s|%s|$SUBST|g" \
+ -e "s|%llvmgcc|llvm-gcc -emit-llvm|g" \
+ -e "s|%llvmgxx|llvm-g++ -emit-llvm|g" \
+ -e "s|%prcontext|prcontext.tcl|g" \
+ -e "s|clang|$CLANG|g" > $SCRIPT

nice cleanup. meanwhile I have added a new %t substitution, please add
that too.

I made a %t1 -> %t substitution in a TestRunner.sh comment. I'm not
100% sure about it, though. Feel free to back that out if it's wrong.

- PATH=$$PATH:$(ToolDir):$(LLVM_SRC_ROOT)/test/Scripts \
+ PATH=$$PATH:$(ToolDir):$(LLVM_SRC_ROOT)/test/Scripts VG=${VG} \

Why do you use wiggly brackets in ${VG}. The file uses parentheses,
let's stick to them.

Oops. Personal habit. Fixed.

Btw. test/Makefile is obsolete and will go away soon. Please apply the
change to test/Makefile.parallel too.

Yeah, I thought I'd wait 'til things settled down. Done.

Thanks again!

Sam

valgrind-make-test-v3.patch (2.85 KB)

Thanks Sam!

There is some problem though:

cat: Output/Parser/implicit-casts.c.out.vg.*: No such file or directory
// repeated many times //

this is on darwin.

The patch applied, but I did not check it in because of this.

Can you imagine what is going wrong?

Cheers,

  Gabor

Yes, sorry. You don't have old valgrind logs from a previous run...

Ok, this should be it. Thanks again for your help!

Sam

valgrind-make-test-v4.patch (2.87 KB)

Can you imagine what is going wrong?

Yes, sorry. You don't have old valgrind logs from a previous run...

Ok, this should be it. Thanks again for your help!

Yep, committed!

<http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080317/004806.html>

Cheers, and happy valgrinding!

  Gabor