Add a --valgrind configure option.

Message ID 1361217866-30351-1-git-send-email-siretart@tauware.de
State New
Headers show

Commit Message

Reinhard Tartler Feb. 18, 2013, 8:04 p.m.
With this configure option, "make fate" executes all programs with
valgrind.  This option optionally takes as parameter the complete path
and command line for invoking the valgrind program. For use in the FATE
environment, use "./configure --valgrind='valgrind --error-exitcode=1'"

Patch based on work by: Reimar Doeffinger <Reimar.Doeffinger@gmx.de>
cherry picked from commits 77b90f0cd0b702b9ffff55d8612ef8b487705fd6 and
d41c824b233292d7e5314a7029fab8cf554a9b2b
---
 configure                |    8 ++++++++
 tests/fate-valgrind.supp |   31 +++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 tests/fate-valgrind.supp

Comments

Reinhard Tartler Feb. 18, 2013, 8:06 p.m. | #1
On Mon, Feb 18, 2013 at 9:04 PM, Reinhard Tartler <siretart@tauware.de> wrote:
> With this configure option, "make fate" executes all programs with
> valgrind.  This option optionally takes as parameter the complete path
> and command line for invoking the valgrind program. For use in the FATE
> environment, use "./configure --valgrind='valgrind --error-exitcode=1'"
>
> Patch based on work by: Reimar Doeffinger <Reimar.Doeffinger@gmx.de>
> cherry picked from commits 77b90f0cd0b702b9ffff55d8612ef8b487705fd6 and
> d41c824b233292d7e5314a7029fab8cf554a9b2b
> ---
>  configure                |    8 ++++++++
>  tests/fate-valgrind.supp |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>  create mode 100644 tests/fate-valgrind.supp

With the included supression file,
http://bugzilla.libav.org/show_bug.cgi?id=451 can be closed.
Diego Biurrun Feb. 18, 2013, 8:58 p.m. | #2
On Mon, Feb 18, 2013 at 09:04:26PM +0100, Reinhard Tartler wrote:
> With this configure option, "make fate" executes all programs with
> valgrind.  This option optionally takes as parameter the complete path
> and command line for invoking the valgrind program. For use in the FATE
> environment, use "./configure --valgrind='valgrind --error-exitcode=1'"

Why do I still have to pass an extra parameter to valgrind manually?

> Patch based on work by: Reimar Doeffinger <Reimar.Doeffinger@gmx.de>

Döffinger

> --- a/configure
> +++ b/configure
> @@ -278,6 +278,10 @@ Developer options (useful when working on Libav itself):
>    --enable-debug=LEVEL     set the debug level [$debuglevel]
>    --disable-optimizations  disable compiler optimizations
>    --enable-extra-warnings  enable more compiler warnings
> +  --valgrind=VALGRIND      run "make fate" tests through valgrind to detect memory

"make fate" --> FATE

Diego
Reinhard Tartler Feb. 18, 2013, 9:14 p.m. | #3
On Mon, Feb 18, 2013 at 9:58 PM, Diego Biurrun <diego@biurrun.de> wrote:
> On Mon, Feb 18, 2013 at 09:04:26PM +0100, Reinhard Tartler wrote:
>> With this configure option, "make fate" executes all programs with
>> valgrind.  This option optionally takes as parameter the complete path
>> and command line for invoking the valgrind program. For use in the FATE
>> environment, use "./configure --valgrind='valgrind --error-exitcode=1'"
>
> Why do I still have to pass an extra parameter to valgrind manually?

You don't, ./configure --valgrind just works. However, if you want to
execute valgrind with the option --error-exitcode=1, as currently done
on some of our fate instances, you can optionally pass them as
arguments.

>
>> Patch based on work by: Reimar Doeffinger <Reimar.Doeffinger@gmx.de>
>
> Döffinger
>
>> --- a/configure
>> +++ b/configure
>> @@ -278,6 +278,10 @@ Developer options (useful when working on Libav itself):
>>    --enable-debug=LEVEL     set the debug level [$debuglevel]
>>    --disable-optimizations  disable compiler optimizations
>>    --enable-extra-warnings  enable more compiler warnings
>> +  --valgrind=VALGRIND      run "make fate" tests through valgrind to detect memory
>
> "make fate" --> FATE

I don't think that this change will make the situation any clearer, so
I'd prefer to keep it as it is. But I wouldn't insist on the current
wording either, so I'm unsure what to do right now.
Luca Barbato Feb. 18, 2013, 9:50 p.m. | #4
On 18/02/13 21:04, Reinhard Tartler wrote:

I'm not sure it is _that_ compelling.

> +                           NB: Cannot be combined with --target-exec.

This line could go in the documentation (malloc-fill is debatable btw)
but we shouldn't force this, it makes the whole feature less useful.

> +   target_exec="${valgrind##--} --malloc-fill=0x2a --track-origins=yes --leak-check=full --gen-suppressions=all --suppressions=$source_path/tests/fate-valgrind.supp"


This is useful and should be part of the documentation, not of the tests.

> diff --git a/tests/fate-valgrind.supp b/tests/fate-valgrind.supp
> new file mode 100644
> index 0000000..db72c54
> --- /dev/null
> +++ b/tests/fate-valgrind.supp

I'd consider the whole thing a bit more.

