bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] suggestion enhancement valgrind_tests.m4


From: Bernd Becker
Subject: Re: [PATCH] suggestion enhancement valgrind_tests.m4
Date: Sat, 20 Nov 2010 17:07:14 +0100
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.1.15) Gecko/20101026 SUSE/3.0.10 Thunderbird/3.0.10 ThunderBrowse/3.3.2

Hi Simon,

please find my answers inline.

>> ++AC_DEFUN([gl_VALGRIND_CONSISTENCY],
>> +[
>> +  if test "$opt_valgrind_tests" = "no" && test
>> "$opt_valgrind_memleaks_are_errors" != "no"; then
>> +  AC_MSG_WARN(
>> +  [
>> +    *******************************************************************
>> +    ***                                                             ***
>> +    *** Memory leak checking with Valgrind is disabled.             ***
>> +    ***                                                             ***
>> +    *** Ignoring: --enable-valgrind-tests                           ***
>> +    *** Ignoring: settings in \$VALGRIND_MEMLEAK_OPTS                ***
>> +    ***                                                             ***
>> +    *******************************************************************
>> +  ])
>> +  fi
>> +])
>>     
> This is noisy, especially since the majority of users won't need to
> build using valgrind.  You could put this into your project's
> configure.ac if you want it.
>
>   
yes you are right. I removed it from the new patch (below).
>> +AC_DEFUN([gl_VALGRIND_SUMMARY],
>> +[
>> +  valgrind_options=""
>> +  if test "$opt_valgrind_tests" = "yes"; then
>> +    if test "$opt_valgrind_memleaks_are_errors" = "yes" ||
>> +       test "$opt_valgrind_memleaks_are_errors" = "no";
>> +    then
>> +      valgrind_options="Memleaks are errors:
>> ${opt_valgrind_memleaks_are_errors}"
>> +    else
>> +      valgrind_options="Valgrind options:
>> ${opt_valgrind_memleaks_are_errors}"
>> +    fi
>> +  fi
>> +])
>>     
> What does this do?
>
>   
This creates the possibility to print the valgrind options into a
configure summary like e.g. in the following
example: I did that by overwriting the opt_valgrind_memleaks_are_errors
in an earlier macro.

AC_MSG_NOTICE([summary of build options:

  Host type:            ${host}
  Install prefix:       ${prefix}
  Compiler:             ${CC}
  Library types:        Shared=${enable_shared}, Static=${enable_static}
  Valgrind:             ${opt_valgrind_tests}
  ${valgrind_options}
])

Note: I renamed valgrind_options to valgrind_summary in the attached patch.
>> +
>>  # gl_VALGRIND_TESTS()
>>  # -------------------
>>  # Check if valgrind is available, and set VALGRIND to it if available.
>> @@ -14,21 +78,27 @@ AC_DEFUN([gl_VALGRIND_TESTS],
>>    AC_ARG_ENABLE(valgrind-tests,
>>      AS_HELP_STRING([--enable-valgrind-tests],
>>                     [run self tests under valgrind]),
>> -    [opt_valgrind_tests=$enableval], [opt_valgrind_tests=yes])
>> +    [opt_valgrind_tests=$enableval], [opt_valgrind_tests=no])
>>     
> Why change the default?
>
>   
The idea was to explicitly enable valgrind tests. Usually searching for
memory leaks takes a lot of time and when doing regression during e.g. a
"test-first" development approach I usually don't want to spend the
extra time for memory leak testing when checking the logic of the
program. I prefer to make leak checks at the end of feature
implementation. Thus spending the extra time only once.

>From my perspective it is not a big deal to keep the "yes" default.
Maybe it's even better to have it enabled by default.
Changed in the attached patch.

>>    # Run self-tests under valgrind?
>>    if test "$opt_valgrind_tests" = "yes" && test "$cross_compiling" =
>> no; then
>>      AC_CHECK_PROGS(VALGRIND, valgrind)
>>    fi
>>
>> -  if test -n "$VALGRIND" && $VALGRIND -q true > /dev/null 2>&1; then
>> +  if test -n "$VALGRIND" && $VALGRIND true > /dev/null 2>&1; then
>>      opt_valgrind_tests=yes
>> -    VALGRIND="$VALGRIND -q"
>> +    VALGRIND="libtool --mode=execute $VALGRIND \$(VALGRIND_MEMLEAK_OPTS)"
>>     
> This needs to be conditioned on whether libtool is used, and shouldn't
> it call the libtool created in the top-level directory of the project
> rather than the system libtool?
>
>   
Yes, good point. I didn't think of projects without libtool and version
differences.
Changed in the below patch

