Allow overriding default flags

jonasnick commented at 12:54 pm on December 17, 2019: contributor

Right now, it’s not easy to reduce the optimization level with CFLAGS because configure overwrites any optimization flag with -O3. The automake documentation 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 (EDIT: link fix).

real-or-random commented at 1:10 pm on December 17, 2019: contributor

Approach ACK

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.

I think the relevant section is https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#C-Compiler , macro AC_PROG_CC

gmaxwell commented at 11:55 am on December 18, 2019: contributor

This has irritated me a number of times. It’s conventional to prepend rather than append.

in configure.ac:11 in bfc8533e12 outdated

 6@@ -7,6 +7,9 @@ AH_TOP([#ifndef LIBSECP256K1_CONFIG_H])
 7 AH_TOP([#define LIBSECP256K1_CONFIG_H])
 8 AH_BOTTOM([#endif /*LIBSECP256K1_CONFIG_H*/])
 9 AM_INIT_AUTOMAKE([foreign subdir-objects])
10+
11+# Set -g if CFLAGS are not already set

real-or-random commented at 2:15 pm on December 28, 2019:

0# Set -g but not -O2 if CFLAGS are not already set (see AC_PROG_CC in the Autoconf manual)

This is obscure enough to deserve a comment.

ACK otherwise

jonasnick commented at 12:37 pm on January 5, 2020:

fixed

jonasnick commented at 3:15 pm on January 5, 2020:

Eh, anything starting with AC_ in an error if it occurs in a comment:

0configure:3073: error: possibly undefined macro: AC_PROG_CC
1      If this token and others are legitimate, please use m4_pattern_allow.
2      See the Autoconf documentation.

Now fixed for real

sipa commented at 2:47 pm on December 29, 2019: contributor

Concept ACK, I don’t think the current behavior is intentional at all. @theuni want to have a look?

jonasnick force-pushed on Jan 5, 2020

Remove test in configure.ac because it doesn't have an effect 613c34cd86

Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables ecba8138ec

Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual) 83fb1bcef4

real-or-random commented at 11:22 am on January 6, 2020: contributor

ACK 83fb1bcef49b1c12ef349f62d90bfcc83f0f7398

sipa commented at 12:35 pm on January 6, 2020: contributor

@theuni Would you mind having a look?

theuni commented at 0:48 am on February 11, 2020: contributor

Does this end up changing Core’s build to -O2, since the -O3 is no longer forced on? I believe our CFLAGS specify -O2, at least for release builds.

Edit: It’s fine if that’s the case, but presumably we’d want Core to explicitly pass -O3 in order to get the current behavior.

real-or-random commented at 1:53 am on February 11, 2020: contributor

Does this end up changing Core’s build to -O2, since the -O3 is no longer forced on? I believe our CFLAGS specify -O2, at least for release builds.

Edit: It’s fine if that’s the case, but presumably we’d want Core to explicitly pass -O3 in order to get the current behavior.

Yes, e.g., in https://github.com/bitcoin/bitcoin/blob/452bb90c718da18a79bfad50ff9b7d1c8f1b4aa3/depends/hosts/mingw32.mk, also in the gitian builds.

We should probably first fix this in Core then. Anyone feel competent to do that change?

gmaxwell commented at 5:26 am on February 11, 2020: contributor

It might be better to spend time making code quality improvements to narrow the difference between O3 and O2 and then just be O2 by default.

theuni commented at 2:28 am on February 12, 2020: contributor

I could come up with something for Core, but I agree with @gmaxwell as well.

Are the performance differences between -O2 and -O3 generally understood for libsecp256k1? I realize that’s a pretty broad question as it spans compilers…

If not, I should think that the “-O3” performance should be achievable with “-O2 -ffoo -fbar” as well, and I could do some quick benches to see which specific flags account for the bulk of the differences. Hopefully that would point to some obvious manual optimizations.

gmaxwell commented at 9:20 am on February 12, 2020: contributor

At least some of it is due to autovectorization.

real-or-random commented at 11:47 am on February 12, 2020: contributor

I did some quick and dirty benchmarks and I couldn’t find a significant difference between -O2 and -O3 on my machine. I tried multiple options (with and without asm, with and without gmp). But this feels too good to be true, maybe something is wrong with my setup. (No, I didn’t forget to checkout this PR here :)).

If there is no difference (we care about) then we should simply switch to -O2. Otherwise I’m not sure. In a world with infinite developer time, it’s for sure to make manual improvements. But I’m somewhat concerned in the real world this will postpone this PR (which fixes something like a real bug) forever.

sipa commented at 4:12 pm on February 12, 2020: contributor

This wouldn’t surprise me too much. GCC got a lot better since the code was originally written and we decided to pick -O3. It would be useful to not just test x86_64 though.

jonasnick commented at 1:43 pm on February 18, 2020: contributor

Did a couple of experiments summarized in the following table. There are statistically significant but mostly small differences.

0| arch (*) | compiler (**) | gmp | asm | result (***)                                |
1|----------+---------------+-----+-----+---------------------------------------------|
2| x86_64   | gcc           | yes | yes | sign: +0.1us (0.2%), verify: +0.4us (0.6%)  |
3| x86_64   | gcc           | no  | yes | sign: +0.1us (0.2%), verify: -0.5us (-0.7%) |
4| x86_64   | gcc           | no  | no  | sign: +0.3us (0.7%), verify: -3.2us (-4.7%) |
5| x86_64   | clang         | yes | yes | sign: +0.0us (0.0%), verify: +0.1us (0.1%)  |
6| aarch64  | gcc           | yes | no  | sign: +2.0us (0.7%), verify: +5.0us (1.0%)  |
7| aarch64  | gcc           | no  | no  | sign: +3.0us (1.1%), verify: -9.0us (-1.7%) |
8| aarch64  | clang         | yes | no  | sign: -2.0us (0.6%), verify: +4.0us (0.7%)  |

(*) x86_64: Intel(R) Core(TM) i7-7820HQ CPU max 3.9GHz, aarch64: Cortex A53 max 1.5GHz (**) gcc 8.3.0, clang 9.0.1 (***) Result is bench_sign and bench_verify --O3 optimization level subtracted from --O2 (so positive means --O3 is faster). benchmark iteration count was increased from 10 to 100.

real-or-random commented at 2:23 pm on February 18, 2020: contributor

Nice! I think that rather speaks for -O2 then.

Did you also look at the binary sizes, too? I guess -O2 is smaller there.

jonasnick commented at 3:02 pm on February 18, 2020: contributor

On aarch64 without bignum libsecp256k1.a is 159K (--O3) vs. 154K (--O2).

theuni commented at 3:17 pm on February 18, 2020: contributor

+1 for just dropping the -O3 stuff, then.

theuni commented at 3:22 pm on February 18, 2020: contributor

ACK the approach and changes here, btw, excluding the optimization issue. Thanks for fixing this.

real-or-random commented at 3:52 pm on February 18, 2020: contributor

on x86_64, gcc (no static precomp): O2: 88698 bytes O3: 94722 bytes

Yes, I think we should make -O2 the default. People can still override this if they want -O3.

Compile with optimization flag -O2 by default instead of -O3 ca739cba23

in configure.ac:14 in ca739cba23

 6@@ -7,6 +7,11 @@ AH_TOP([#ifndef LIBSECP256K1_CONFIG_H])
 7 AH_TOP([#define LIBSECP256K1_CONFIG_H])
 8 AH_BOTTOM([#endif /*LIBSECP256K1_CONFIG_H*/])
 9 AM_INIT_AUTOMAKE([foreign subdir-objects])
10+
11+# Set -g if CFLAGS are not already set, which matches the default autoconf
12+# behavior (see PROG_CC in the Autoconf manual) with the exception that we don't
13+# set -O2 here because we set it in any case (see further down).
14+: ${CFLAGS="-g"}

real-or-random commented at 4:57 pm on February 19, 2020:

Couldn’t we get rid of this entirely then because Autoconf’s default is -g -O2?

If your answer is, maybe keep this variant still as a branch somewhere until we have further Concept ACKs on the idea of switching to -O2 :p

jonasnick commented at 5:04 pm on February 19, 2020:

If we remove this then if $CFLAGS aren’t already set they will be set to -O2 ... -O2. The comment additionally serves as a hint to how autoconf works :) because I’d rather set : ${CFLAGS=""} in the future, because anything else doesn’t make much sense to me.

real-or-random commented at 5:10 pm on February 19, 2020:

Okay yes. Also, autoconf sets -O2 apparently only for gcc and not for other compilers according to its manual… So we need to keep the -O2 further below anyway.

gmaxwell commented at 9:07 am on February 20, 2020: contributor

I tried multiple options (with and without asm, with and without gmp). But this feels too good to be true,

This is really surprising, previously with ASM disabled O3 got almost as much speedup as enabling asm did. Are you now seeing ASM not making a big performance difference anymore at O2? Is it possible that you’re somehow ending up with cached results?

[To be clear, I think moving off O3 is probably a good idea, but it should be done with an understanding of the performance impact in mind… and if it were large, perhaps some bisection to determine what functions had the biggest impact so they could get optimization effort]

real-or-random commented at 3:27 pm on February 20, 2020: contributor

much speedup as enabling asm did. Are you now seeing ASM not making a big performance dif @jonasnick’s table seems to imply that (on x86_64) O2 is even faster with asm and gmp turned off.

real-or-random commented at 1:29 pm on February 24, 2020: contributor

@gmaxwell Are you willing to quickly verify our results? I think this is essentially ready to merge if we’re confident about the benchmarks.

theuni commented at 6:52 pm on February 24, 2020: contributor

I’m a bit confused by some of those numbers. I’ll run some benches today as well.

theuni commented at 10:55 pm on February 24, 2020: contributor

I pushed a quick hack branch here to help with benching: https://github.com/theuni/secp256k1/commits/run_benches

It switches to clock_gettime to rule out wall-clock/drift issues, bumps the bench iteration to 100 to match @jonasnick’s tests, and adds a quick bench-runner script.

Locally I’ve disabled my cpu’s turbo boost and set all governors to “performance”. Additionally, the bench runner sets the cpu affinity to match on all tests.

It tests a matrix of gcc/clang versions, configure switches, and optimization flags.

I’ll post the results tonight or tomorrow when it’s done.

theuni commented at 5:35 am on February 25, 2020: contributor

I’ll sort/analyze tomorrow if nobody beats me to it. Will repeat on arm as well. One quick spoiler though. The winner on my desktop:

0gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O2

AMD Ryzen Threadripper 2970WX. Performance governor puts it at 3ghz. Cleaned up output:

  0Using global config: --enable-ecmult-static-precomputation --with-bignum=no --disable-openssl-tests
  1
  2gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
  3ecdsa_verify: min 118604ns / avg 118639ns / max 118686ns
  4
  5gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
  6ecdsa_verify: min 108535ns / avg 108591ns / max 108662ns
  7
  8gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
  9ecdsa_verify: min 108905ns / avg 108936ns / max 108979ns
 10
 11gcc-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
 12ecdsa_verify: min 108969ns / avg 109005ns / max 110088ns
 13
 14gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
 15ecdsa_verify: min 126077ns / avg 126628ns / max 126854ns
 16
 17gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
 18ecdsa_verify: min 113199ns / avg 113212ns / max 113235ns
 19
 20gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
 21ecdsa_verify: min 113855ns / avg 113996ns / max 120455ns
 22
 23gcc-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
 24ecdsa_verify: min 113168ns / avg 113185ns / max 113207ns
 25
 26gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
 27ecdsa_verify: min 118461ns / avg 118571ns / max 118642ns
 28
 29gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
 30ecdsa_verify: min 106635ns / avg 106666ns / max 106704ns
 31
 32gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
 33ecdsa_verify: min 107805ns / avg 107830ns / max 108036ns
 34
 35gcc-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
 36ecdsa_verify: min 107810ns / avg 108053ns / max 112453ns
 37
 38gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
 39ecdsa_verify: min 125447ns / avg 125699ns / max 132602ns
 40
 41gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
 42ecdsa_verify: min 105002ns / avg 105066ns / max 110299ns
 43
 44gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
 45ecdsa_verify: min 112369ns / avg 112528ns / max 119829ns
 46
 47gcc-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
 48ecdsa_verify: min 112420ns / avg 112430ns / max 112449ns
 49
 50clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
 51ecdsa_verify: min 108773ns / avg 108919ns / max 109015ns
 52
 53clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
 54ecdsa_verify: min 108381ns / avg 108404ns / max 108429ns
 55
 56clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
 57ecdsa_verify: min 108207ns / avg 108225ns / max 108284ns
 58
 59clang-6.0 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
 60ecdsa_verify: min 108276ns / avg 108293ns / max 108386ns
 61
 62clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
 63ecdsa_verify: min 119659ns / avg 119890ns / max 123911ns
 64
 65clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
 66ecdsa_verify: min 117406ns / avg 117645ns / max 127949ns
 67
 68clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
 69ecdsa_verify: min 115289ns / avg 115734ns / max 116006ns
 70
 71clang-6.0 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
 72ecdsa_verify: min 115479ns / avg 115696ns / max 115969ns
 73
 74clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
 75ecdsa_verify: min 109078ns / avg 109222ns / max 109610ns
 76
 77clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
 78ecdsa_verify: min 108055ns / avg 108095ns / max 108376ns
 79
 80clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
 81ecdsa_verify: min 108176ns / avg 108217ns / max 108264ns
 82
 83clang-7 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
 84ecdsa_verify: min 108207ns / avg 108222ns / max 108238ns
 85
 86clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
 87ecdsa_verify: min 112558ns / avg 112778ns / max 123525ns
 88
 89clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
 90ecdsa_verify: min 109983ns / avg 110230ns / max 119333ns
 91
 92clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
 93ecdsa_verify: min 108829ns / avg 108928ns / max 108972ns
 94
 95clang-7 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
 96ecdsa_verify: min 110449ns / avg 111127ns / max 124666ns
 97
 98clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
 99ecdsa_verify: min 108850ns / avg 109174ns / max 111568ns
100
101clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
102ecdsa_verify: min 108154ns / avg 108197ns / max 108234ns
103
104clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
105ecdsa_verify: min 108208ns / avg 108236ns / max 108266ns
106
107clang-8 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
108ecdsa_verify: min 108177ns / avg 108198ns / max 108280ns
109
110clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
111ecdsa_verify: min 112673ns / avg 112833ns / max 119882ns
112
113clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
114ecdsa_verify: min 111448ns / avg 114072ns / max 128824ns
115
116clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
117ecdsa_verify: min 108672ns / avg 108699ns / max 108768ns
118
119clang-8 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
120ecdsa_verify: min 108354ns / avg 108391ns / max 108509ns
121
122clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Os
123ecdsa_verify: min 108979ns / avg 109332ns / max 111432ns
124
125clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O2
126ecdsa_verify: min 108060ns / avg 108094ns / max 108400ns
127
128clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -O3
129ecdsa_verify: min 108029ns / avg 108044ns / max 108074ns
130
131clang-9 --with-asm=x86_64 --with-scalar=64bit --with-field=64bit -Ofast
132ecdsa_verify: min 108175ns / avg 108212ns / max 108249ns
133
134clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -Os
135ecdsa_verify: min 110937ns / avg 111384ns / max 111919ns
136
137clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -O2
138ecdsa_verify: min 108739ns / avg 108756ns / max 108830ns
139
140clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -O3
141ecdsa_verify: min 107166ns / avg 107202ns / max 107242ns
142
143clang-9 --with-asm=no --with-scalar=64bit --with-field=64bit -Ofast
144ecdsa_verify: min 107047ns / avg 107064ns / max 107116ns

gmaxwell commented at 7:38 am on February 25, 2020: contributor

Can you take two interesting configs, like -O3 with asm, and O2 no asm, and step back through a number of historical versions to see if changes to the library (or the benchmark) changed the results? (I’m kinda wondering if the benchmarks didn’t get messed up.)

Old chatlogs suggested that there were enormous differences that justified O3 in the first place. The PR that added scalar asm claims it was at the time a 12% speedup for signing, etc.

real-or-random commented at 10:10 am on February 25, 2020: contributor

@gmaxwell I tried to do this and I couldn’t find a revision where O3asm is really better for verification than O2. Maybe the difference is more significant for signing. I checked signing only later.

Here’s a way too long but helpful command that’s pretty robust across revisions. But you should make sure to grep the Makefile for CFLAGS to make sure it’s really doing the right thing. sed -i 's/-O3//g' configure.ac && ./autogen.sh && ./configure --enable-benchmark --with-bignum=no --with-asm=no --disable-ecmult-static-precomputation --disable-tests --disable-exhaustive-tests CFLAGS="-O2" && make clean && make && mv -f bench_verify bench_verify_O2 && mv -f bench_sign bench_sign_O2 && ./configure --enable-benchmark --with-bignum=no --with-asm=x86_64 --disable-ecmult-static-precomputation --disable-tests --disable-exhaustive-tests CFLAGS="-O3" && make clean && make && mv -f bench_verify bench_verify_O3asm && mv -f bench_sign bench_sign_O3asm && ./bench_verify_O2 && ./bench_verify_O3asm && ./bench_sign_O2 && ./bench_sign_O3asm ; git checkout -- configure.ac

Your example of #161 (asm for scalar): Before #161:

0O2    min 58.544us / avg 59.346us / max 61.378us
1O3    min 57.509us / avg 58.296us / max 59.643us

After #161:

0O2    min 58.603us / avg 59.665us / max 61.372us
1O3asm min 55.958us / avg 56.943us / max 58.980us

So the speedup introduced by this PR is only ~2% on my machine with gcc 9. Back then people used gcc 4, so maybe compilers really got that much better for our code.

theuni commented at 3:15 pm on February 25, 2020: contributor

For posterity, I used gcc’s “-Q –help=optimizers” options to find the actual difference between O2 and O3, so we can speak in terms of functionality rather than black boxes :)

For gcc 8 on x86_64, this should be equivalent to using -O3:

0./configure CFLAGS="-O2 -fgcse-after-reload -finline-functions -fipa-cp-clone -floop-interchange -floop-unroll-and-jam -fpeel-loops -fpredictive-commoning -fsplit-loops -fsplit-paths -ftree-loop-distribute-patterns -ftree-loop-distribution -ftree-loop-vectorize -ftree-partial-pre -ftree-slp-vectorize -funswitch-loops -fvect-cost-model=dynamic"

I think we’ve beaten the topic to death here, but someone down the road might be interested in looking at turning on/off optimizations with more precision. It’s interesting to think that could be a better use of time than just writing optimized asm.

sipa commented at 3:50 pm on February 25, 2020: contributor

The main thing that the hand-rolled assembly helps with is register allocation. When it was written, the compiler-emitted code made some silly decisions to spill intermittently (but often) used values to the stack and load them every time.

It’s possible that GCC (and other compilers) have just improved enough to the point that the benefits of handrolled asm is much smaller.

real-or-random commented at 4:01 pm on February 25, 2020: contributor

Even if we don’t know the reasons exactly, the benchmarks show that switching to O2 is a reasonable thing to do, so we should do this.

ACK ca739cba238cdd7c513abfc719b0b0eb957c9458

theuni commented at 11:29 pm on February 25, 2020: contributor

ACK ca739cba238cdd7c513abfc719b0b0eb957c9458.

I’ll follow-up with a PR to move away from modifying CFLAGS itself. @jonasnick is exactly right, that’s nasty.

elichai commented at 8:47 am on March 5, 2020: contributor

Just realized that until this is merged the last test in the matrix with EXTRAFLAGS=CFLAGS=-O0 is useless, because it still adds -O3 at the end. https://travis-ci.org/elichai/secp256k1/jobs/658344952#L388

elichai commented at 8:48 am on March 5, 2020: contributor

ACK ca739cba238cdd7c513abfc719b0b0eb957c9458

real-or-random merged this on Mar 20, 2020

real-or-random closed this on Mar 20, 2020

real-or-random cross-referenced this on Mar 27, 2020 from issue We shouldn't modify CFLAGS by real-or-random

greenaddress cross-referenced this on Mar 30, 2020 from issue Issue 178 - don't override CFLAGS, default to O2 by greenaddress

real-or-random cross-referenced this on Apr 26, 2020 from issue Compile time SHA256 override? by gmaxwell

real-or-random cross-referenced this on May 29, 2020 from issue Consider removing x86_64 field assembly by jonasnick

deadalnix referenced this in commit e1a3a76d28 on Jun 2, 2020

deadalnix referenced this in commit 091e926715 on Jun 3, 2020

sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa

fanquake referenced this in commit 8c97780db8 on Jun 13, 2020

sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020

ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020

ftrader referenced this in commit c41e8ad213 on Aug 17, 2020

hebasto cross-referenced this on Feb 17, 2021 from issue build: configure script warnings on macOS by hebasto

in configure.ac:13 in 83fb1bcef4 outdated

 6@@ -7,6 +7,10 @@ AH_TOP([#ifndef LIBSECP256K1_CONFIG_H])
 7 AH_TOP([#define LIBSECP256K1_CONFIG_H])
 8 AH_BOTTOM([#endif /*LIBSECP256K1_CONFIG_H*/])
 9 AM_INIT_AUTOMAKE([foreign subdir-objects])
10+
11+# Set -g (but not -O2 because this would override -O3 which we're adding later)
12+# if CFLAGS are not already set (see PROG_CC in the Autoconf manual)
13+: ${CFLAGS="-g"}

hebasto commented at 6:21 pm on February 17, 2021:

83fb1bcef49b1c12ef349f62d90bfcc83f0f7398

This causes multiple rm: conftest.dSYM: is a directory warnings on macOS (#896).

hebasto commented at 6:30 pm on February 17, 2021: member

This also means that if CFLAGS are defined by the user, then -g is not added (which does not seem to make much sense).

Do I understand correctly that the intention is to pass -g flag to a compiler regardless of flags provided by a user?

jonasnick commented at 7:04 pm on February 17, 2021: contributor

IIRC no, this PR makes it so that -g is passed if the user doesn’t provide custom CFLAGS.

real-or-random commented at 10:26 pm on February 17, 2021: contributor

So https://github.com/bitcoin-core/secp256k1/commit/83fb1bcef49b1c12ef349f62d90bfcc83f0f7398 means that the flags used during the tests are simply “-g” (instead of “-g -O2”), and this is what makes Apple’s fake gcc creating the directory. I believe we should simply set : ${CFLAGS=""} here. Or if we want to keep the -g as a default, we could remember whether CFLAGS was unset, and if not, add -g later.

Do we want to keep the -g default?

UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021

5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021

gades referenced this in commit d855cc511d on May 8, 2022

Allow overriding default flags #700