From 07256267ffa9fb37609ec46260e9990bccd35dc5 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 13 May 2021 17:06:16 +0200 Subject: [PATCH 1/5] build: Use own variable SECP_CFLAGS instead of touching user CFLAGS Fixes one of the items in #923, namely the warnings of the form '_putenv' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] This also cleans up the way we add CFLAGS, in particular flags enabling warnings. Now we perform some more fine-grained checking for flag support, which is not strictly necessary but the changes also help to document autoconf.ac. --- Makefile.am | 8 +++- build-aux/m4/bitcoin_secp.m4 | 16 +++++++ configure.ac | 83 ++++++++++++++++++------------------ 3 files changed, 64 insertions(+), 43 deletions(-) diff --git a/Makefile.am b/Makefile.am index 23b29281df882..1e03560884338 100644 --- a/Makefile.am +++ b/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 diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4 index e57888ca18968..8245b2b863520 100644 --- a/build-aux/m4/bitcoin_secp.m4 +++ b/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) +]) diff --git a/configure.ac b/configure.ac index 1ed991afa7714..aaca5343063c3 100644 --- a/configure.ac +++ b/configure.ac @@ -70,35 +70,41 @@ case $host_os in ;; esac -CFLAGS="-W $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" -saved_CFLAGS="$CFLAGS" -CFLAGS="$warn_CFLAGS $CFLAGS" -AC_MSG_CHECKING([if ${CC} supports ${warn_CFLAGS}]) -AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])], - [ AC_MSG_RESULT([yes]) ], - [ AC_MSG_RESULT([no]) - CFLAGS="$saved_CFLAGS" - ]) - -saved_CFLAGS="$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" - ]) +# 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 still touch the CFLAGS for --coverage and -O0/-O2. +# 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) + + 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. + SECP_TRY_APPEND_CFLAGS([-Wno-overlength-strings], $1) # GCC >= 4.2, -Woverlength-strings is implied by -pedantic. + SECP_TRY_APPEND_CFLAGS([-Wall], $1) # GCC >= 2.95 and probably many other compilers + SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall. + 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. + SECP_TRY_APPEND_CFLAGS([-Wcast-align], $1) # GCC >= 2.95 + SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only + SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0 + + CFLAGS="$SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS" +]) +SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS) ### ### Define config arguments @@ -360,8 +366,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" + SECP_CFLAGS_FOR_BUILD="$SECP_CFLAGS" CPPFLAGS_FOR_BUILD="$CPPFLAGS" + CFLAGS_FOR_BUILD="$CFLAGS" LDFLAGS_FOR_BUILD="$LDFLAGS" else AX_PROG_CC_FOR_BUILD @@ -378,15 +385,7 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then SAVE_LDFLAGS="$LDFLAGS" LDFLAGS="$LDFLAGS_FOR_BUILD" - warn_CFLAGS_FOR_BUILD="-Wall -Wextra -Wno-unused-function" - 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" - ]) + SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS_FOR_BUILD) AC_MSG_CHECKING([for working native compiler: ${CC_FOR_BUILD}]) AC_RUN_IFELSE( @@ -394,8 +393,6 @@ 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" @@ -419,6 +416,7 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then fi AC_SUBST(CC_FOR_BUILD) + AC_SUBST(SECP_CFLAGS_FOR_BUILD) AC_SUBST(CFLAGS_FOR_BUILD) AC_SUBST(CPPFLAGS_FOR_BUILD) AC_SUBST(LDFLAGS_FOR_BUILD) @@ -490,6 +488,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,12 +531,14 @@ fi echo echo " valgrind = $enable_valgrind" echo " CC = $CC" +echo " SECP_CFLAGS = $SECP_CFLAGS" echo " CFLAGS = $CFLAGS" echo " CPPFLAGS = $CPPFLAGS" echo " LDFLAGS = $LDFLAGS" echo if test x"$set_precomp" = x"yes"; then echo " CC_FOR_BUILD = $CC_FOR_BUILD" +echo " SECP_CFLAGS_FOR_BUILD = $SECP_CFLAGS_FOR_BUILD" echo " CFLAGS_FOR_BUILD = $CFLAGS_FOR_BUILD" echo " CPPFLAGS_FOR_BUILD = $CPPFLAGS_FOR_BUILD" echo " LDFLAGS_FOR_BUILD = $LDFLAGS_FOR_BUILD" From 595e8a35d80c932f91e810ce889c48b6efbaf890 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 13 May 2021 17:14:56 +0200 Subject: [PATCH 2/5] build: Enable -Wcast-align=strict warning --- configure.ac | 1 + 1 file changed, 1 insertion(+) diff --git a/configure.ac b/configure.ac index aaca5343063c3..9c3cb37ce9716 100644 --- a/configure.ac +++ b/configure.ac @@ -99,6 +99,7 @@ AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [ SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall. 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. SECP_TRY_APPEND_CFLAGS([-Wcast-align], $1) # GCC >= 2.95 + SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0 SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0 From 7939cd571c7a236f0d46e5cd7b6529ae29757c5a Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 13 May 2021 18:54:37 +0200 Subject: [PATCH 3/5] build: List *CPPFLAGS before *CFLAGS like on the compiler command line --- configure.ac | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/configure.ac b/configure.ac index 9c3cb37ce9716..0caa0cf5b0bb4 100644 --- a/configure.ac +++ b/configure.ac @@ -367,8 +367,8 @@ 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" - SECP_CFLAGS_FOR_BUILD="$SECP_CFLAGS" CPPFLAGS_FOR_BUILD="$CPPFLAGS" + SECP_CFLAGS_FOR_BUILD="$SECP_CFLAGS" CFLAGS_FOR_BUILD="$CFLAGS" LDFLAGS_FOR_BUILD="$LDFLAGS" else @@ -379,10 +379,10 @@ 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" @@ -397,14 +397,14 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then # 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 @@ -417,9 +417,9 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then fi AC_SUBST(CC_FOR_BUILD) + AC_SUBST(CPPFLAGS_FOR_BUILD) AC_SUBST(SECP_CFLAGS_FOR_BUILD) AC_SUBST(CFLAGS_FOR_BUILD) - AC_SUBST(CPPFLAGS_FOR_BUILD) AC_SUBST(LDFLAGS_FOR_BUILD) else set_precomp=no @@ -532,15 +532,15 @@ fi echo echo " valgrind = $enable_valgrind" echo " CC = $CC" +echo " CPPFLAGS = $CPPFLAGS" echo " SECP_CFLAGS = $SECP_CFLAGS" echo " CFLAGS = $CFLAGS" -echo " CPPFLAGS = $CPPFLAGS" echo " LDFLAGS = $LDFLAGS" echo if test x"$set_precomp" = x"yes"; then echo " CC_FOR_BUILD = $CC_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 " CPPFLAGS_FOR_BUILD = $CPPFLAGS_FOR_BUILD" echo " LDFLAGS_FOR_BUILD = $LDFLAGS_FOR_BUILD" fi From b924e1e605dcf9f9b362531184d16d643cc3baa9 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 13 May 2021 19:34:16 +0200 Subject: [PATCH 4/5] build: Ensure that configure's compile checks default to -O2 Fixes #896. --- .gitignore | 1 + configure.ac | 13 ++++++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index b62055a39bc77..79b740db8ae35 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,7 @@ aclocal.m4 autom4te.cache/ config.log config.status +conftest* *.tar.gz *.la libtool diff --git a/configure.ac b/configure.ac index 0caa0cf5b0bb4..3a68e85850596 100644 --- a/configure.ac +++ b/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. @@ -84,7 +80,6 @@ esac # 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 still touch the CFLAGS for --coverage and -O0/-O2. # 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 @@ -220,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 From 0302138f7508414e9e5212bc45b4ca4c0e5f081c Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 6 May 2021 14:02:00 +0200 Subject: [PATCH 5/5] ci: Make compiler warning into errors on CI This also tidies the list of environment variables in .cirrus.yml. --- .cirrus.yml | 21 ++++++++++++++------- configure.ac | 3 +++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 6d63511e6d3a9..aca8e0b5274de 100644 --- a/.cirrus.yml +++ b/.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 - CTIMETEST: yes - BENCH: yes + ### test options TEST_ITERS: + BENCH: yes BENCH_ITERS: 2 - MAKEFLAGS: -j2 + CTIMETEST: yes cat_logs_snippet: &CAT_LOGS always: diff --git a/configure.ac b/configure.ac index 3a68e85850596..7172a9f22a257 100644 --- a/configure.ac +++ b/configure.ac @@ -357,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