| Message ID | 1459452667-15573-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Mar 31, 2016 at 12:31:07PM -0700, William Tu wrote: > Before, the 'make check-valgrind' merely outputs results to > tests/testsuite.dir/*/valgrind* and depends on users to verify any errors > in those files. This patch greps results and shows a summary. > > The patch adds '-' before $(SHELL) so that even if test case fails, > the make continues executing and reports total errors. The additional > option --errors-for-leak-kinds=definite will force valgrind's definite > memory leaks as errors and show at the last line of valgrind.* as > "ERROR SUMMARY: <N> errors". In addition, at the end, add checks for > valgrind's error patterns. > > Signed-off-by: William Tu <u9012063@gmail.com> Thanks, William. > @@ -201,17 +202,20 @@ $(valgrind_wrappers): tests/valgrind-wrapper.in > CLEANFILES += $(valgrind_wrappers) > EXTRA_DIST += tests/valgrind-wrapper.in > > -VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full \ > +VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full --errors-for-leak-kinds=definite \ I don't think we should suppress the possible leaks. Sometimes it is possible to rearrange data structures so they don't appear even to be possible leaks, and in those cases I prefer to do that. > --suppressions=$(abs_top_srcdir)/tests/glibc.supp \ > --suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=20 > EXTRA_DIST += tests/glibc.supp tests/openssl.supp > check-valgrind: all tests/atconfig tests/atlocal $(TESTSUITE) \ > $(valgrind_wrappers) $(check_DATA) > - $(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) > + -$(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) I think that we should try to save the exit status of the testsuite and use it as the ultimate exit status for the set of commands. > @echo > - @echo '----------------------------------------------------------------------' > - @echo 'Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' > - @echo '----------------------------------------------------------------------' > + @echo '---------------------------------------------------------------------------------' > + @echo -n Total errors: `find tests/testsuite.dir -name "valgrind.*" | xargs cat | \ > + sed -n 's/.*ERROR\ SUMMARY:\ \([0-9]*\)\ errors.*/.+\1/p' | bc | tail -1`'.' > + @echo ' Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' > + @echo '---------------------------------------------------------------------------------' I would have expected to see the above moved into the new script also. > + @$(SHELL) $(abs_top_srcdir)/tests/valgrind-parse.sh > > # OFTest support. > > diff --git a/tests/valgrind-parse.sh b/tests/valgrind-parse.sh > new file mode 100755 > index 0000000..0aea1a2 > --- /dev/null > +++ b/tests/valgrind-parse.sh > @@ -0,0 +1,64 @@ > +#!/bin/bash I think that this should be /bin/sh instead of bash, because /bin/sh is portable and I don't see any advantage to using bash-specific features here. > +# Licensed under the Apache License, Version 2.0 (the "License"); > +# you may not use this file except in compliance with the License. > +# You may obtain a copy of the License at: > +# > +# http://www.apache.org/licenses/LICENSE-2.0 > +# > +# Unless required by applicable law or agreed to in writing, software > +# distributed under the License is distributed on an "AS IS" BASIS, > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > +# See the License for the specific language governing permissions and > +# limitations under the License. > +# The following makes some unnecessary portability assumptions. Autoconf has already found the correct invocation for egrep, so I would use that by passing it in the environment to the script, e.g. by invoking it as @EGREP='$(EGREP)' $(SHELL) $(abs_top_srcdir)/tests/valgrind-parse.sh If you're concerned about people invoking the script directly rather than through the Makefile, then you could add a command in the script like EGREP=${EGREP-grep -E}. > +if ! which grep &> /dev/null; then > + exit 1 > +fi > +EGREP="grep -E" > +# Valgrind error pattern, see: > +# http://valgrind.org/docs/manual/mc-manual.html#mc-manual.errormsgs > + > +valgrind_def_leak='definitely lost in' > +valgrind_invalid_rw='Invalid (write|read) of size' > +valgrind_invalid_free='(Invalid|Mismatched) free' > +valgrind_uninit_jmp='Conditional jump or move depends on uninitialised value' > +valgrind_uninit_syscall='Syscall param write(buf) points to uninitialised' > +valgrind_overlap='Source and destination overlap in' > +valgrind_output_dir='tests/testsuite.dir/[0-9]*/valgrind*' The following is pretty repetitive. I'd suggest using a shell function. Also, "echo -n" isn't portable; I suggest using printf instead. > +echo -n 'Check definitely memory leak... ' > +if $EGREP -r "$valgrind_def_leak" $valgrind_output_dir > /dev/null; \ > + then echo 'FAILED'; \ > + else echo 'ok'; \ > +fi > +echo -n 'Check invalid read/write... ' > +if $EGREP -r "$valgrind_invalid_rw" $valgrind_output_dir > /dev/null; \ > + then echo 'FAILED'; \ > + else echo 'ok'; \ > +fi > +echo -n 'Check invalid free... ' > +if $EGREP -r "$valgrind_invalid_free" $valgrind_output_dir > /dev/null; \ > + then echo 'FAILED'; \ > + else echo 'ok'; \ > +fi > +echo -n 'Check use of uninitialised values... ' > +if $EGREP -r "$valgrind_uninit_jmp" $valgrind_output_dir > /dev/null; \ > + then echo 'FAILED'; \ > + else echo 'ok'; \ > +fi > +echo -n 'Check use of uninitialised or unaddressable values in system calls... ' > +if $EGREP -r "$valgrind_uninit_syscall" $valgrind_output_dir > /dev/null; \ > + then echo 'FAILED'; \ > + else echo 'ok'; \ > +fi > +echo -n 'Check overlapping source and destination blocks... ' > +if $EGREP -r "$valgrind_overlap" $valgrind_output_dir > /dev/null; \ > + then echo 'FAILED'; \ > + else echo 'ok'; \ > +fi > +echo '---------------------------------------------------------------------------------' > +exit 0 Thanks, Ben.
diff --git a/tests/automake.mk b/tests/automake.mk index aed032b..57e494c 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -10,7 +10,8 @@ EXTRA_DIST += \ tests/atlocal.in \ $(srcdir)/package.m4 \ $(srcdir)/tests/testsuite \ - $(srcdir)/tests/testsuite.patch + $(srcdir)/tests/testsuite.patch \ + $(srcdir)/tests/valgrind-parse.sh COMMON_MACROS_AT = \ tests/ovsdb-macros.at \ @@ -201,17 +202,20 @@ $(valgrind_wrappers): tests/valgrind-wrapper.in CLEANFILES += $(valgrind_wrappers) EXTRA_DIST += tests/valgrind-wrapper.in -VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full \ +VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full --errors-for-leak-kinds=definite \ --suppressions=$(abs_top_srcdir)/tests/glibc.supp \ --suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=20 EXTRA_DIST += tests/glibc.supp tests/openssl.supp check-valgrind: all tests/atconfig tests/atlocal $(TESTSUITE) \ $(valgrind_wrappers) $(check_DATA) - $(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) + -$(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) @echo - @echo '----------------------------------------------------------------------' - @echo 'Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' - @echo '----------------------------------------------------------------------' + @echo '---------------------------------------------------------------------------------' + @echo -n Total errors: `find tests/testsuite.dir -name "valgrind.*" | xargs cat | \ + sed -n 's/.*ERROR\ SUMMARY:\ \([0-9]*\)\ errors.*/.+\1/p' | bc | tail -1`'.' + @echo ' Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' + @echo '---------------------------------------------------------------------------------' + @$(SHELL) $(abs_top_srcdir)/tests/valgrind-parse.sh # OFTest support. diff --git a/tests/valgrind-parse.sh b/tests/valgrind-parse.sh new file mode 100755 index 0000000..0aea1a2 --- /dev/null +++ b/tests/valgrind-parse.sh @@ -0,0 +1,64 @@ +#!/bin/bash +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +if ! which grep &> /dev/null; then + exit 1 +fi +EGREP="grep -E" + +# Valgrind error pattern, see: +# http://valgrind.org/docs/manual/mc-manual.html#mc-manual.errormsgs + +valgrind_def_leak='definitely lost in' +valgrind_invalid_rw='Invalid (write|read) of size' +valgrind_invalid_free='(Invalid|Mismatched) free' +valgrind_uninit_jmp='Conditional jump or move depends on uninitialised value' +valgrind_uninit_syscall='Syscall param write(buf) points to uninitialised' +valgrind_overlap='Source and destination overlap in' +valgrind_output_dir='tests/testsuite.dir/[0-9]*/valgrind*' + +echo -n 'Check definitely memory leak... ' +if $EGREP -r "$valgrind_def_leak" $valgrind_output_dir > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ +fi +echo -n 'Check invalid read/write... ' +if $EGREP -r "$valgrind_invalid_rw" $valgrind_output_dir > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ +fi +echo -n 'Check invalid free... ' +if $EGREP -r "$valgrind_invalid_free" $valgrind_output_dir > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ +fi +echo -n 'Check use of uninitialised values... ' +if $EGREP -r "$valgrind_uninit_jmp" $valgrind_output_dir > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ +fi +echo -n 'Check use of uninitialised or unaddressable values in system calls... ' +if $EGREP -r "$valgrind_uninit_syscall" $valgrind_output_dir > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ +fi +echo -n 'Check overlapping source and destination blocks... ' +if $EGREP -r "$valgrind_overlap" $valgrind_output_dir > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ +fi +echo '---------------------------------------------------------------------------------' +exit 0 +
Before, the 'make check-valgrind' merely outputs results to tests/testsuite.dir/*/valgrind* and depends on users to verify any errors in those files. This patch greps results and shows a summary. The patch adds '-' before $(SHELL) so that even if test case fails, the make continues executing and reports total errors. The additional option --errors-for-leak-kinds=definite will force valgrind's definite memory leaks as errors and show at the last line of valgrind.* as "ERROR SUMMARY: <N> errors". In addition, at the end, add checks for valgrind's error patterns. Signed-off-by: William Tu <u9012063@gmail.com> --- tests/automake.mk | 16 ++++++++----- tests/valgrind-parse.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) create mode 100755 tests/valgrind-parse.sh