Improve precision of code coverage and add report to CI

real-or-random commented at 2:10 pm on June 17, 2021: contributor

real-or-random commented at 5:48 pm on June 17, 2021: contributor

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 commented at 2:35 pm on June 21, 2021: contributor

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 commented at 5:02 pm on June 21, 2021: contributor

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 force-pushed on Jun 22, 2021

real-or-random commented at 11:53 am on June 22, 2021: contributor

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 src/util.h:79 in 0965b2fd0a outdated

72@@ -59,7 +73,11 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
73 
74 /* Like assert(), but when VERIFY is defined, and side-effect safe. */
75 #if defined(COVERAGE)
76-#define VERIFY_CHECK(check)
77+/* Do nothing in coverage mode but try to stay syntactically correct.
78+   This suppresses a lot of implicit branches introduced by shortcutting
79+   operators at the cost of not being side-effect safe in coverage mode.
80+   We rely on the compiler to eliminate the if (0) statement entirely. */

real-or-random commented at 11:55 am on June 22, 2021:

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.

ci: Add code coverage report af0b3e6184

Use CHECK only in the tests be2df91385

Don't actually CHECK in tests in COVERAGE mode 38f84cefd1

Fix unused variable warnings in COVERAGE mode 249cdee08c

Exclude valgrind ctime test from coverage reports 6b8dd8a9b3

real-or-random force-pushed on Jun 22, 2021

real-or-random commented at 1:13 pm on June 22, 2021: contributor

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.

in src/util.h:54 in 6b8dd8a9b3

46@@ -43,7 +47,17 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
47 #define EXPECT(x,c) (x)
48 #endif
49 
50-#ifdef DETERMINISTIC
51+/* CHECK() is like assert(). We use it only in the tests. */
52+#if defined(COVERAGE)
53+/* Don't abort in coverage mode.
54+   This avoids branches which are not expected to be taken.
55+   We still use cond as usual to avoid unused variable warnings. */

real-or-random commented at 10:28 am on June 23, 2021:

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 commented at 11:00 am on June 23, 2021:

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

in src/util.h:32 in 6b8dd8a9b3

24@@ -25,7 +25,11 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
25     cb->fn(text, (void*)cb->data);
26 }
27 
28-#ifdef DETERMINISTIC
29+#if defined(COVERAGE)
30+/* Do nothing in COVERAGE mode. This will make the compiler optimize away the actual branch,
31+   and we get useful branch coverage within our test files.  */
32+#define TEST_FAILURE(msg)
33+#elif defined(DETERMINISTIC)

jonasnick commented at 10:57 am on June 23, 2021:

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

real-or-random commented at 1:58 pm on June 28, 2021: contributor

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 commented at 11:16 am on June 29, 2021: contributor

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 cross-referenced this on Jul 3, 2021 from issue WIP: Add gcov coverage report to Travis CI by real-or-random

real-or-random commented at 9:55 am on September 23, 2021: contributor

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 cross-referenced this on Mar 24, 2022 from issue Initial gcov work by cbatson

real-or-random commented at 10:13 pm on October 24, 2023: contributor

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

real-or-random added the label assurance on Oct 25, 2023

real-or-random added the label ci on Oct 25, 2023

Improve precision of code coverage and add report to CI #954