Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1243: build: Ensure no optimization when…
Browse files Browse the repository at this point in the history
… building for coverage analysis

8e79c7e build: Ensure no optimization when building for coverage analysis (Hennadii Stepanov)

Pull request description:

  bitcoin#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](https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html).

  This PR restores the pre-bitcoin#944 behaviour.

  In contrast to an alternative smaller diff:
  ```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](https://github.com/bitcoin/bitcoin/blob/460e394625fab2942748aaeec9be31f460f91c58/configure.ac#L879-L884).

ACKs for top commit:
  jonasnick:
    tested ACK 8e79c7e
  real-or-random:
    utACK 8e79c7e

Tree-SHA512: f04b55921d397bd7c003ec0283101d3908f3fb507789c855e1b6d5abd150e7d6281d5eeb8fefbb7d6a55b3c6f29a19324f570eee009794f8fa9bca956229e7ce
  • Loading branch information
real-or-random committed Mar 21, 2023
2 parents 427bc3c + 8e79c7e commit 0cf2fb9
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ AM_INIT_AUTOMAKE([1.11.2 foreign subdir-objects])
# Make the compilation flags quiet unless V=1 is used.
m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])])

if test "${CFLAGS+set}" = "set"; then
CFLAGS_overridden=yes
else
CFLAGS_overridden=no
fi
AC_PROG_CC
AM_PROG_AS
AM_PROG_AR
Expand Down Expand Up @@ -241,6 +246,12 @@ fi
if test x"$enable_coverage" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
# If coverage is enabled, and the user has not overridden CFLAGS,
# override Autoconf's value "-g -O2" with "-g". Otherwise we'd end up
# with "-O0 --coverage -g -O2".
if test "$CFLAGS_overridden" = "no"; then
CFLAGS="-g"
fi
LDFLAGS="--coverage $LDFLAGS"
else
# Most likely the CFLAGS already contain -O2 because that is autoconf's default.
Expand Down

0 comments on commit 0cf2fb9

Please sign in to comment.