214042a170 build: don't append valgrind CPPFLAGS if not installed (fanquake)
Pull request description:
Valgrinds CPPFLAGS, i.e `-I/usr/local/opt/valgrind/include`, are currently added to CPPFLAGS, regardless of whether valgrind is installed. This changes configure so that they are only added if valgrind is available. i.e the output of `brew list --versions valgrind` is non-null.
ACKs for top commit:
real-or-random:
ACK 214042a170
hebasto:
ACK 214042a170, tested on macOS Big Sur 11.6.1 (20G224, Intel).
Tree-SHA512: 5101636a0a12f1941b01967ca8eab7aa20f44db0d1ef4571a5ad6026bb89494b983465d34d93c8b17a260b695116792991da53d135bc19a3c9e974f5266a90af
This header contains a static array that replaces the ecmult_context pre_g and pre_g_128 tables.
The gen_ecmult_static_pre_g program generates this header file.
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.
Bitcoin Core's `configure` script uses `AC_CHECK_PROG` to find brew in the `PATH` [1]. If found, this macro will set `BREW=brew`. When building with dependencies however the `BREW` variable is set to `no` on macOS via `depends/<host_prefix>/share/config.site` [2] and this overrides `AC_CHECK_PROG` results [3]. Ideally, secp256k1's `configure` script should follow the same logic but this is not what happens because secp256k1's `configure` uses `AC_PATH_PROG` instead which respects preset variable values (in this case for variable `BREW`) only if they are a valid path (i.e., they match `[\\/*] | ?:[\\/]*` [4]), and `no` is not a path.
This commit changes `AC_PATH_PROG` to `AC_CHECK_PROG` to be consistent with Core's `AC_CHECK_PROG`. Both of these macros are supposed to find executables in the `PATH` but the difference is that former is supposed to return the full path whereas the latter is supposed to find only the program. As a result, the latter will accept even non-paths `no` as an override. Not knowing the full path is not an issue for the `configure` script because it will only execute `BREW` immediately afterwards, which works fine without the full path. (In particular, `PATH` cannot have changed in between [5].)
[1] https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L684
[2] https://github.com/bitcoin/bitcoin/blob/master/depends/config.site.in#L73-L76
[3] 6d38e9fa2b/lib/autoconf/programs.m4 (L47)
[4] 6d38e9fa2b/lib/autoconf/programs.m4 (L127)
[5] [3ab1178](3ab1178d54)
This compiler flag is available for clang but not gcc.
Test plan:
```
autogen.sh
./configure
make check
CC=clang ./configure
make check
```
If a variable is used uninitialized, the warning should look something
like:
```
CC src/tests-tests.o
src/tests.c:4336:15: warning: variable 'recid' may be uninitialized when used here [-Wconditional-uninitialized]
CHECK(recid >= 0 && recid < 4);
^~~~~
./src/util.h:54:18: note: expanded from macro 'CHECK'
if (EXPECT(!(cond), 0)) { \
^~~~
./src/util.h:41:39: note: expanded from macro 'EXPECT'
^
src/tests.c:4327:14: note: initialize the variable 'recid' to silence this warning
int recid;
^
= 0
1 warning generated.
```
This commits simply uses CC as CC_FOR_BUILD and the same for
corresponding flags if we're not cross-compiling. This has a number of
benefits in this common case:
- It avoids strange cases where very old compilers are used (#768).
- Flags are consistently set for CC and CC_FOR_BUILD.
- ./configure is faster.
- You get compiler x consistently if you set CC=x; we got this wrong
in CI in the past.
./configure warns if a _FOR_BUILD variable is set but ignored because
we're not cross-compiling.
The change exposed that //-style comments are used in gen_context.c,
which is also fixed by this commit.
This commit also reorganizes code in configure.ac to have a cleaner
separation of sections.
Valgrind is typically installed using brew on macOS. This commit
makes ./configure detect this case set the appropriate include
directory (in the same way as we already do for openssl and gmp).
412bf874d0 configure: Allow specifying --with[out]-valgrind explicitly (Luke Dashjr)
Pull request description:
ACKs for top commit:
sipa:
ACK 412bf874d0. Tested by running configure on a system with and without valgrind, and with no argument, with `--with-valgrind`, and with `--without-valgrind`.
real-or-random:
ACK 412bf874d0
jonasnick:
ACK 412bf874d0
Tree-SHA512: 92417609751e5af813faff1661055cd37f3d00dbcf109a8f14f8ba59d9f3d620c9c6b67d2b1629b6ab75e2afcd47d2b3898a0427931567fb505bc92fa5ee3532
0dccf98a21 Use preprocessor macros instead of autoconf to detect endianness (Tim Ruffing)
Pull request description:
This does not fix any particular issue but it's preferable to not
rely on autoconf. This avoids endianness mess for users on BE hosts
if they use their build without autoconf.
The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.
Supersedes #770 .
ACKs for top commit:
sipa:
ACK 0dccf98a21
gmaxwell:
ACK 0dccf98a21
Tree-SHA512: 6779458de5cb6eaef2ac37f9d4b8fa6c9b299f58f6e5b72f2b0d7e36c12ea06074e483acfb85085a147e0f4b51cd67d897f61a67250ec1cea284a0f7680eb2e8
This does not fix any particular issue but it's preferable to not
rely on autoconf. This avoids endianness mess for users on BE hosts
if they use their build without autoconf.
The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.
Instead of supporting configuration of the field and scalar size independently,
both are now controlled by the availability of a 64x64->128 bit multiplication
(currently only through __int128). This is autodetected from the C code through
__SIZEOF_INT128__, but can be overridden using configure's
--with-test-override-wide-multiply, or by defining
USE_FORCE_WIDEMUL_{INT64,INT128} manually.
ca739cba23 Compile with optimization flag -O2 by default instead of -O3 (Jonas Nick)
83fb1bcef4 Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual) (Jonas Nick)
ecba8138ec Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables (Jonas Nick)
613c34cd86 Remove test in configure.ac because it doesn't have an effect (Jonas Nick)
Pull request description:
Right now, it's not easy to reduce the optimization level with `CFLAGS` because `configure` overwrites any optimization flag with `-O3`. The [automake documentation](https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html) states that:
> The reason ‘$(CPPFLAGS)’ appears after ‘$(AM_CPPFLAGS)’ or ‘$(mumble_CPPFLAGS)’ in the compile command is that users should always have the last say.
and also that it's incorrect to redefine CFLAGS in the first place
> You should never redefine a user variable such as CPPFLAGS in Makefile.am. [...] You should not add options to these user variables within configure either, for the same reason
With this PR `CFLAGS` is still redefined, but user-provided flags appear after the default `CFLAGS` which means that they override the default flags (at least in clang and gcc). Otherwise, the default configuration is not changed. This also means that if CFLAGS are defined by the user, then -g is not added (which does not seem to make much sense). In order to keep the `-O3` despite the reordering we need to explicitly tell autoconf to not append `-O2` by setting the default to `-g` with `: ${CFLAGS="-g"}` as per [the manual](https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#C-Compiler) (EDIT: link fix).
ACKs for top commit:
real-or-random:
ACK ca739cba23
theuni:
ACK ca739cba23.
elichai:
ACK ca739cba23
Tree-SHA512: be92589faa461d245203385d44b489c7d6917b0c68472b8d7576806c0250cf5ff61d5c99ce04eebb8ff5279b9987185d4e5d2da979683fb1c489fdf3e5b59630
Valgrind does bit-level tracking of the "uninitialized" status of memory,
property tracks memory which is tainted by any uninitialized memory, and
warns if any branch or array access depends on an uninitialized bit.
That is exactly the verification we need on secret data to test for
constant-time behaviour. All we need to do is tell valgrind our
secret key is actually uninitialized memory.
This adds a valgrind_ctime_test which is compiled if valgrind is installed:
Run it with libtool --mode=execute:
$ libtool --mode=execute valgrind ./valgrind_ctime_test
ECDSA signing has a retry loop for the exceptionally unlikely case
that S==0. S is not a secret at this point and this case is so
rare that it will never be observed but branching on it will trip
up tools analysing if the code is constant time with respect to
secrets.
Derandomized ECDSA can also loop on k being zero or overflowing,
and while k is a secret these cases are too rare (1:2^255) to
ever observe and are also of no concern.
This adds a function for marking memory as no-longer-secret and
sets it up for use with the valgrind memcheck constant-time
test.
make ECMULT_GEN_PREC_BITS configurable
ecmult_static_context.h: add compile time config assertion (#3) - Prevents accidentally using a file which was generated with a
different configuration.
README: mention valgrind issue
With --with-ecmult-gen-precision=8, valgrind needs a max stack size
adjustment to not run into a stack switching heuristic:
http://valgrind.org/docs/manual/manual-core.html
> -max-stackframe= [default: 2000000]
> The maximum size of a stack frame. If the stack pointer moves by more than this amount then Valgrind will assume that the program is switching to a different stack.
You may need to use this option if your program has large stack-allocated arrays.
basic-config: undef ECMULT_WINDOW_SIZE before (re-)defining it
This makes WINDOW_G a configurable value in the range of [2..24].
The upper limit of 24 is a defensive choice. The code is probably
correct for values up to 27 but those larger values yield in huge
tables (>= 256MiB), which are i) unlikely to be really beneficial
in practice and ii) increasingly difficult to test.
a34bcaa Actually pass CFLAGS_FOR_BUILD and LDFLAGS_FOR_BUILD to linker (Tim Ruffing)
2d5f4ce configure: Use CFLAGS_FOR_BUILD when checking native compiler (Tim Ruffing)
Pull request description:
This fixes a bug where configure would fail or disable static
ecmult tables because it wrongly checks the native compiler using
the target CFLAGS (instead of the native CFLAGS_FOR_BUILD).
Moreover, this commit adds tests to figure out whether the native
compiler supports the warning flags passed during the build, and it
contains a few minor improvements to the code that checks the native
compiler.
Tree-SHA512: 31a92a5516cf2f9801c918edfba0059aa4f8549b0c1de94fc166b5e92ad1868a480c48cdc5ff62679ba20e26f4a0e2948c71fd2b3e80766673d2bf7512da3875
3965027 Summarize build options in configure script (Evan Klitzke)
Pull request description:
This is a trivial build system change to summarize the build options after running configure.
Example output:
```
$ ./configure
....
<many lines omitted>
...
config.status: src/libsecp256k1-config.h is unchanged
config.status: executing depfiles commands
config.status: executing libtool commands
Build Options:
with endomorphism = no
with ecmult precomp = yes
with jni = no
module ecdh = no
module recovery = no
asm = x86_64
bignum = gmp
field = 64bit
scalar = 64bit
CC = gcc
CFLAGS = -g -O2 -W -std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wno-unused-function -Wno-long-long -Wno-overlength-strings -fvisibility=hidden -O3
CPPFLAGS =
LDFLAGS =
```
I tried to just include the configure options that looked interesting; let me know if there are any I didn't include that I should have.
Tree-SHA512: 428381654d772f76bc81210d39ba5c3f07a94dc6a6378a02ccc6f23ebce7f501896268bcd2e94e2b0d8aea54c9c70c44a9238a0f0960600f463b1e2847c7ed1f
270f6c8 Portability fix for the configure scripts generated (Pierre Pronchery)
Pull request description:
Found thanks to the developer checks from the pkgsrc software
distribution (for NetBSD, SmartOS, Minix, MacOS X, Linux, and more).
Tree-SHA512: 2589545aa4d0620db66e79df1dc148a487384b5169ba7323937490d802973388859d30d45b35ee3e614be6d49cb694f37f585a16caa87ad1e500a0b7368dcc0a
Tim Ruffing
fanquake
Dimitris Apostolou
Pieter Wuille
Russell O'Connor
Jonas Nick
UdjinM6
PiRK
Elichai Turkel
Luke Dashjr
Gregory Maxwell
djb
DesWurstes