Improve precision of code coverage and add report to CI

[real-or-random]

No description provided.

[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

[jonasnick]

Very nice, concept ACK.

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.

[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]

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]

This is what GCC does. I can't really test with clang because the coverage report is empty, even though it should work with clang too when passing --gcov-executable="llvm-cov gcov" to gcovr. No idea what's wrong or if this is a bug somewhere in the tools.

[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]

s/unused variable/unused return values/

(or actually both, but the "unused variables" can be suppressed by casting to void, which would the thing most people would expect.)

[jonasnick]

Perhaps also note that this works because an if statement with an empty consequent is not treated as a branch.

[jonasnick]

This seems to be unnecessary now that the definition of CHECK is changed.

[jonasnick]

Perhaps also note that this works because an if statement with an empty consequent is not treated as a branch.

[real-or-random]

I still need to update this but let me note that #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]

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]

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.

20abd52#diff-c2d5f1f7616875ab71cd41b053cfb428696988ff89642b931a0963d50f34f7e8R3438-R3439

[real-or-random]

I should rework this for GitHub Actions. https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/ will be useful.