Add valgrind check to travis

elichai commented at 1:48 pm on November 8, 2019: contributor

As discussed in #687 This adds valgrind check to the repo.

It doesn’t run on recovery+ecdh because of the time. No openssl because of uninitialized mem. I debated between with and without ASM, but decided with ASM because it might be more fragile(?).

I wasn’t sure if I should pass -DVALGRIND via CFLAGS or CPPFLAGS, it seems like because this is only C then there shouldn’t even be CPPFLAGS but looks like we use CPPFLAGS in other places for the preprocessor definitions.

If people are worried about the time it takes we can mark it as allow_failure although I don’t think it’s a problem here because there’s only a handful of PRs and they’re usually open for weeks.

Add valgrind check to travis b4c1382a87

real-or-random commented at 2:15 pm on November 8, 2019: contributor

Concept ACK

I wasn’t sure if I should pass -DVALGRIND via CFLAGS or CPPFLAGS, it seems like because this is only C then there shouldn’t even be CPPFLAGS but looks like we use CPPFLAGS in other places for the preprocessor definitions.

CPPFLAGS are the flags for the C preprocessor. If you need to define flags for a C++ compiler, then you want CXXFLAGS. (Yes, it’s confusing…)

elichai commented at 2:17 pm on November 8, 2019: contributor

LOOOL. everything makes sense now. Thanks :) (i’ve been passing C++ flags to both CPPFLAGS and CXXFLAGS for years :) )

in .travis.yml:66 in b4c1382a87 outdated

60@@ -61,10 +61,21 @@ matrix:
61           packages:
62             - gcc-multilib
63             - libgmp-dev:i386
64+    - compiler: gcc
65+      env:
66+        - FIELD=auto  BIGNUM=no  SCALAR=auto  ENDOMORPHISM=yes  STATICPRECOMPUTATION=yes  ASM=x86_64

jonasnick commented at 6:08 pm on November 8, 2019:

Imo easier to read if you don’t redefine variables like field and scalar.

in .travis.yml:67 in b4c1382a87 outdated

60@@ -61,10 +61,21 @@ matrix:
61           packages:
62             - gcc-multilib
63             - libgmp-dev:i386
64+    - compiler: gcc
65+      env:
66+        - FIELD=auto  BIGNUM=no  SCALAR=auto  ENDOMORPHISM=yes  STATICPRECOMPUTATION=yes  ASM=x86_64
67+        - EXPERIMENTAL=no ECDH=no  RECOVERY=no VALGRIND=yes EXTRAFLAGS="--disable-openssl-tests CPPFLAGS=-DVALGRIND"  HOST= BUILD= JNI=no

jonasnick commented at 6:08 pm on November 8, 2019:

I think we want ECDH and RECOVERY enabled here?

elichai commented at 7:34 pm on November 8, 2019:

On my first tries it took ~25 minutes, but now with ./test 8 it might be faster, i’ll try re-enabling them.

jonasnick commented at 10:40 pm on November 8, 2019:

It worked in my testing without endo and with schnorrsig. Without enabling all features we would still want the memsanitizer PR.

jonasnick commented at 11:19 pm on November 8, 2019:

see https://travis-ci.org/jonasnick/secp256k1/jobs/609452969

in .travis.yml:81 in b4c1382a87 outdated

77  - if [ -n "$HOST" ]; then export USE_HOST="--host=$HOST"; fi
78  - if [ "x$HOST" = "xi686-linux-gnu" ]; then export CC="$CC -m32"; fi
79- - ./configure --enable-experimental=$EXPERIMENTAL --enable-endomorphism=$ENDOMORPHISM --with-field=$FIELD --with-bignum=$BIGNUM --with-asm=$ASM --with-scalar=$SCALAR --enable-ecmult-static-precomputation=$STATICPRECOMPUTATION --with-ecmult-gen-precision=$ECMULTGENPRECISION --enable-module-ecdh=$ECDH --enable-module-recovery=$RECOVERY --enable-jni=$JNI $EXTRAFLAGS $USE_HOST && make -j2 $BUILD
80+ - ./configure --enable-experimental=$EXPERIMENTAL --enable-endomorphism=$ENDOMORPHISM --with-field=$FIELD --with-bignum=$BIGNUM --with-asm=$ASM --with-scalar=$SCALAR --enable-ecmult-static-precomputation=$STATICPRECOMPUTATION --with-ecmult-gen-precision=$ECMULTGENPRECISION --enable-module-ecdh=$ECDH --enable-module-recovery=$RECOVERY --enable-jni=$JNI $EXTRAFLAGS $USE_HOST
81+ - if [ -n "$BUILD" ]; then make -j2 $BUILD; fi
82+ - if [ -n "$VALGRIND" ]; then make -j2 && travis_wait 30 valgrind --error-exitcode=42 ./tests 8; fi

jonasnick commented at 6:11 pm on November 8, 2019:

What does travis_wait do and why error-exitcode=42? Would be good to add a comment. Also, how about also doing the exhaustive tests?

elichai commented at 7:37 pm on November 8, 2019:

