diff mbox

[ovs-dev,v2] valgrind: Parse the summary of valgrind results.

Message ID 1459351413-3796-1-git-send-email-u9012063@gmail.com
State Changes Requested
Headers show

Commit Message

William Tu March 30, 2016, 3:23 p.m. UTC 
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>
---
v1->v2
    - remove check-valgrind-verbose, merge into check-valgrind
    - use $(EGREP) instead of grep
    - if user has .valgrindrc set to '-q', then no SUMMARY will show up,
	  but the error pattern checking at the end will catch the error if any.

Example output:
----------------------------------------------------------------------------------
Total errors: 13. Valgrind output can be found in tests/testsuite.dir/*/valgrind.*
----------------------------------------------------------------------------------
Check definitely memory leak... ok 
Check invalid read/write... FAILED
Check invalid free... ok
Check use of uninitialised values... ok
Check use of uninitialised or unaddressable values in system calls... ok
Check overlapping source and destination blocks... ok
----------------------------------------------------------------------------------

---
 tests/automake.mk | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 5 deletions(-)

Comments

Ben Pfaff March 31, 2016, 5:21 p.m. UTC | #1 
On Wed, Mar 30, 2016 at 08:23:33AM -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>
> ---
> v1->v2
>     - remove check-valgrind-verbose, merge into check-valgrind
>     - use $(EGREP) instead of grep
>     - if user has .valgrindrc set to '-q', then no SUMMARY will show up,
> 	  but the error pattern checking at the end will catch the error if any.

That's a lot of shell script to put in a Makefile.  Can we put it into a
separate script instead?
William Tu March 31, 2016, 5:31 p.m. UTC | #2 
Thanks. I will put it in separate script.

On Thu, Mar 31, 2016 at 10:21 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Mar 30, 2016 at 08:23:33AM -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>
> > ---
> > v1->v2
> >     - remove check-valgrind-verbose, merge into check-valgrind
> >     - use $(EGREP) instead of grep
> >     - if user has .valgrindrc set to '-q', then no SUMMARY will show up,
> >         but the error pattern checking at the end will catch the error
> if any.
>
> That's a lot of shell script to put in a Makefile.  Can we put it into a
> separate script instead?
>
diff mbox

Patch

diff --git a/tests/automake.mk b/tests/automake.mk
index aed032b..5ae7b05 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -201,17 +201,61 @@  $(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 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*
+
+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 '---------------------------------------------------------------------------------'
+	@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 '---------------------------------------------------------------------------------'
+
 
 # OFTest support.