Various improvements related to CFLAGS

real-or-random commented at 7:37 pm on May 13, 2021: contributor

Fixes strange warnings in mingw builds (one item in #923).
Enables more and more fine-grained compiler warnings.
Fixes minor build issue on macOS #896 on macOS.
Makes compiler warnings into errors on CI (-Werror, mentioned in #929 (comment)).

real-or-random cross-referenced this on May 13, 2021 from issue ci: Set -Werror for all CI builds by real-or-random

real-or-random commented at 7:40 pm on May 13, 2021: contributor

CI seems to be confused here because this is the same source branch as on #938 . Trying to close and reopen the PR to fix it.

edit: ok, didn’t work. I forced push the last commit with a new date to make it work.

real-or-random closed this on May 13, 2021

real-or-random reopened this on May 13, 2021

real-or-random force-pushed on May 13, 2021

real-or-random force-pushed on May 25, 2021

real-or-random commented at 12:48 pm on May 25, 2021: contributor

rebased

roconnor-blockstream commented at 9:30 pm on May 28, 2021: contributor

LGTM. I did test building in under Nixos, but that was the extent of my testing.

in build-aux/m4/bitcoin_secp.m4:86 in 64ec678daa outdated

81@@ -82,3 +82,19 @@ if test x"$has_valgrind" != x"yes"; then
82   AC_CHECK_HEADER([valgrind/memcheck.h], [has_valgrind=yes; AC_DEFINE(HAVE_VALGRIND,1,[Define this symbol if valgrind is installed])])
83 fi
84 ])
85+
86+dnl SECP_TRY_APPEND_CFLAGS([flags], VAR)

hebasto commented at 2:10 pm on June 2, 2021:

From my understanding, the square brackets [...] in a comment, that describes a new macro, means that parameter is optional. They do not mean quoting. Right?

real-or-random commented at 3:20 pm on June 10, 2021:

fixed

in configure.ac:73 in 64ec678daa outdated

65@@ -70,35 +66,40 @@ case $host_os in
66    ;;
67 esac
68 
69-CFLAGS="-W $CFLAGS"

hebasto commented at 2:11 pm on June 2, 2021:

Maybe document the removing of -W option?

real-or-random commented at 10:17 am on June 7, 2021:

Apparently I’ve documented it here already https://github.com/bitcoin-core/secp256k1/pull/944/files#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810R95

hebasto commented at 11:57 am on June 7, 2021:

Ah, right.

hebasto commented at 2:21 pm on June 2, 2021: member

Approach ACK 64ec678daaf215e007d44433cdc9b969b6e09098, tested on:

Linux Mint 20.1 (x86_64)
macOS 11.4 (20F71)

Warnings introduced in #700 (reported in #896) are gone.

The last two commits seems the one to me, maybe squash them? But maybe another approach could be considered?

Why a CI is not a “user” from the Automake’s point of view? If so, why to introduce the WERROR_CFLAGS variable, while we have a dedicated CFLAGS one?

real-or-random commented at 3:06 pm on June 2, 2021: contributor

Why a CI is not a “user” from the Automake’s point of view? If so, why to introduce the WERROR_CFLAGS variable, while we have a dedicated CFLAGS one?

It may seem overly complicated but I think it’s better for testing/CI because it does not interfere with autoconf’s default for CFLAGS (which is -g -O2 or just -g depending on the compiler). What we typically want to test in CI is building with unset CFLAGS. We could always set -Werror -g -O2 instead of -Werror but that seems complicated too.

By the way, I think I overlooked this line: https://github.com/real-or-random/secp256k1/blob/202105-ci-werror/configure.ac#L397 Will address the other comments.

hebasto commented at 3:21 pm on June 2, 2021: member

Why a CI is not a “user” from the Automake’s point of view? If so, why to introduce the WERROR_CFLAGS variable, while we have a dedicated CFLAGS one?

It may seem overly complicated but I think it’s better for testing/CI because it does not interfere with autoconf’s default for CFLAGS (which is -g -O2 or just -g depending on the compiler). What we typically want to test in CI is building with unset CFLAGS. We could always set -Werror -g -O2 instead of -Werror but that seems complicated too.

Ah, indeed. This repo does not have the --enable-werror configure option as Bitcoin Core has :)

real-or-random commented at 3:39 pm on June 2, 2021: contributor

Oh interesting, maybe I should just have looked at Core then… ;)

For context: #938 (comment)

Anyway, this thing here works and I don’t want to touch it further unless necessary.

