diff mbox

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

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

Commit Message

William Tu March 17, 2016, 3:14 a.m. UTC
For target 'check-valgrind', add '-' 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 consider
valgrind's definite memory leaks as errors and show at the last line of
valgrind.* as "ERROR SUMMARY: <N> errors". The patch grep this pattern
and sum the total number of errors.

To better understand the erros, add a new target 'check-valgrind-verbose',
which checks different valgrind's error patterns. Note: all the results
are from tests/testsuite.dir/<N>/valgrind.*, make sure this folder has no
stale files.

Signed-off-by: William Tu <u9012063@gmail.com>
---
 tests/automake.mk | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

Comments

Ben Pfaff March 23, 2016, 4:44 p.m. UTC | #1
On Wed, Mar 16, 2016 at 08:14:55PM -0700, William Tu wrote:
> For target 'check-valgrind', add '-' 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 consider
> valgrind's definite memory leaks as errors and show at the last line of
> valgrind.* as "ERROR SUMMARY: <N> errors". The patch grep this pattern
> and sum the total number of errors.
> 
> To better understand the erros, add a new target 'check-valgrind-verbose',
> which checks different valgrind's error patterns. Note: all the results
> are from tests/testsuite.dir/<N>/valgrind.*, make sure this folder has no
> stale files.
> 
> Signed-off-by: William Tu <u9012063@gmail.com>

Thanks for working on this.

I couldn't get this to work for me, though.  I made a change to
ovs-ofctl to cause it to segfault:

    diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
    index 713edc7..1cd7b38 100644
    --- a/utilities/ovs-ofctl.c
    +++ b/utilities/ovs-ofctl.c
    @@ -129,6 +129,7 @@ static bool recv_flow_stats_reply(struct vconn *, ovs_be32 send_xid,
     int
     main(int argc, char *argv[])
     {
    +    asm volatile ("movb $0,0");
         struct ovs_cmdl_context ctx = { .argc = 0, };
         set_program_name(argv[0]);
         service_start(&argc, &argv);

which caused tests to fail, e.g.:

## ------------------------------ ##
## openvswitch 2.5.90 test suite. ##
## ------------------------------ ##
234: OFPST_PORT request - 1.0                        FAILED (ofp-print.at:1577)

which ended up with a reasonable valgrind message:

==2995== Invalid write of size 1
==2995==    at 0x804BEA6: main (ovs-ofctl.c:132)
==2995==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

and yet:

----------------------------------------------------------------------
Total errors:
Valgrind output can be found in tests/testsuite.dir/*/valgrind.*
----------------------------------------------------------------------

Reflecting, I think it's probably because I have "-q" in ~/.valgrindrc.
Not sure what to do about that.

The regular expressions in check-valgrind-verbose take advantage of a
GNU extension to basic regular expressions, that is, \|.  I suggest
instead using extended regular expressions and $(EGREP), for
portability.

The commands for check-valgrind-verbose are a bit long.  I think they
might be more comfortable in a shell script.  I am not sure that there
is value in making them a separate target; I guess I'd be OK with adding
them to plain "check-valgrind".
diff mbox

Patch

diff --git a/tests/automake.mk b/tests/automake.mk
index 592f648..5d16664 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -184,17 +184,62 @@  $(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 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 '----------------------------------------------------------------------'
+
+# 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'
+
+check-valgrind-verbose: check-valgrind
+	@echo -n 'Check definitely memory leak... '
+	@if grep -r $(valgrind_def_leak) tests/testsuite.dir/* > /dev/null; \
+	then echo 'FAILED';	\
+	else echo 'ok'; 	\
+	fi
+	@echo -n 'Check invalid read/write... '
+	@if grep -r $(valgrind_invalid_rw) tests/testsuite.dir/* > /dev/null; \
+	then echo 'FAILED';	\
+	else echo 'ok'; 	\
+	fi
+	@echo -n 'Check invalid free... '
+	@if grep -r $(valgrind_invalid_free) tests/testsuite.dir/* > /dev/null; \
+	then echo 'FAILED';	\
+	else echo 'ok'; 	\
+	fi
+	@echo -n 'Check use of uninitialised values... '
+	@if grep -r $(valgrind_uninit_jmp) tests/testsuite.dir/* > /dev/null; \
+	then echo 'FAILED';	\
+	else echo 'ok'; 	\
+	fi
+	@echo -n 'Check use of uninitialised or unaddressable values in system calls... '
+	@if grep -r $(valgrind_uninit_syscall) tests/testsuite.dir/* > /dev/null; \
+	then echo 'FAILED';	\
+	else echo 'ok'; 	\
+	fi
+	@echo -n 'Check overlapping source and destination blocks... '
+	@if grep -r $(valgrind_overlap) tests/testsuite.dir/* > /dev/null; \
+	then echo 'FAILED';	\
+	else echo 'ok'; 	\
+	fi
+	@echo '----------------------------------------------------------------------'
+
 
 # OFTest support.