Merge bitcoin-core/secp256k1#944: Various improvements related to CFLAGS

0302138f7508414e9e5212bc45b4ca4c0e5f081c ci: Make compiler warning into errors on CI (Tim Ruffing)
b924e1e605dcf9f9b362531184d16d643cc3baa9 build: Ensure that configure's compile checks default to -O2 (Tim Ruffing)
7939cd571c7a236f0d46e5cd7b6529ae29757c5a build: List *CPPFLAGS before *CFLAGS like on the compiler command line (Tim Ruffing)
595e8a35d80c932f91e810ce889c48b6efbaf890 build: Enable -Wcast-align=strict warning (Tim Ruffing)
07256267ffa9fb37609ec46260e9990bccd35dc5 build: Use own variable SECP_CFLAGS instead of touching user CFLAGS (Tim Ruffing)

Pull request description:

ACKs for top commit:
  jonasnick:
    ACK 0302138f7508414e9e5212bc45b4ca4c0e5f081c

Tree-SHA512: 619eb6b512ae0eb8c51134f5bb1b7bc7a397321dc51073ae3117f9433505ec19b407518b47a181163e1a841216b20487c7a50c6f5045faffa5cfa7fad0b8c906

.cirrus.yml

@@ -1,21 +1,28 @@
 env:
-  WIDEMUL: auto
+  ### compiler options
+  HOST:
+  # Specific warnings can be disabled with -Wno-error=foo.
+  # -pedantic-errors is not equivalent to -Werror=pedantic and thus not implied by -Werror according to the GCC manual.
+  WERROR_CFLAGS: -Werror -pedantic-errors
+  MAKEFLAGS: -j2
+  BUILD: check
+  ### secp256k1 config
   STATICPRECOMPUTATION: yes
   ECMULTGENPRECISION: auto
   ASM: no
-  BUILD: check
+  WIDEMUL: auto
   WITH_VALGRIND: yes
   EXTRAFLAGS:
-  HOST:
+  ### secp256k1 modules
+  EXPERIMENTAL: no
   ECDH: no
   RECOVERY: no
   SCHNORRSIG: no
-  EXPERIMENTAL: no
+  ### test options
-  CTIMETEST: yes
-  BENCH: yes
   TEST_ITERS:
+  BENCH: yes
   BENCH_ITERS: 2
-  MAKEFLAGS: -j2
+  CTIMETEST: yes
 
 cat_logs_snippet: &CAT_LOGS
   always:

.gitignore

@@ -23,6 +23,7 @@ aclocal.m4
 autom4te.cache/
 config.log
 config.status
+conftest*
 *.tar.gz
 *.la
 libtool

Makefile.am

@@ -1,5 +1,9 @@
 ACLOCAL_AMFLAGS = -I build-aux/m4
 
+# AM_CFLAGS will be automatically prepended to CFLAGS by Automake when compiling some foo
+# which does not have an explicit foo_CFLAGS variable set.
+AM_CFLAGS = $(SECP_CFLAGS)
+
 lib_LTLIBRARIES = libsecp256k1.la
 include_HEADERS = include/secp256k1.h
 include_HEADERS += include/secp256k1_preallocated.h
@@ -129,10 +133,10 @@ CPPFLAGS_FOR_BUILD +=-I$(top_srcdir) -I$(builddir)/src
 gen_context_OBJECTS = gen_context.o
 gen_context_BIN = gen_context$(BUILD_EXEEXT)
 gen_%.o: src/gen_%.c src/libsecp256k1-config.h
-	$(CC_FOR_BUILD) $(DEFS) $(CPPFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) -c $< -o $@
+	$(CC_FOR_BUILD) $(DEFS) $(CPPFLAGS_FOR_BUILD) $(SECP_CFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) -c $< -o $@
 
 $(gen_context_BIN): $(gen_context_OBJECTS)
-	$(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@
+	$(CC_FOR_BUILD) $(SECP_CFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@
 
 $(libsecp256k1_la_OBJECTS): src/ecmult_static_context.h
 $(tests_OBJECTS): src/ecmult_static_context.h

build-aux/m4/bitcoin_secp.m4

@@ -82,3 +82,19 @@ if test x"$has_valgrind" != x"yes"; then
   AC_CHECK_HEADER([valgrind/memcheck.h], [has_valgrind=yes; AC_DEFINE(HAVE_VALGRIND,1,[Define this symbol if valgrind is installed])])
 fi
 ])
