Page MenuHomePhabricator

[CMAKE] Add support for generating test coverage reports
Needs RevisionPublic

Authored by Fabien on Feb 24 2020, 21:42.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This is inspired by the autotools make cov feature, but is not
identical.

Main differences with core:

  • The way autotools coverage works requires the user to run the build

before calling the cov target, in order to build the baseline coverage
data. With cmake, the all target is built to regenerate the coverage
baseline data.

  • The coverage target is associated to the test target. So `ninja

coverage-check` build a coverage report for the check target, `ninja
coverage-check-functional` for the check-functional target, etc.
To get a global coverage, one can use ninja coverage-check-all or
ninja coverage-check-extended. It also avoids adding switch at build
time to run the extended tests (what autotools does).

Note: the -DUSE_COVERAGE=1 from core is not ported as it is no longer
in use since D5047.

For now only the association of lcov, gcov and genhtml is
supported (llvm-cov requires a wrapper to work as gcov).

Test Plan
cmake -GNinja .. \
  -DCMAKE_C_COMPILER=gcc \
  -DCMAKE_CXX_COMPILER=g++ \
  -DCCACHE=OFF \
  -DENABLE_COVERAGE=ON \
  -DENABLE_BRANCH_COVERAGE=ON
ninja coverage-check-all

Be patient, it takes a long time to complete.
Open the generated check-all.coverage/index.html and navigate through
the coverage report.

With secp256k1 standalone:

cmake -GNinja .. \
  -DCMAKE_C_COMPILER=gcc \
  -DCCACHE=OFF \
  -DSECP256K1_ENABLE_COVERAGE=ON \
  -DSECP256K1_ENABLE_BRANCH_COVERAGE=ON
ninja coverage-check-secp256k1

Diff Detail

Repository
rABC Bitcoin ABC
Branch
coverage_reworked
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9884
Build 17628: Bitcoin ABC Buildbot
Build 17627: arc lint + arc unit

Event Timeline

Fabien created this revision.Feb 24 2020, 21:42
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 24 2020, 21:42
Fabien updated this revision to Diff 16602.Feb 28 2020, 13:09

Rebase after D5319 update.

