Improve precision of code coverage and add report to CI by real-or-random · Pull Request #954 · bitcoin-core/secp256k1

real-or-random · 2021-06-17T14:10:43Z

No description provided.

real-or-random · 2021-06-17T17:48:08Z

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 · 2021-06-21T17:02:30Z

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 · 2021-06-22T11:53:08Z

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 · 2021-06-22T11:55:50Z

src/util.h

+/* Do nothing in coverage mode but try to stay syntactically correct.
+   This suppresses a lot of implicit branches introduced by shortcutting
+   operators at the cost of not being side-effect safe in coverage mode.
+   We rely on the compiler to eliminate the if (0) statement entirely. */


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 · 2021-06-22T13:13:00Z

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 · 2021-06-23T10:28:03Z

src/util.h

+#if defined(COVERAGE)
+/* Don't abort in coverage mode.
+   This avoids branches which are not expected to be taken.
+   We still use cond as usual to avoid unused variable warnings. */


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.)

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

jonasnick · 2021-06-23T10:57:14Z

src/util.h

+#if defined(COVERAGE)
+/* Do nothing in COVERAGE mode. This will make the compiler optimize away the actual branch,
+   and we get useful branch coverage within our test files.  */
+#define TEST_FAILURE(msg)
+#elif defined(DETERMINISTIC)


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

jonasnick · 2021-06-23T11:00:17Z

src/util.h

+#if defined(COVERAGE)
+/* Don't abort in coverage mode.
+   This avoids branches which are not expected to be taken.
+   We still use cond as usual to avoid unused variable warnings. */


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

real-or-random · 2021-06-28T13:58:44Z

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 · 2021-06-29T11:16:03Z

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 · 2021-09-23T09:55:41Z

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 · 2023-10-24T22:13:18Z

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

jonasnick reviewed Jun 21, 2021

View reviewed changes

real-or-random force-pushed the 202106-wip-codecov branch from 9447601 to 0965b2f Compare June 22, 2021 10:29

real-or-random commented Jun 22, 2021

View reviewed changes

real-or-random added 5 commits June 22, 2021 15:10

ci: Add code coverage report

af0b3e6

Use CHECK only in the tests

be2df91

Don't actually CHECK in tests in COVERAGE mode

38f84ce

Fix unused variable warnings in COVERAGE mode

249cdee

Exclude valgrind ctime test from coverage reports

6b8dd8a

real-or-random force-pushed the 202106-wip-codecov branch from 0965b2f to 6b8dd8a Compare June 22, 2021 13:11

real-or-random commented Jun 23, 2021

View reviewed changes

jonasnick reviewed Jun 23, 2021

View reviewed changes

real-or-random mentioned this pull request Jul 3, 2021

WIP: Add gcov coverage report to Travis CI #705

Closed

real-or-random mentioned this pull request Mar 24, 2022

Initial gcov work #219

Closed

real-or-random added assurance ci labels Oct 25, 2023

Conversation

real-or-random commented Jun 17, 2021

Uh oh!

real-or-random commented Jun 17, 2021

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Jun 21, 2021

Uh oh!

real-or-random commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

real-or-random Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

real-or-random Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasnick Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

jonasnick Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

jonasnick Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Jun 28, 2021

Uh oh!

real-or-random commented Jun 29, 2021

Uh oh!

real-or-random commented Sep 23, 2021

Uh oh!

real-or-random commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

real-or-random commented Jun 22, 2021 •

edited

Loading

real-or-random commented Jun 22, 2021 •

edited

Loading

real-or-random Jun 23, 2021 •

edited

Loading