+
+dnl SECP_TRY_APPEND_CFLAGS(flags, VAR)
+dnl Append flags to VAR if CC accepts them.
+AC_DEFUN([SECP_TRY_APPEND_CFLAGS], [
+  AC_MSG_CHECKING([if ${CC} supports $1])
+  SECP_TRY_APPEND_CFLAGS_saved_CFLAGS="$CFLAGS"
+  CFLAGS="$1 $CFLAGS"
+  AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])], [flag_works=yes], [flag_works=no])
+  AC_MSG_RESULT($flag_works)
+  CFLAGS="$SECP_TRY_APPEND_CFLAGS_saved_CFLAGS"
+  if test x"$flag_works" = x"yes"; then
+    $2="$$2 $1"
+  fi
+  unset flag_works
+  AC_SUBST($2)
+])

configure.ac

@@ -8,10 +8,6 @@ AH_TOP([#define LIBSECP256K1_CONFIG_H])
 AH_BOTTOM([#endif /*LIBSECP256K1_CONFIG_H*/])
 AM_INIT_AUTOMAKE([foreign subdir-objects])
 
-# Set -g if CFLAGS are not already set, which matches the default autoconf
-# behavior (see PROG_CC in the Autoconf manual) with the exception that we don't
-# set -O2 here because we set it in any case (see further down).
-: ${CFLAGS="-g"}
 LT_INIT
 
 # Make the compilation flags quiet unless V=1 is used.
@@ -70,35 +66,41 @@ case $host_os in
    ;;
 esac
 
-CFLAGS="-W $CFLAGS"
+# Try if some desirable compiler flags are supported and append them to SECP_CFLAGS.
+#
+# These are our own flags, so we append them to our own SECP_CFLAGS variable (instead of CFLAGS) as
+# recommended in the automake manual (Section "Flag Variables Ordering"). CFLAGS belongs to the user
+# and we are not supposed to touch it. In the Makefile, we will need to ensure that SECP_CFLAGS
+# is prepended to CFLAGS when invoking the compiler so that the user always has the last word (flag).
+#
+# Another advantage of not touching CFLAGS is that the contents of CFLAGS will be picked up by
+# libtool for compiling helper executables. For example, when compiling for Windows, libtool will
+# generate entire wrapper executables (instead of simple wrapper scripts as on Unix) to ensure
+# proper operation of uninstalled programs linked by libtool against the uninstalled shared library.
+# These executables are compiled from C source file for which our flags may not be appropriate,
+# e.g., -std=c89 flag has lead to undesirable warnings in the past.
+#
+# TODO We should analogously not touch CPPFLAGS and LDFLAGS but currently there are no issues.
+AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [
+    # Try to append -Werror=unknown-warning-option to CFLAGS temporarily. Otherwise clang will
+    # not error out if it gets unknown warning flags and the checks here will always succeed
+    # no matter if clang knows the flag or not.
+    SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS="$CFLAGS"
+    SECP_TRY_APPEND_CFLAGS([-Werror=unknown-warning-option], CFLAGS)
 
-warn_CFLAGS="-std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-unused-function -Wno-long-long -Wno-overlength-strings"
+    SECP_TRY_APPEND_CFLAGS([-std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef], $1) # GCC >= 3.0, -Wlong-long is implied by -pedantic.
-saved_CFLAGS="$CFLAGS"
+    SECP_TRY_APPEND_CFLAGS([-Wno-overlength-strings], $1) # GCC >= 4.2, -Woverlength-strings is implied by -pedantic.
-CFLAGS="$warn_CFLAGS $CFLAGS"
+    SECP_TRY_APPEND_CFLAGS([-Wall], $1) # GCC >= 2.95 and probably many other compilers
-AC_MSG_CHECKING([if ${CC} supports ${warn_CFLAGS}])
+    SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall.
-AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
+    SECP_TRY_APPEND_CFLAGS([-Wextra], $1) # GCC >= 3.4, this is the newer name of -W, which we don't use because older GCCs will warn about unused functions.
-    [ AC_MSG_RESULT([yes]) ],
+    SECP_TRY_APPEND_CFLAGS([-Wcast-align], $1) # GCC >= 2.95
-    [ AC_MSG_RESULT([no])
+    SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0
-      CFLAGS="$saved_CFLAGS"
+    SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only
-    ])
+    SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0
 
-saved_CFLAGS="$CFLAGS"
+    CFLAGS="$SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS"
-CFLAGS="-Wconditional-uninitialized $CFLAGS"
-AC_MSG_CHECKING([if ${CC} supports -Wconditional-uninitialized])
-AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-    [ AC_MSG_RESULT([yes]) ],
-    [ AC_MSG_RESULT([no])
-      CFLAGS="$saved_CFLAGS"
-    ])
-
-saved_CFLAGS="$CFLAGS"
-CFLAGS="-fvisibility=hidden $CFLAGS"
-AC_MSG_CHECKING([if ${CC} supports -fvisibility=hidden])
-AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-    [ AC_MSG_RESULT([yes]) ],
-    [ AC_MSG_RESULT([no])
-      CFLAGS="$saved_CFLAGS"
 ])
+SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS)
 
 ###
 ### Define config arguments
@@ -213,10 +215,14 @@ AM_CONDITIONAL([VALGRIND_ENABLED],[test "$enable_valgrind" = "yes"])
 
 if test x"$enable_coverage" = x"yes"; then
     AC_DEFINE(COVERAGE, 1, [Define this symbol to compile out all VERIFY code])
-    CFLAGS="-O0 --coverage $CFLAGS"
+    SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
     LDFLAGS="--coverage $LDFLAGS"
 else
-    CFLAGS="-O2 $CFLAGS"
+    # Most likely the CFLAGS already contain -O2 because that is autoconf's default.
+    # We still add it here because passing it twice is not an issue, and handling
+    # this case would just add unnecessary complexity (see #896).
+    SECP_CFLAGS="-O2 $SECP_CFLAGS"
+    SECP_CFLAGS_FOR_BUILD="-O2 $SECP_CFLAGS_FOR_BUILD"
 fi
 
 if test x"$req_asm" = x"auto"; then
@@ -351,6 +357,9 @@ if test x"$enable_valgrind" = x"yes"; then
   SECP_INCLUDES="$SECP_INCLUDES $VALGRIND_CPPFLAGS"
 fi
 
+# Add -Werror and similar flags passed from the outside (for testing, e.g., in CI)
+SECP_CFLAGS="$SECP_CFLAGS $WERROR_CFLAGS"
+
 # Handle static precomputation (after everything which modifies CFLAGS and friends)
 if test x"$use_ecmult_static_precomputation" != x"no"; then
   if test x"$cross_compiling" = x"no"; then
@@ -360,8 +369,9 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
     fi
     # If we're not cross-compiling, simply use the same compiler for building the static precompation code.
     CC_FOR_BUILD="$CC"
-    CFLAGS_FOR_BUILD="$CFLAGS"
     CPPFLAGS_FOR_BUILD="$CPPFLAGS"
+    SECP_CFLAGS_FOR_BUILD="$SECP_CFLAGS"
+    CFLAGS_FOR_BUILD="$CFLAGS"
     LDFLAGS_FOR_BUILD="$LDFLAGS"
   else
     AX_PROG_CC_FOR_BUILD
@@ -371,22 +381,14 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
     cross_compiling=no
     SAVE_CC="$CC"
     CC="$CC_FOR_BUILD"
-    SAVE_CFLAGS="$CFLAGS"
-    CFLAGS="$CFLAGS_FOR_BUILD"
     SAVE_CPPFLAGS="$CPPFLAGS"
     CPPFLAGS="$CPPFLAGS_FOR_BUILD"
+    SAVE_CFLAGS="$CFLAGS"
+    CFLAGS="$CFLAGS_FOR_BUILD"
     SAVE_LDFLAGS="$LDFLAGS"
     LDFLAGS="$LDFLAGS_FOR_BUILD"
 
-    warn_CFLAGS_FOR_BUILD="-Wall -Wextra -Wno-unused-function"
+    SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS_FOR_BUILD)
-    saved_CFLAGS="$CFLAGS"
-    CFLAGS="$warn_CFLAGS_FOR_BUILD $CFLAGS"
-    AC_MSG_CHECKING([if native ${CC_FOR_BUILD} supports ${warn_CFLAGS_FOR_BUILD}])
-    AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
-        [ AC_MSG_RESULT([yes]) ],
-        [ AC_MSG_RESULT([no])
-          CFLAGS="$saved_CFLAGS"
-        ])
 
     AC_MSG_CHECKING([for working native compiler: ${CC_FOR_BUILD}])
     AC_RUN_IFELSE(
@@ -394,19 +396,17 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
       [working_native_cc=yes],
       [working_native_cc=no],[:])
 