lu
Reinhard Tartler Feb. 19, 2013, 6:57 a.m. | #5
On Mon, Feb 18, 2013 at 10:50 PM, Luca Barbato <lu_zero@gentoo.org> wrote:
> On 18/02/13 21:04, Reinhard Tartler wrote:
>
> I'm not sure it is _that_ compelling.
>
>> +                           NB: Cannot be combined with --target-exec.
>
> This line could go in the documentation (malloc-fill is debatable btw)
> but we shouldn't force this, it makes the whole feature less useful.

I'm not sure if I grasp your review in full. I understand that you
object to having that line in the configure output, but suggest to
place it somewhere in doc/developer.texi instead. Is this correct?


>
>> +   target_exec="${valgrind##--} --malloc-fill=0x2a --track-origins=yes --leak-check=full --gen-suppressions=all --suppressions=$source_path/tests/fate-valgrind.supp"
>
>
> This is useful and should be part of the documentation, not of the tests.

I don't understand this comment. I do not propose to have it as part
of the test-suite, but as a convenience configure-switch that sets
--target-exec for use with valgrind. Do you object to this idea? If
yes, please elaborate why, so that I can propose a wording to
doc/developer.texi that addresses your objection.

>> diff --git a/tests/fate-valgrind.supp b/tests/fate-valgrind.supp
>> new file mode 100644
>> index 0000000..db72c54
>> --- /dev/null
>> +++ b/tests/fate-valgrind.supp
>
> I'd consider the whole thing a bit more.

I'm also unsure what you mean with this. Should this be committed
separately? Or do you object to having it in libav.git at all? If yes,
why?

Or do you request additional time for thinking about this a bit more?
Luca Barbato Feb. 19, 2013, 9:18 a.m. | #6
On 19/02/13 07:57, Reinhard Tartler wrote:
> On Mon, Feb 18, 2013 at 10:50 PM, Luca Barbato <lu_zero@gentoo.org> wrote:
>> On 18/02/13 21:04, Reinhard Tartler wrote:
>>
>> I'm not sure it is _that_ compelling.
>>
>>> +                           NB: Cannot be combined with --target-exec.
>>
>> This line could go in the documentation (malloc-fill is debatable btw)
>> but we shouldn't force this, it makes the whole feature less useful.
> 
> I'm not sure if I grasp your review in full. I understand that you
> object to having that line in the configure output, but suggest to
> place it somewhere in doc/developer.texi instead. Is this correct?
> 

Instead of hardwiring valgrind this way I'd:

- document an useful command line
- have the suppression file in documentation or contrib

If you feel that we should absolutely provide a configure option, I
would make it a little general (I know people are using pin and other
analyzers/binary-patchers).

lu

Patch

diff --git a/configure b/configure
index 471240c..3f6c790 100755
--- a/configure
+++ b/configure
@@ -278,6 +278,10 @@  Developer options (useful when working on Libav itself):
   --enable-debug=LEVEL     set the debug level [$debuglevel]
   --disable-optimizations  disable compiler optimizations
   --enable-extra-warnings  enable more compiler warnings
+  --valgrind=VALGRIND      run "make fate" tests through valgrind to detect memory
+                           leaks and errors, using the specified valgrind binary
+                           (and optionally extra arguments).
+                           NB: Cannot be combined with --target-exec.
   --samples=PATH           location of test samples for FATE, if not set use
                            \$LIBAV_SAMPLES at make invocation time.
   --enable-xmm-clobber-test check XMM registers for clobbering (Win64-only;
@@ -1396,6 +1400,7 @@  CMDLINE_SET="
     target_os
     target_path
     toolchain
+    valgrind
 "
 
 CMDLINE_APPEND="
@@ -3594,6 +3599,9 @@  fi
 
 enabled debug && add_cflags -g"$debuglevel" && add_asflags -g"$debuglevel"
 
+test -n "$valgrind" &&
+   target_exec="${valgrind##--} --malloc-fill=0x2a --track-origins=yes --leak-check=full --gen-suppressions=all --suppressions=$source_path/tests/fate-valgrind.supp"
+
 # add some useful compiler flags if supported
 check_cflags -Wdeclaration-after-statement
 check_cflags -Wall
diff --git a/tests/fate-valgrind.supp b/tests/fate-valgrind.supp
new file mode 100644
index 0000000..db72c54
--- /dev/null
+++ b/tests/fate-valgrind.supp
@@ -0,0 +1,31 @@ 
+# seems fixed in newer versions
+# http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=577135
+{
+   zlib-inflate
+   Memcheck:Cond
+   fun:inflateReset2
+   fun:inflateInit2_
+}
+# libc overreads on purpose
+# http://sourceware.org/bugzilla/show_bug.cgi?id=12424
+{
+   eval-strtod
+   Memcheck:Addr8
+   fun:__GI___strncasecmp_l
+   fun:____strtod_l_internal
+   fun:av_strtod
+}
+{
+   eval-strtod
+   Memcheck:Value8
+   fun:__GI___strncasecmp_l
+   fun:____strtod_l_internal
+   fun:av_strtod
+}
+{
+   eval-strtod
+   Memcheck:Cond
+   fun:__GI___strncasecmp_l
+   fun:____strtod_l_internal
+   fun:av_strtod
+}