Context isn't freed in the ECDH benchmark

[elichai]

Hi,
Currently the context is leaked in bench_ecdh.
Wanted to prevent this happening in the future by adding it to the valgrind run in travis.
for that I made run_benchmark pass the number of iterations as a variable into the benchmarks so that the iteration number it uses to print results actually matches the one used in the benchmark.
and added a way to pass -DITERS=N to change the baseline number of iterations used in the benchmarks(so that we can run the benchmarks with ITERS=1 under travis to make sure they compile, run, don't leak or have UB)

[real-or-random]

Concept ACK.

This is neat. Travis fails at the moment: https://travis-ci.org/bitcoin-core/secp256k1/jobs/658249366#L447

A few observations:

• If we already make iter configurable, wouldn't it be more convenient to obtain the parameter at runtime?
• Should we make count configurable too?

[elichai]

* If we already make `iter` configurable, wouldn't it be more convenient to obtain the parameter at runtime?

That's what I did at first, but it somewhat collides with have_flag so I decided to start with a simpler version and if people think it's a useful feature I can implement have_flag in a way that will work both with the number of iterations and the flags without ambiguity(ie --tests=N)

* Should we make `count` configurable too?

Count doesn't seem to affect performance that match, so I didn't need to do that. but if we'll add runtime configurations we can definitely add that too.

[elichai]

Concept ACK.

This is neat. Travis fails at the moment: https://travis-ci.org/bitcoin-core/secp256k1/jobs/658249366#L447

Right, forgot I need to check if recovery and/or ecdh are compiled or not. that will be a big enough logic that I'm thinking of moving this to autotools maybe (although that will make it hard again to run under valgrind) Will fix this later/tomorrow and see how complex it will look

[sipa]

configure: error: unrecognized option: `-DITERS=1'

[jonasnick]

I agree that it would be better to make the benchmark iterations runtime configurable. Perhaps getenv would be an acceptable workaround?

[real-or-random]

I agree that it would be better to make the benchmark iterations runtime configurable. Perhaps getenv would be an acceptable workaround?

I think an environment variable is not bad because it can persist and it applies to all benchmarks. That fits our usage of the benchmarks.

[real-or-random]

ACK except nit

[real-or-random]

ACK 31c3d6c I've looked at the diff but I haven't tested the changes

[gmaxwell]

FWIW, when you setup 'fast' CI tests, please at least run them with 2 iterations, not 1. 1 iteration will fail to catch some kinds of bugs in the test harnesses, e.g. use after free if the deallocations are in the wrong place with the testing loops, etc. If 2 is too much, maybe resize the underlying work units so that they're not.

[jonasnick]

ACK mod nit

[jonasnick]

ACK 85b35af

[real-or-random]

ACK 85b35af I looked at the diff and tested the ecdh benchmark using valgrind

[real-or-random]

This is minor inconvenience that I can fix on the way, e.g. in #723:

> SECP256K1_BENCH_ITERS= ./bench_ecdh 
[1]    96654 floating point exception (core dumped)