real-or-random force-pushed on Jun 10, 2021

real-or-random commented at 3:20 pm on June 10, 2021: contributor

Addressed the comments. I also rebased on master (sorry, that wasn’t even intentional…)

The last two commits seems the one to me, maybe squash them?

Done.

hebasto commented at 8:46 pm on June 10, 2021: member

ACK a5c6ff2917edf97e888c441eb545ac70083b5376

in .cirrus.yml:6 in a5c6ff2917 outdated

0@@ -1,21 +1,28 @@
1 env:
2-  WIDEMUL: auto
3+  ### compiler options
4+  HOST:
5+  # Specific warnings can be disabled with -Wno-error=foo.
6+  # -pedantic-errors is not equivalent to -Werror=pedantic and thus not implied by -Werror according to the GCC manual.
7+  WERROR_CFLAGS: -Werror -pedantic-errors

jonasnick commented at 10:48 am on June 30, 2021:

nit: trailing whitespace

in configure.ac:90 in a5c6ff2917 outdated

114+    # Try to append -Werror=unknown-warning-option to CFLAGS temporarily. Otherwise clang will
115+    # not error out if it gets unknown warning flags and the checks here will always succeed
116+    # no matter if clang knows the flag or not.
117+    SECP_TRY_APPEND_DEFAULT_CFLAGS_saved_CFLAGS="$CFLAGS"
118+    SECP_TRY_APPEND_CFLAGS([-Werror=unknown-warning-option], CFLAGS)
119+

jonasnick commented at 10:48 am on June 30, 2021:

nit: trailing whitespace

in configure.ac:99 in a5c6ff2917 outdated

123+    SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall.
124+    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.
125+    SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0
126+    SECP_TRY_APPEND_CFLAGS([-Wconditional-uninitialized], $1) # Clang >= 3.0 only
127+    SECP_TRY_APPEND_CFLAGS([-fvisibility=hidden], $1) # GCC >= 4.0
128+

jonasnick commented at 10:49 am on June 30, 2021:

nit: trailing whitespace

in configure.ac:97 in a5c6ff2917 outdated

120+    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.
121+    SECP_TRY_APPEND_CFLAGS([-Wno-overlength-strings], $1) # GCC >= 4.2, -Woverlength-strings is implied by -pedantic.
122+    SECP_TRY_APPEND_CFLAGS([-Wall], $1) # GCC >= 2.95 and probably many other compilers
123+    SECP_TRY_APPEND_CFLAGS([-Wno-unused-function], $1) # GCC >= 3.0, -Wunused-function is implied by -Wall.
124+    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.
125+    SECP_TRY_APPEND_CFLAGS([-Wcast-align=strict], $1) # GCC >= 8.0

jonasnick commented at 3:03 pm on June 30, 2021:

clang doesn’t have -Wcast-align=strict but only -Wcast-align. Since this PR replaces the latter with the former, when using clang there are no alignment warnings anymore.

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.

07256267ff

build: Enable -Wcast-align=strict warning 595e8a35d8

build: List *CPPFLAGS before *CFLAGS like on the compiler command line 7939cd571c

build: Ensure that configure's compile checks default to -O2

Fixes #896.

b924e1e605

real-or-random force-pushed on Jul 1, 2021

ci: Make compiler warning into errors on CI

This also tidies the list of environment variables in .cirrus.yml.

0302138f75

real-or-random force-pushed on Jul 1, 2021

real-or-random commented at 6:38 pm on July 1, 2021: contributor

fixed

jonasnick commented at 7:43 pm on July 1, 2021: contributor

ACK 0302138f7508414e9e5212bc45b4ca4c0e5f081c

jonasnick merged this on Jul 1, 2021

jonasnick closed this on Jul 1, 2021

sipa referenced this in commit c020cbaa5c on Jul 14, 2021

apoelstra cross-referenced this on Jul 27, 2021 from issue Upstream PRs 879, 959, 955, 944, 951, 960, 844, 963, 965 by apoelstra

apoelstra referenced this in commit 196c993d1f on Jul 28, 2021

janus referenced this in commit 62075fc9aa on Nov 5, 2021

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

hebasto cross-referenced this on Mar 14, 2023 from issue build: Ensure no optimization when building for coverage analysis by hebasto

real-or-random referenced this in commit 0cf2fb91ef on Mar 21, 2023

str4d referenced this in commit 7648f6bf55 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

Various improvements related to CFLAGS #944