secp256k1 icon indicating copy to clipboard operation
secp256k1 copied to clipboard

Improve precision of code coverage and add report to CI

Open real-or-random opened this issue 4 years ago • 8 comments

real-or-random avatar Jun 17 '21 14:06 real-or-random

The coverage reports are linked in the Cirrus web interface, e.g., from https://cirrus-ci.com/task/4981579249352704.

Here's a direct link for reference: https://api.cirrus-ci.com/v1/artifact/task/4981579249352704/coverage_report/index.html

real-or-random avatar Jun 17 '21 17:06 real-or-random

this shows some unreachable verify_check branches as uncovered.

Good catch. This seems to be a consequence of the commit that silences the unused variable warnings. Not sure if there's a way to achieve both, but if not it would indeed be better to avoid false reporting.

Yes, this is just a bug in this commit, I'll update.

real-or-random avatar Jun 21 '21 17:06 real-or-random

Okay, updated. Now this suppresses the evaluation of the conditions in VERIFY_CHECK, which avoids the VERIFY_CHECK(... && ...) branches (the shortcutting operator introduces a branch) but while still keeping the code in the source to avoid "unused variable" warnings.

We can't do the same for CHECK because we have a lot of CHECKs with side-effects. That is the false positive rate for the branch coverage in tests.c can still be improved, e.g., by avoiding && and || in CHECKs in a further PR.

Nevertheless, this exposes a minor bug in the tests, where the "then" branch here is never taken for count == 64: https://github.com/bitcoin-core/secp256k1/blob/master/src/tests.c#L3366 see https://api.cirrus-ci.com/v1/artifact/task/6531663917219840/coverage_report/index.src_tests.c.html Even though this bug is obvious also from the line (not branch) coverage report alone.

real-or-random avatar Jun 22 '21 11:06 real-or-random

In case anyone is wondering (I was): gcovr correctly collects data from multiple binaries and merges the data, i.e., it's okay to run gen_context, tests and tests_exhaustive in a row. It just means we can't see from the report which binary hit which line.

(But I pushed again to avoid exposing the raw gcov files in CI. I think those are just properly merged and anyway not very useful).

Conceptually gcovr also supports merging results from multiple compilations with different options (e.g., WIDEMUL) using the --add-tracefile option but this doesn't fit nicely in our CI matrix (we'd need a special job that builds multiple configuration) and it's not tremendously useful: we just need to look at two reports instead of one.

real-or-random avatar Jun 22 '21 13:06 real-or-random

I still need to update this but let me note that https://github.com/bitcoin-core/secp256k1/pull/959 is a nice example of why we want to test branch coverage even in the tests.

Although for this specific case and this broken line, the report here doesn't show a branch at all... I suspect the branch is optimized away with an optimization so "trivial" that is not even disabled at -O0 (like for example the if (0) stuff in this PR, which apparently doesn't count as branch either).

real-or-random avatar Jun 28 '21 13:06 real-or-random

Further note: Having a separate report for the cttime tests is helpful for checking that all public branches are covered: Valgrind will complain on an executed branch on secret data but not if that secret branch is never reached due to an earlier branch on public data.

real-or-random avatar Jun 29 '21 11:06 real-or-random

There's a bug on current master which would have been caught by this PR, see https://gnusha.org/secp256k1/2021-09-22.log for details.

https://github.com/bitcoin-core/secp256k1/commit/20abd52c2e107e79391a19d2d2f8845e83858dea#diff-c2d5f1f7616875ab71cd41b053cfb428696988ff89642b931a0963d50f34f7e8R3438-R3439

real-or-random avatar Sep 23 '21 09:09 real-or-random