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

1. 
   hebasto commented at 7:51 pm on March 14, 2023: member

   #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:

    0--- a/configure.ac
    1+++ b/configure.ac
    2@@ -240,7 +240,7 @@ fi
    3 
    4 if test x"$enable_coverage" = x"yes"; then
    5     SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
    6-    SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
    7+    CFLAGS="$CFLAGS -O0 --coverage "
    8     LDFLAGS="--coverage $LDFLAGS"
    9 else
   10     # 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.

2. build: Ensure no optimization when building for coverage analysis 8e79c7ed11
3. 
   real-or-random commented at 3:33 pm on March 17, 2023: contributor

   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?

4. 
   hebasto commented at 4:51 pm on March 17, 2023: member

   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.

5. 
   jonasnick commented at 7:38 pm on March 20, 2023: contributor

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

   tested ACK 8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea

6. real-or-random approved
7. 
   real-or-random commented at 8:54 am on March 21, 2023: contributor

   utACK 8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea
8. real-or-random merged this on Mar 21, 2023
9. real-or-random closed this on Mar 21, 2023

   ---

10. hebasto deleted the branch on Mar 21, 2023
11. sipa referenced this in commit e1552d578e on Apr 11, 2023
12. sipa referenced this in commit c981671e9b on Apr 14, 2023
13. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
14. RandyMcMillan referenced this in commit 3cc75121b3 on May 27, 2023
15. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
16. vmta referenced this in commit 8f03457eed on Jul 1, 2023
17. alokeutpal approved
18. jonasnick cross-referenced this on Jul 21, 2023 from issue Upstream PRs 1228, 1236, 1243, 1238, 1246, 1247, 1242, 1250, 1244, 1241, 1257, 1226, 1252, 1118, 1245, 1266, 1269 by jonasnick
19. jonasnick cross-referenced this on Jul 21, 2023 from issue Upstream PRs 1228, 1236, 1243, 1238, 1246, 1247, 1242, 1250, 1244, 1241, 1257, 1226, 1252, 1118, 1245, 1266, 1269 by jonasnick

Contributors
hebasto real-or-random jonasnick alokeutpal