configure: Use CFLAGS_FOR_BUILD when checking native compiler #584

real-or-random commented at 5:01 PM on January 16, 2019: contributor

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.

real-or-random commented at 5:03 PM on January 16, 2019: contributor

@greenaddress Lawrence, does this fix your issue?

greenaddress commented at 7:01 PM on January 16, 2019: contributor

@real-or-random nice! seems to work for me but I've done very basic testing. thank you!

greenaddress commented at 11:23 PM on January 23, 2019: contributor

@real-or-random I did some more testing. I found it didn't work in some cross compiling scenarios when using platform specific LDFLAGS. For example for arm it is common to add to LDFLAGS "--fix-cortex-a8" which happens to break the configure check for working_native_cc. Your PR plus something like the diff below should fix it (does for me).


@@ -202,6 +202,8 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
   CFLAGS="$CFLAGS_FOR_BUILD"
   SAVE_CPPFLAGS="$CPPFLAGS"
   CPPFLAGS="$CPPFLAGS_FOR_BUILD"
+  SAVE_LDFLAGS="$LDFLAGS"
+  LDFLAGS="$LDFLAGS_FOR_BUILD"
 
   warn_CFLAGS_FOR_BUILD="-Wall -Wextra -Wno-unused-function"
   saved_CFLAGS="$CFLAGS"
@@ -222,6 +224,7 @@ if test x"$use_ecmult_static_precomputation" != x"no"; then
   CFLAGS_FOR_BUILD="$CFLAGS"
   CPPFLAGS="$SAVE_CPPFLAGS"
   CFLAGS="$SAVE_CFLAGS"
+  LDFLAGS="$SAVE_LDFLAGS"

real-or-random commented at 8:05 AM on January 24, 2019: contributor

Oh yep, we should handle all the flags. Thanks for figuring out! I'll add a commit later.

greenaddress cross-referenced this on Jan 25, 2019 from issue Ecmult precomputation fix by greenaddress

real-or-random force-pushed on Jan 26, 2019

real-or-random commented at 1:43 PM on January 26, 2019: contributor

Force pushed for the LDFLAGS issue

greenaddress commented at 12:26 PM on January 28, 2019: contributor

utack 88c051442742b04f82c79fc9f5a6dab21657d487

sipa commented at 10:35 PM on February 4, 2019: contributor

Ping @theuni, feel like reviewing this?

in configure.ac:195 in 88c0514427 outdated

 193 | +      ])
 194 | +
 195 | +  AC_MSG_CHECKING([for working native compiler: ${CC_FOR_BUILD}])
 196 |    AC_RUN_IFELSE(
 197 | -    [AC_LANG_PROGRAM([], [return 0])],
 198 | +    [AC_LANG_PROGRAM([], [])],

theuni commented at 5:45 PM on February 8, 2019:

IIRC The "return 0" silences a warning, which can have an effect with -Werror.

Maybe I have that backwards, what was the reason for this change?

real-or-random commented at 10:46 AM on February 10, 2019:

This change is rather cosmetic. We're using AC_LANG_PROGRAM here, which adds already return 0 according to https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/Generating-Sources.html. So previously we had two returns.

in configure.ac:181 in 88c0514427 outdated

 178 | +  SAVE_CFLAGS="$CFLAGS"
 179 | +  CFLAGS="$CFLAGS_FOR_BUILD"
 180 | +  SAVE_CPPFLAGS="$CPPFLAGS"
 181 | +  CPPFLAGS="$CPPFLAGS_FOR_BUILD"
 182 | +  SAVE_LDFLAGS="$LDFLAGS"
 183 | +  LDFLAGS="$LDFLAGS_FOR_BUILD"

theuni commented at 5:54 PM on February 8, 2019:

Not a problem with this PR, but this points out that LDFLAGS_FOR_BUILD is currently unused. And while we're fixing that, may as well add CFLAGS_FOR_BUILD to the link-line as well, as would be expected:

diff --git a/Makefile.am b/Makefile.am
index bfa3f46..4f89b78 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -115,7 +115,7 @@ gen_%.o: src/gen_%.c
        $(CC_FOR_BUILD) $(CPPFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) -c $< -o $@

 $(gen_context_BIN): $(gen_context_OBJECTS)
-       $(CC_FOR_BUILD) $^ -o $@
+       $(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@

 $(libsecp256k1_la_OBJECTS): $(SECP256K1_ECMULT_STATIC_CONTEXT_HEADER_INT)
 $(tests_OBJECTS): $(SECP256K1_ECMULT_STATIC_CONTEXT_HEADER_INT)

(I'm working on a heavily modified branch, that may be different in master but you get the idea.)

real-or-random commented at 10:47 AM on February 10, 2019:

Makes sense, I'll add that.

in configure.ac:211 in 88c0514427 outdated

 210 | +  LDFLAGS="$SAVE_LDFLAGS"
 211 |  
 212 |    if test x"$working_native_cc" = x"no"; then
 213 | +    AC_MSG_RESULT([no])
 214 |      set_precomp=no
 215 | +    m4_define([please_set_for_build], [Please set CC_FOR_BUILD and/or CFLAGS_FOR_BUILD, CPPFLAGS_FOR_BUILD, and LDFLAGS_FOR_BUILD.])

theuni commented at 6:01 PM on February 8, 2019:

Nit: CC_FOR_BUILD, CFLAGS_FOR_BUILD, CPPFLAGS_FOR_BUILD, and/or LDFLAGS_FOR_BUILD.

real-or-random commented at 10:49 AM on February 10, 2019:

Fixing.

theuni commented at 6:03 PM on February 8, 2019: contributor

Concept ACK and utACK with those little things addressed.

configure: Use CFLAGS_FOR_BUILD when checking native compiler

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), and
similar for CPPFLAGS and LDFLAGS.

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.

2d5f4cebdc

Actually pass CFLAGS_FOR_BUILD and LDFLAGS_FOR_BUILD to linker a34bcaadf1

real-or-random force-pushed on Feb 10, 2019

real-or-random commented at 10:59 AM on February 10, 2019: contributor

addressed @theuni's comments

gmaxwell commented at 7:11 PM on February 20, 2019: contributor

ACK

laanwj commented at 2:04 PM on February 21, 2019: member

utACK a34bcaadf14442b86a5de120d4afd131f910d73d

gmaxwell commented at 1:29 AM on February 22, 2019: contributor

ACK

gmaxwell merged this on Feb 22, 2019

gmaxwell closed this on Feb 22, 2019

gmaxwell referenced this in commit 5545e13dea on Feb 22, 2019

greenaddress referenced this in commit b16f3ec4ff on Apr 19, 2019

real-or-random cross-referenced this on May 24, 2019 from issue configure: Use CFLAGS_FOR_BUILD when checking native compiler by real-or-random