deadalnix added inline comments.
CMakeLists.txt
86 ↗(On Diff #16602)

Why?

Fabien added inline comments.Mar 2 2020, 10:51
CMakeLists.txt
86 ↗(On Diff #16602)

Because this is not part of the (main) node software, and it makes it easier to compare with core coverage values if needed.
Do you want me to remove the exclusion ?

Fabien planned changes to this revision.Mar 2 2020, 13:44
Fabien edited the summary of this revision. (Show Details)
Fabien updated this revision to Diff 16681.Mar 2 2020, 13:46
Fabien edited the summary of this revision. (Show Details)

Rebase on top of D5387 and add cmake instructions in secp256k1's README.

deadalnix requested changes to this revision.Mar 6 2020, 22:00
deadalnix added inline comments.
cmake/modules/Coverage.cmake
1 ↗(On Diff #16681)

I started digging into this, but I find it extremely hard to follow. I keep having to jump back and forth several screens and very little code make sense in isolation. This needs to be reorganized.

60 ↗(On Diff #16681)

There are only two call sites for this function, and it is defined 2 universes away. That makes the code very hard to follow, with a lot of jumping back and forth. This is big enough as it is.

140 ↗(On Diff #16681)

This is effectively a constant. Why does this need a variable expect to make the code more confusing?

src/secp256k1/CMakeLists.txt
11 ↗(On Diff #16681)

Why is coverage a build type? Tat does not make sense to me.

This revision now requires changes to proceed.Mar 6 2020, 22:00
Fabien planned changes to this revision.Mar 10 2020, 08:44
Fabien added inline comments.Thu, Mar 12, 14:18
src/secp256k1/CMakeLists.txt
11 ↗(On Diff #16681)

Having coverage as a build type is actually not a rare pattern (LLVM use it for example).
It solves a number of problems such as overriding the optimization flags, and can make sense if you consider that the build type (as debug or release) means nothing when building for coverage and can just give you wrong result due to optimization.

That said having to test the build type is a bad design and won't work with IDE based generators such as Xcode or VS, I'll need to find another solution.

Fabien edited the summary of this revision. (Show Details)Fri, Mar 13, 22:31
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 16931.Fri, Mar 13, 22:34
Fabien edited the summary of this revision. (Show Details)

Attempt to make the code easier to follow.
Add a new option to enable coverage, since generator expressions are not mature enough to avoid it using $<CONFIG>.
Various minor fixes and improvements.

deadalnix requested changes to this revision.Thu, Mar 19, 00:16

Well then, if it is a build type, then it is a build type, no? Not an option.

src/secp256k1/CMakeLists.txt
335 ↗(On Diff #16931)

I don't think that this is correct. You want VERIFY enabled for tests no matter if there is coverage or not. Is there something I'm missing?

This revision now requires changes to proceed.Thu, Mar 19, 00:16
Fabien added a comment.Thu, Mar 19, 09:59

Yes in the end I require an option because I need to be able to branch on coverage at configure time (or at build time but on properties that don't support generator expressions yet).
I use the build type only as a convenience to set the optimization flags and make the result configuration agnostic. If you are uncomfortable with this solution I can override the C/CXX flags directly.

src/secp256k1/CMakeLists.txt
335 ↗(On Diff #16931)

Honestly I don't know. This has been added in https://github.com/bitcoin-core/secp256k1/pull/428 but there is no comment on why this is required. My bet is that it will decrease coverage statistics when there is no failure, because the check failed branch is never covered.

Note that I am missing the COVERAGE definition (will update), that undefines VERIFY_CHECK as well:
from util.h

/* Like assert(), but when VERIFY is defined, and side-effect safe. */
#if defined(COVERAGE)
#define VERIFY_CHECK(check)
#define VERIFY_SETUP(stmt)
#elif defined(VERIFY)
#define VERIFY_CHECK CHECK
#define VERIFY_SETUP(stmt) do { stmt; } while(0)
#else
#define VERIFY_CHECK(cond) do { (void)(cond); } while(0)
#define VERIFY_SETUP(stmt)
#endif

Not sure what to do here, should I let VERIFY as standard and have different result than autotools ?

Fabien updated this revision to Diff 17034.Thu, Mar 19, 10:13

Add the COVERAGE definition to secp256k1.

deadalnix requested changes to this revision.Thu, Mar 19, 13:24
deadalnix added inline comments.
CMakeLists.txt
83 ↗(On Diff #17034)

Doesn't this interfere with secp256k1's coverage?

cmake/modules/TestSuite.cmake
31 ↗(On Diff #17034)

Why not inconditionally include sooner? I'm not 100% but cmake may well be processing this again and again every time it is included.

src/secp256k1/CMakeLists.txt
28 ↗(On Diff #17034)

Is there a reason not to use OverrideInitFlags ?

329 ↗(On Diff #17034)

Add a comment explaining why.

This revision now requires changes to proceed.Thu, Mar 19, 13:24
Fabien planned changes to this revision.Thu, Mar 19, 13:41

The diff is actually not the one I expected, I should have done something wrong. The previous one was correct though.

CMakeLists.txt
83 ↗(On Diff #17034)

It does unless you run from the standalone secp256k1 project (which doesn't have this file), in which case you get secp256k1's coverage only which is what you want.

cmake/modules/TestSuite.cmake
31 ↗(On Diff #17034)

Actually this is a bug, there should be no if(CMAKE_BUILD_TYPE STREQUAL "Coverage") anywhere.
I should have failed my rebase somehow, will update.

src/secp256k1/CMakeLists.txt
28 ↗(On Diff #17034)

secp256k1 doesn't use the override mechanism, as the initial reason for using it was removing the NDEBUG which is not an issue for secp256k1.
That said I don't see any reason why it can't be done, but I'll better make it another diff.

329 ↗(On Diff #17034)

Will do.

Fabien updated this revision to Diff 17035.Thu, Mar 19, 13:50

Add a comment as to why VERIFY shouldn't be defined with coverage.

deadalnix requested changes to this revision.Thu, Mar 19, 14:10
deadalnix added inline comments.
CMakeLists.txt
69

You decided this was a build type.

83

If it interfers, then it probably worth reconsidering.

doc/developer-notes.md
299

This seems to be out of date. Why disable CCACHE ?

src/secp256k1/CMakeLists.txt
9

You made the decision that Coverage was a build type. This is inconsistent with that decision.

This revision now requires changes to proceed.Thu, Mar 19, 14:10