Page MenuHomePhabricator

[CMAKE] Add support for generating test coverage reports
ClosedPublic

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

Details

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_BUILD_TYPE=Release -DENABLE_COVERAGE=ON

Should display a warning message: It is advised to not enforce CMAKE_BUILD_TYPE to get the best coverage results

cmake -GNinja .. \
  -DCMAKE_C_COMPILER=gcc \
  -DCMAKE_CXX_COMPILER=g++ \
  -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.

Try to run a few other coverage targets, like:

ninja coverage-check

or

ninja coverage-check-functional

With secp256k1 standalone:

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

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.Mar 19 2020, 00:16

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 ?

Add the COVERAGE definition to secp256k1.

deadalnix requested changes to this revision.Mar 19 2020, 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.Mar 19 2020, 13:24
Fabien planned changes to this revision.Mar 19 2020, 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.

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

deadalnix requested changes to this revision.Mar 19 2020, 14:10
deadalnix added inline comments.
CMakeLists.txt
69 ↗(On Diff #17035)

You decided this was a build type.

83 ↗(On Diff #17035)

If it interfers, then it probably worth reconsidering.

doc/developer-notes.md
299 ↗(On Diff #17035)

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

src/secp256k1/CMakeLists.txt
9 ↗(On Diff #17035)

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

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

Remove all reference to CMAKE_BUILD_TYPE.
Remove disabling ccache from the doc.
Include secp256k1 and seeder into coverage, as we run tests for them.

Remove the OverrideInitFlags change, no longer needed.

I think there is a problem with the way you handle the optimization, and various nits, but overall this looks solid.

CMakeLists.txt
77 ↗(On Diff #18758)

See comment in libsecp256k1

src/secp256k1/CMakeLists.txt
27 ↗(On Diff #18758)

glob is usually a bad idea due to dependency management. See for instance qt mocks that need to run for every build, because dependencies for them cannot be tracked.

30 ↗(On Diff #18758)

This is a bit of a problem because if I ask an optimized build, I should get one. It's okay for this to change defaults, but not superimpose user settings.

src/secp256k1/README.md
136 ↗(On Diff #18758)

You have twice the same option here.

test/CMakeLists.txt
73 ↗(On Diff #18758)

Does this work at all? I don't think anything in the test plan related to this.

src/secp256k1/CMakeLists.txt
27 ↗(On Diff #18758)

I agree with the general case, but in this case there is no dependency to track. I can just list the files if you're more comfortable with this solution, this is equivalent (and there are only 8 files), you tell me.

30 ↗(On Diff #18758)

I had the same questioning, this is also one of the reason the initial version used a coverage build type.
If you ask for an optimized build and coverage, you're basically asking for something that can't work (or badly, since the coverage result will still be generated but hard to understand).

I don't have a strong opinion of which option should have priority. Maybe a tradeoff would be to set some warning if the used specified a build type different from Debug with coverage enabled ? Otherwise I can just filter and let the user do what he wants, only overriding when there is no build type explicitly set. I don't have a strong opinion on what is a better UX here.

test/CMakeLists.txt
73 ↗(On Diff #18758)

Yes, it does. I just didn't put all the combinations in the test plan. Here is an example output (was generated before I added back secp256k1 and seeder, so they are missing) for the extended functional tests with upgrade activated:

coverage_functional_upgraded_extended.PNG (642×1 px, 95 KB)

deadalnix requested changes to this revision.Apr 16 2020, 13:37
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
27 ↗(On Diff #18758)

Listing them is an option. But maybe even better, is it possible to pass a glob to the script that generate the coverage report so it can exclude these files?

After all, there is no point having cmake track this.

30 ↗(On Diff #18758)

Well I know, but in that case, you'd want to have some error message or something, not doing something random that do not match what the user asked.

Maybe making coverage a build type is the way to go, but then you actually want to commit to that design decision, not do something where it's half a config, half a build type, like you previously did.

test/CMakeLists.txt
73 ↗(On Diff #18758)

I'm really puzzled that this works at all.

This revision now requires changes to proceed.Apr 16 2020, 13:37
src/secp256k1/CMakeLists.txt
27 ↗(On Diff #18758)

The script does not handle glob like pattern but searches for the string, so passing ${CMAKE_CURRENT_SOURCE_DIR}/src/bench should work, and is cleaner. Good idea !

30 ↗(On Diff #18758)

This is actually not possible to have it be a build type without also having an option (due to cmake limitations), so I'd better stick with this option solution. This is less elegant but at least it works.

I will find a solution to make it more user friendly.

Pass secp256k1 bench files directly to the filter with no glob.

The optimization level is overriden only when there is no user defined build type, and displays a warning otherwise.

I think there is a problem with the way you handle the optimization, and various nits, but overall this looks solid. Accepting for now, but please keep in mind the optimization thing need to be sorted out.

This revision is now accepted and ready to land.Apr 17 2020, 00:28