build: Ensure no optimization when building for coverage analysis #1243

#944 introduced a regression when building for coverage analysis. The -O2 flag from the default Autoconf's CFLAGS overrides the coverage-specific -O0 one, which makes coverage analysis results less reliable.

This PR restores the pre-#944 behaviour.

In contrast to an alternative smaller diff:

--- a/configure.ac
+++ b/configure.ac
@@ -240,7 +240,7 @@ fi
 
 if test x"$enable_coverage" = x"yes"; then
     SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
-    SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
+    CFLAGS="$CFLAGS -O0 --coverage "
     LDFLAGS="--coverage $LDFLAGS"
 else
     # Most likely the CFLAGS already contain -O2 because that is autoconf's default.

this PR ensures that the user always has the last word.

FWIW, Bitcoin Core uses a similar approach.

Concept ACK

Good catch!

I first wondered if simply setting -g will result in the same problem as here. To be honest, I don't remember exactly how this caused a problem on mac.

But since you use a Mac, and since Core does the same, I assume your PR here does the right thing?

I first wondered if simply setting -g will result in the same problem as here. To be honest, I don't remember exactly how this caused a problem on mac.

The mentioned problem can be easily inducted by ./configure CFLAGS=-g on macOS. But that is the user's responsibility :)

https://lists.nongnu.org/archive/html/bug-autoconf/2007-11/msg00032.html:

So the Autoconf test that checks for -g support creates this directory
which is not removed by most rm -f conftest.% because most of the time
the % is explicitly given (e.g., that test runs "rm -f core *.core
core.conftest.* gmon.out bb.out conftest$ac_exeext conftest.$ac_objext
conftest.$ac_ext").  Other tests simply go for "rm -f conftest.*" and
fail on conftest.dSYM being a directory.

But since you use a Mac

Not my main machine :)

and since Core does the same

Not "the same", but similar. Bitcoin Core clears CFLAGS. This PR restores flags as they were before #944.

I remember getting quite unexpected output in a coverage report recently. This may have very well been the issue. Thanks!

tested ACK 8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea

utACK 8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea