Fixed O3 replacement #1555

pull EduMenges wants to merge 1 commits into bitcoin-core:master from GeniusVentures:master changing 1 files +1 −1
  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


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-09 07:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me