cmake: Drop support for coverage in combination with multi-generator

[josibake]

Opening this to solicit feedback. Currently, its only possible to generate a coverage report by using the make / autoconf build system. Ideally, the developer can use either CMake or make and be able to generate coverage reports. I haven't spent much time researching how to do this with CMake, but some quick googling seems to indicate that it is at least possible, e.g., https://danielsieger.com/blog/2024/08/03/code-coverage-with-cmake.html.

I'll update this issue periodically as I come back to this, but also interested to hear if others think this is worth it or have recommendations on the approach.

cc @hebasto

[real-or-random]

This is supported. We currently have a "Coverage" build type, but we figured out that using a build type (like Release, Debug, ...) may be the wrong approach because it results in issues with CMake multi-config generators.

See #1592, #1291, #1251.

[josibake]

Nice, just confirmed I was able to build the same coverage report using cmake -B build -DCMAKE_BUILD_TYPE=Coverage. I incorrectly assumed we couldn't do this with CMake since the docs only list commands for make.

but we figured out that using a build type (like Release, Debug, ...) may be the wrong approach because

I'm guessing this is the reason the documentation hasn't been updated, i.e., this is expected to change? I'll take a look at the mentioned PRs to see if I can help there, but also happy to update the docs if what we have now is "good enough."

[real-or-random]

Let's see if we can pick up the work again. :)

@hebasto I think the last real activity was me asking why #1592 is better than #1291. It's probably also worth taking a look at the blog post mentioned by josie. It just builds a separate test binary for coverage. Perhaps this is the simple solution we've been overlooking all the time? This approach gets rid of the need to perform the entire build in a special coverage "mode" (however this is implemented). You could just build the coverage tests along with all normal tests. The same should work with autotools.

[hebasto]

This is supported. We currently have a "Coverage" build type...

The following is an example workflow on Ubuntu 24.04:

cmake -B build -DCMAKE_GENERATOR=Ninja -DCMAKE_BUILD_TYPE=Coverage
cmake --build build  # Ninja uses all available cores by default
ctest --test-dir build -j $(nproc)
mkdir -p build/coverage
gcovr --merge-mode-functions=separate --exclude 'src/bench*' --html --html-details -o build/coverage/coverage.html
firefox build/coverage/coverage.html

... but we figured out that using a build type (like Release, Debug, ...) may be the wrong approach because it results in issues with CMake multi-config generators.

For the multi-config generators, please see an example in #1592 (comment).

[hebasto]

Let's see if we can pick up the work again. :)

There are many code coverage implementations, and two of them are most notable.

First one is based on the gcov tool. The basic guidance is to compile without optimization, i.e., with -O0. n CMake, the general approach is to encapsulate optimisation- and debug-level flags into an abstraction called a build/configuration type. This ensures there are no conflicts between flags specifying different optimisation levels, and works particularly well with multi-config generators. In Professional CMake: A Practical Guide 21st Edition, section 33.2. Code Coverage, the author suggests the same approach.

There is a caveat specific to the libsecp256k1 code: when VERIFY is defined, it makes the resulting coverage reports meaningless. Therefore, the tests executable should not be built instrumented for coverage analysis at all. This is not a trivial task with CMake's multi-config generators. #1592 addressed this issue, given that in practice coverage reports using GCC + gcov could be limited to the “Ninja Multi-Config” generator.

Second is Clang Source-based Coverage, which claims that:

llvm optimizations (such as inlining or CFG simplification) should have no impact on coverage report quality.

This is not documented in this project.

A Note from Bitcoin Core. Historically, Bitcoin Core used the first method. Recently, many developers have been leaning towards the second.

@hebasto I think the last real activity was me asking why #1592 is better than #1291.

When using the SECP256K1_COVERAGE configuration option with multi-config generators, we have to deal with optimization-level flags in already defined build types. We could either (1) overwrite configuration-specific flags for every configuration, which is widely agreed to be poor practice, or (2) specify an additional non-defined build type, e.g., NONE, to nullify them, which is awkward.

It's probably also worth taking a look at the blog post mentioned by josie. It just builds a separate test binary for coverage. Perhaps this is the simple solution we've been overlooking all the time? This approach gets rid of the need to perform the entire build in a special coverage "mode" (however this is implemented). You could just build the coverage tests along with all normal tests. The same should work with autotools.

At present, two test executables generate data for coverage analysis: noverify_tests and exhaustive_tests.

[hebasto]

@josibake @real-or-random

Please also consider #1251 (comment).

[josibake]

Thanks for the detailed update, @hebasto. I'll read through and try to test the various approaches this week and update here with what I find/learn.

