Old replacement of O3 in CMAKE_C_FLAGS_RELEASE skip spaces, which is problematic. For instance, if CMAKE_C_FLAGS_RELEASE = "-O3 -DFOO", regex will replace it with -O2-DFOO, which causes a compile error.
This patch changes this behavior, keeping whichever space exists between the flags.
If I may question, what is the rationale behind replacing O3 with O2? Changing the user's flags is a bad practice overall, and I don't see how this replacement is beneficial.
cc @hebasto
Thanks for the PR.
If I may question, what is the rationale behind replacing
O3withO2? Changing the user's flags is a bad practice overall, and I don't see how this replacement is beneficial.
We strongly recommend -O2. It turns out that -O3 is often not faster for our code. Even if it is slightly faster, we believe that -O2 is the better trade-off between performance and the risk of miscompilations, which, for a cryptographic library, includes the compiler adding branches to code which was supposed to be constant-time to avoid timing side channels. That's why also why almost all of our testing and assurance goes into -O2.
But we believe that the user should have the last word, of course. We give you a variable SECP256K1_APPEND_CFLAGS (still called SECP256K1_LATE_CFLAGS in the latest release) to overrule any flag.
Ok Tim, thanks for clarifying it.
184 | @@ -185,7 +185,7 @@ else() 185 | string(REGEX REPLACE "-DNDEBUG[ \t\r\n]*" "" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}") 186 | string(REGEX REPLACE "-DNDEBUG[ \t\r\n]*" "" CMAKE_C_FLAGS_MINSIZEREL "${CMAKE_C_FLAGS_MINSIZEREL}") 187 | # Prefer -O2 optimization level. (-O3 is CMake's default for Release for many compilers.) 188 | - string(REGEX REPLACE "-O3[ \t\r\n]*" "-O2" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}") 189 | + string(REGEX REPLACE "-O3([ \t\r\n]*)" "-O2\\1" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}")
Maybe make it more generic an robust:
string(REGEX REPLACE "(^| )-O3( |$)" "\\1-O2\\2" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}")
?
ACK 46922945b28485858ff1a2f75b26d7631a6d3bdc.
Changing the user's flags is a bad practice overall...
This can be resolved by using CMAKE_USER_MAKE_RULES_OVERRIDE_C. Feel free to pick https://github.com/hebasto/secp256k1/commit/90d876088e6ed4321dcac5ef8d45dc09d3c38d26 :)
Ok
re-ACK b8fe33332b7c6a7700f74be20c022fcc49753cd2.