about exhaustive test i’m not sure about time. But travis_wait is because if 10 minutes pass without anything printed to stdout travis will terminate it, so travis_wait silently print an empty char every minute for the written amount(30m) so it won’t timeout. the error code, as you can see in the manual http://valgrind.org/docs/manual/manual-core.html is 0 by default, meaning the tests will pass even though valgrind found errors.

jonasnick commented at 10:47 pm on November 8, 2019:

Exhaustive tests are over pretty quickly. It worked for me with everything enabled (including schnorrsig and non-endo) and without travis_wait, which seems pretty useful given that the maximum allowed time for a job is 50 minutes. I didn’t know about the valgrind exit code - makes sense. Would still be good to add a comment explaining that.

jonasnick commented at 6:12 pm on November 8, 2019: contributor

How about also adding an ENDOMORPHISM=no test as @real-or-random suggested in the original PR?

elichai force-pushed on Nov 9, 2019

elichai force-pushed on Nov 10, 2019

in .travis.yml:95 in eae17f2b32 outdated

91+ - # travis_wait extends the 10 minutes without output allowed (https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received)
92+ - # the `--error-exitcode` is required to make the test fail if valgrind found errors, otherwise it'll return 0 (http://valgrind.org/docs/manual/manual-core.html)
93+ - if [ -n "$VALGRIND" ]; then
94+   make -j2 &&
95+   travis_wait 30 valgrind --error-exitcode=42 ./tests 8 &&
96+   travis_wait 30 valgrind --error-exitcode=42 ./exhaustive_tests 8;

jonasnick commented at 7:47 pm on November 10, 2019:

nit: exhaustive_tests don’t take an argument

in .travis.yml:74 in eae17f2b32 outdated

69+        apt:
70+          packages:
71+            - valgrind
72+    - compiler: gcc
73+      env: # The same as above but without endomorphism.
74+        - BIGNUM=no  ENDOMORPHISM=no  STATICPRECOMPUTATION=yes  ASM=x86_64

jonasnick commented at 7:50 pm on November 10, 2019:

nit: STATICPRECOMPUTATION is yes by default

jonasnick commented at 12:29 pm on November 11, 2019:

nit: it’s still present in the endo valgrind run

elichai commented at 12:34 pm on November 11, 2019:

Totally forgot I split them in 2 :)

in .travis.yml:94 in eae17f2b32 outdated

90+ - if [ -n "$BUILD" ]; then make -j2 $BUILD; fi
91+ - # travis_wait extends the 10 minutes without output allowed (https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received)
92+ - # the `--error-exitcode` is required to make the test fail if valgrind found errors, otherwise it'll return 0 (http://valgrind.org/docs/manual/manual-core.html)
93+ - if [ -n "$VALGRIND" ]; then
94+   make -j2 &&
95+   travis_wait 30 valgrind --error-exitcode=42 ./tests 8 &&

jonasnick commented at 7:53 pm on November 10, 2019:

We could optimize the test runs parameter because it’s basically free. For example it looks like 16 would work just as well.

jonasnick commented at 7:54 pm on November 10, 2019: contributor

Looks good to me mod nits.

elichai force-pushed on Nov 11, 2019

travis: Added a valgrind test without endro and enabled recovery+ecdh dd98cc988f

elichai force-pushed on Nov 11, 2019

in .travis.yml:66 in dd98cc988f

60@@ -61,10 +61,36 @@ matrix:
61           packages:
62             - gcc-multilib
63             - libgmp-dev:i386
64+    - compiler: gcc
65+      env:
66+        - BIGNUM=no  ENDOMORPHISM=yes  ASM=x86_64 EXPERIMENTAL=yes ECDH=yes  RECOVERY=yes

jonasnick commented at 4:05 pm on November 11, 2019:

for the record: we don’t have to disable gmp to get a valgrind pass

elichai commented at 4:22 pm on November 11, 2019:

Yes, But I don’t see a lot of value in running valgrind on gmp and I’d rather it tested our implementations. although there’s quite a lot gmp wrapper code too.

gmaxwell commented at 8:26 pm on November 11, 2019:

You might want a second run with endo off. Otherwise you’re not testing the most common config with it, and there have been issues in the past that were valgrind clean with endo on but not off and vice versa. If speed is a concern, just decrease test count… running tests 4 twice is more testing than tests 8. If for some reason only one can be run, endo=off is probably better.

elichai commented at 8:28 pm on November 11, 2019:

There is :) https://github.com/bitcoin-core/secp256k1/pull/690/files#diff-354f30a63fb0907d4ad57269548329e3R74

jonasnick approved

jonasnick commented at 10:05 am on November 25, 2019: contributor

ACK dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7

real-or-random commented at 10:06 am on November 25, 2019: contributor

ACK dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7 I looked at the diff

real-or-random approved

jonasnick referenced this in commit 22a6031184 on Nov 25, 2019

jonasnick merged this on Nov 25, 2019

jonasnick closed this on Nov 25, 2019

deadalnix referenced this in commit 06836677ac on Mar 6, 2020

deadalnix referenced this in commit 14b0126878 on Mar 7, 2020

elichai deleted the branch on May 2, 2020

sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa

fanquake referenced this in commit 8c97780db8 on Jun 13, 2020

sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020

ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020

UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021

5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021

gades referenced this in commit d855cc511d on May 8, 2022

Add valgrind check to travis #690