Bernd

diff --git a/m4/valgrind-tests.m4 b/m4/valgrind-tests.m4
index e2434c6..a9df59a 100644
--- a/m4/valgrind-tests.m4
+++ b/m4/valgrind-tests.m4
@@ -6,6 +6,53 @@ dnl with or without modifications, as long as this
notice is preserved.
 
 dnl From Simon Josefsson
 
+AC_ARG_VAR([VALGRIND_MEMLEAK_OPTS],
+           [use this variable to override command line options of
valgrind for memory leak checking. Run 'valgrind --help' or 'man
valgrind' for valid options])
+
+# gl_VALGRIND_TREAT_LEAKS_AS_ERRORS()
+# -----------------------------------
+# Configuration parameter to treat memory leaks as errors if they
+# occur in a module test
+AC_DEFUN([gl_VALGRIND_MEMLEAKS_ARE_ERRORS],
+[
+  AC_ARG_ENABLE(valgrind-memleaks-are-errors,
+    AS_HELP_STRING([--enable-valgrind-memleaks-are-errors],
+                   [treat module tests with memory leaks as errors
[default=yes]]),
+    [opt_valgrind_memleaks_are_errors=$enableval],
+    [opt_valgrind_memleaks_are_errors=yes])
+
+  if test -z "$VALGRIND_MEMLEAK_OPTS"; then
+    if test "$opt_valgrind_memleaks_are_errors" = "yes"; then
+      opt_valgrind_memleaks_are_errors=yes
+      VALGRIND_MEMLEAK_OPTS="-q --error-exitcode=1 --leak-check=full"
+    else
+      opt_valgrind_memleaks_are_errors=no
+      VALGRIND_MEMLEAK_OPTS="-q"
+    fi
+  else
+    opt_valgrind_memleaks_are_errors="$VALGRIND_MEMLEAK_OPTS"
+  fi
+
+  AC_SUBST([VALGRIND_MEMLEAK_OPTS],[$VALGRIND_MEMLEAK_OPTS])
+  AC_MSG_CHECKING([whether valgrind treats memory leaks as errors])
+  AC_MSG_RESULT($opt_valgrind_memleaks_are_errors)
+])
+
+
+AC_DEFUN([gl_VALGRIND_SUMMARY],
+[
+  valgrind_options=""
+  if test "$opt_valgrind_tests" = "yes"; then
+    if test "$opt_valgrind_memleaks_are_errors" = "yes" ||
+       test "$opt_valgrind_memleaks_are_errors" = "no";
+    then
+      valgrind_summary="Memleaks are errors: 
${opt_valgrind_memleaks_are_errors}"
+    else
+      valgrind_summary="Valgrind options:    
${opt_valgrind_memleaks_are_errors}"
+    fi
+  fi
+])
+
 # gl_VALGRIND_TESTS()
 # -------------------
 # Check if valgrind is available, and set VALGRIND to it if available.
@@ -13,7 +60,7 @@ AC_DEFUN([gl_VALGRIND_TESTS],
 [
   AC_ARG_ENABLE(valgrind-tests,
     AS_HELP_STRING([--enable-valgrind-tests],
-                   [run self tests under valgrind]),
+                   [run self tests under valgrind [default=yes]]),
     [opt_valgrind_tests=$enableval], [opt_valgrind_tests=yes])
 
   # Run self-tests under valgrind?
@@ -21,14 +68,27 @@ AC_DEFUN([gl_VALGRIND_TESTS],
     AC_CHECK_PROGS(VALGRIND, valgrind)
   fi
 
-  if test -n "$VALGRIND" && $VALGRIND -q true > /dev/null 2>&1; then
+  if test -n "$VALGRIND" && $VALGRIND true > /dev/null 2>&1; then
     opt_valgrind_tests=yes
-    VALGRIND="$VALGRIND -q"
+    if test -n "$LIBTOOL"; then
+      VALGRIND="$LIBTOOL --mode=execute $VALGRIND
\$(VALGRIND_MEMLEAK_OPTS)"
+    else
+      VALGRIND="$VALGRIND \$(VALGRIND_MEMLEAK_OPTS)"
+    fi
   else
     opt_valgrind_tests=no
     VALGRIND=
   fi
 
+  gl_VALGRIND_MEMLEAKS_ARE_ERRORS
+  gl_VALGRIND_SUMMARY
+
   AC_MSG_CHECKING([whether self tests are run under valgrind])
   AC_MSG_RESULT($opt_valgrind_tests)
 ])
+
+
+
+
+
+




reply via email to

[Prev in Thread] Current Thread [Next in Thread]