[real-or-random]

All that multi-generator stuff: What about simply dropping support for coverage + multi-generator? I'm not sure if anyone uses the multi-generator stuff at all. I don't think it's unreasonable to ask developers to turn it off if they want coverage reports. At least, that's the simplest thing we can do.

edit: This is particularly true if the best thing we can come up with (#1592) needs a special case for "Ninja Multi-Config". This suggests to me that giving up is a meaningful option.

It's probably also worth taking a look at the blog post mentioned by josie. It just builds a separate test binary for coverage. Perhaps this is the simple solution we've been overlooking all the time? This approach gets rid of the need to perform the entire build in a special coverage "mode" (however this is implemented). You could just build the coverage tests along with all normal tests. The same should work with autotools.

At present, two test executables generate data for coverage analysis: tests and exhaustive_tests.

Indeed, and we could add separate executables coverage_tests and coverage_exhaustive_tests? Would this make things simpler? The nice thing about this is really that you get can get coverage binaries in normal builds, just as a side product.

Second is Clang Source-based Coverage, which claims that:

llvm optimizations (such as inlining or CFG simplification) should have no impact on coverage report quality.

This is not documented in this project.

A Note from Bitcoin Core. Historically, Bitcoin Core used the first method. Recently, many developers have been leaning towards the second.

If you ask me, it's fine to switch to that one if it's just better overall. But I assume that's a bigger project?

[hebasto]

All that multi-generator stuff: What about simply dropping support for coverage + multi-generator? I'm not sure if anyone uses the multi-generator stuff at all. I don't think it's unreasonable to ask developers to turn it off if they want coverage reports. At least, that's the simplest thing we can do.

edit: This is particularly true if the best thing we can come up with (#1592) needs a special case for "Ninja Multi-Config". This suggests to me that giving up is a meaningful option.

Yes, this option is the simplest and seems quite reasonable to me. The current code in the master branch supports this scenario just fine.

It's probably also worth taking a look at the blog post mentioned by josie. It just builds a separate test binary for coverage. Perhaps this is the simple solution we've been overlooking all the time? This approach gets rid of the need to perform the entire build in a special coverage "mode" (however this is implemented). You could just build the coverage tests along with all normal tests. The same should work with autotools.

At present, two test executables generate data for coverage analysis: tests and exhaustive_tests.

Indeed, and we could add separate executables coverage_tests and coverage_exhaustive_tests? Would this make things simpler? The nice thing about this is really that you get can get coverage binaries in normal builds, just as a side product.

Perhaps I'm missing some key point, but I don't think we can get coverage binaries as a side product in normal builds, at least not for free. We would need to ensure that every object is compiled twice: once without coverage flags for the normal executables, and a second time with coverage flags for the coverage executables.

Second is Clang Source-based Coverage, which claims that:

llvm optimizations (such as inlining or CFG simplification) should have no impact on coverage report quality.

This is not documented in this project.
A Note from Bitcoin Core. Historically, Bitcoin Core used the first method. Recently, many developers have been leaning towards the second.

If you ask me, it's fine to switch to that one if it's just better overall. But I assume that's a bigger project?

I don't see any compelling reasons to spend time and efforts on this at the moment. However, if it ever becomes necessary, the approach mentioned in #1716 (comment) could be extended quite easily to support Clang Source-based Coverage.

[real-or-random]

Perhaps I'm missing some key point, but I don't think we can get coverage binaries as a side product in normal builds, at least not for free. We would need to ensure that every object is compiled twice: once without coverage flags for the normal executables, and a second time with coverage flags for the coverage executables.

Sure, every object needs to be compiled twice. It's certainly not free in terms of build time (but that's okay if the user needs to enable it with a variable like SECP256K1_COVERAGE). After looking at the blog post, my impression was that this is easy to do in CMake, so it's (almost) free in terms of build system complexity/maintenance. But I may be wrong! I didn't really think this through.

[hebasto]

Here is another link to gcovr cookbook: https://gcovr.com/en/stable/cookbook.html#oos-cmake.

[real-or-random]

All that multi-generator stuff: What about simply dropping support for coverage + multi-generator? I'm not sure if anyone uses the multi-generator stuff at all. I don't think it's unreasonable to ask developers to turn it off if they want coverage reports. At least, that's the simplest thing we can do.
edit: This is particularly true if the best thing we can come up with (#1592) needs a special case for "Ninja Multi-Config". This suggests to me that giving up is a meaningful option.

Yes, this option is the simplest and seems quite reasonable to me. The current code in the master branch supports this scenario just fine.

Sounds good to me. Let's do this for now. Are you willing to open a PR?