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

pull real-or-random wants to merge 5 commits into bitcoin-core:master from real-or-random:202106-wip-codecov changing 5 files +58 −11
  1. real-or-random commented at 2:10 pm on June 17, 2021: contributor
  2. 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

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

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

  5. real-or-random force-pushed on Jun 22, 2021
  6. 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.

  7. 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.
  8. ci: Add code coverage report af0b3e6184
  9. Use CHECK only in the tests be2df91385
  10. Don't actually CHECK in tests in COVERAGE mode 38f84cefd1
  11. Fix unused variable warnings in COVERAGE mode 249cdee08c
  12. Exclude valgrind ctime test from coverage reports 6b8dd8a9b3
  13. real-or-random force-pushed on Jun 22, 2021
  14. 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.

  15. 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.
  16. 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.
  17. 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).

  18. 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.
  19. real-or-random cross-referenced this on Jul 3, 2021 from issue WIP: Add gcov coverage report to Travis CI by real-or-random
  20. real-or-random cross-referenced this on Mar 24, 2022 from issue Initial gcov work by cbatson
  21. 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.
  22. real-or-random added the label assurance on Oct 25, 2023
  23. real-or-random added the label ci on Oct 25, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 03:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me