secp256k1
secp256k1 copied to clipboard
Improve precision of code coverage and add report to CI
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
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.
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.
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.
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).
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.
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