Fixed O3 replacement #1555

1. 
   EduMenges commented at 2:52 pm on June 27, 2024: contributor

   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.

2. real-or-random added the label build on Jun 27, 2024
3. 
   real-or-random commented at 4:25 pm on June 27, 2024: contributor

   cc @hebasto

   Thanks for the PR.

   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.

   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.

4. 
   EduMenges commented at 4:57 pm on June 27, 2024: contributor

   Ok Tim, thanks for clarifying it.
5. in CMakeLists.txt:188 in 46922945b2 outdated

   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}")
   

   ---

   ---

   

   hebasto commented at 5:00 pm on June 27, 2024:

   Maybe make it more generic an robust:

   0  string(REGEX REPLACE "(^| )-O3( |$)" "\\1-O2\\2" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}")
   

   ?

6. hebasto approved
7. 
   hebasto commented at 5:29 pm on June 27, 2024: member

   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 :)

8. EduMenges force-pushed on Jun 27, 2024
9. 
   hebasto commented at 8:25 pm on June 27, 2024: member

   CI fails:

   0CMake Error at CMakeLists.txt:188 (string):
   1  string sub-command REGEX, mode REPLACE failed to compile regex "-O3(
   2  |$)*)".
   

10. 
    gatleas17 commented at 3:56 am on June 28, 2024: none

    Ok
11. EduMenges force-pushed on Jun 28, 2024
12. cmake: Fixed O3 replacement b8fe33332b
13. EduMenges force-pushed on Jun 28, 2024
14. hebasto approved
15. 
    hebasto commented at 9:09 am on June 29, 2024: member

    re-ACK b8fe33332b7c6a7700f74be20c022fcc49753cd2.
16. real-or-random merged this on Jun 29, 2024
17. real-or-random closed this on Jun 29, 2024

    ---

Contributors
EduMenges real-or-random hebasto gatleas17

Labels
build