-    CFLAGS_FOR_BUILD="$CFLAGS"
-
     # Restore the environment
     cross_compiling=$save_cross_compiling
     CC="$SAVE_CC"
-    CFLAGS="$SAVE_CFLAGS"
     CPPFLAGS="$SAVE_CPPFLAGS"
+    CFLAGS="$SAVE_CFLAGS"
     LDFLAGS="$SAVE_LDFLAGS"
 
     if test x"$working_native_cc" = x"no"; then
       AC_MSG_RESULT([no])
       set_precomp=no
-      m4_define([please_set_for_build], [Please set CC_FOR_BUILD, CFLAGS_FOR_BUILD, CPPFLAGS_FOR_BUILD, and/or LDFLAGS_FOR_BUILD.])
+      m4_define([please_set_for_build], [Please set CC_FOR_BUILD, CPPFLAGS_FOR_BUILD, CFLAGS_FOR_BUILD, and/or LDFLAGS_FOR_BUILD.])
       if test x"$use_ecmult_static_precomputation" = x"yes";  then
         AC_MSG_ERROR([native compiler ${CC_FOR_BUILD} does not produce working binaries. please_set_for_build])
       else
@@ -419,8 +419,9 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
   fi
 
   AC_SUBST(CC_FOR_BUILD)
-  AC_SUBST(CFLAGS_FOR_BUILD)
   AC_SUBST(CPPFLAGS_FOR_BUILD)
+  AC_SUBST(SECP_CFLAGS_FOR_BUILD)
+  AC_SUBST(CFLAGS_FOR_BUILD)
   AC_SUBST(LDFLAGS_FOR_BUILD)
 else
   set_precomp=no
@@ -490,6 +491,7 @@ AC_SUBST(SECP_INCLUDES)
 AC_SUBST(SECP_LIBS)
 AC_SUBST(SECP_TEST_LIBS)
 AC_SUBST(SECP_TEST_INCLUDES)
+AC_SUBST(SECP_CFLAGS)
 AM_CONDITIONAL([ENABLE_COVERAGE], [test x"$enable_coverage" = x"yes"])
 AM_CONDITIONAL([USE_TESTS], [test x"$use_tests" != x"no"])
 AM_CONDITIONAL([USE_EXHAUSTIVE_TESTS], [test x"$use_exhaustive_tests" != x"no"])
@@ -532,13 +534,15 @@ fi
 echo
 echo "  valgrind                = $enable_valgrind"
 echo "  CC                      = $CC"
-echo "  CFLAGS                  = $CFLAGS"
 echo "  CPPFLAGS                = $CPPFLAGS"
+echo "  SECP_CFLAGS             = $SECP_CFLAGS"
+echo "  CFLAGS                  = $CFLAGS"
 echo "  LDFLAGS                 = $LDFLAGS"
 echo
 if test x"$set_precomp" = x"yes"; then
 echo "  CC_FOR_BUILD            = $CC_FOR_BUILD"
-echo "  CFLAGS_FOR_BUILD        = $CFLAGS_FOR_BUILD"
 echo "  CPPFLAGS_FOR_BUILD      = $CPPFLAGS_FOR_BUILD"
+echo "  SECP_CFLAGS_FOR_BUILD   = $SECP_CFLAGS_FOR_BUILD"
+echo "  CFLAGS_FOR_BUILD        = $CFLAGS_FOR_BUILD"
 echo "  LDFLAGS_FOR_BUILD       = $LDFLAGS_FOR_BUILD"
 fi