build: Refactor visibility logic and add override #1696

1. 
   real-or-random commented at 10:16 am on July 2, 2025: contributor

   This is less invasive than #1695. The latter might be the right thing in a new library (and then we’d probably not support autotools in the first place), but any semantic change to this code has the potential to create news bug, or at least breakages for downstream users.

   This is different from #1677 in that it does not set hidden explicitly. I agree with the comment in #1677 that setting hidden violates the principle of least surprise.

   So this similar in spirit to #1674. So I wonder if this should also include https://github.com/bitcoin-core/secp256k1/pull/1674/commits/3eef7362c45e5d4c0d4b06ec4d9af30be8642e5d. I’d say no, fvisibility should then set by the user. But can you, in CMake, set CMAKE_C_VISIBILITY_PRESET from a parent project?

2. real-or-random force-pushed on Jul 2, 2025
3. real-or-random force-pushed on Jul 2, 2025
4. real-or-random added the label build on Jul 2, 2025
5. real-or-random added the label feature on Jul 2, 2025
6. real-or-random added the label refactor/smell on Jul 2, 2025
7. 
   hebasto commented at 10:21 am on July 2, 2025: member

   But can you, in CMake, set CMAKE_C_VISIBILITY_PRESET from a parent project?

   Yes, we can.

8. build: Refactor visibility logic 60c6bd8c38
9. build: Add SECP256K1_NO_VISIBILITY_ATTRIBUTES 82b7412024
10. real-or-random force-pushed on Jul 2, 2025
11. 
    real-or-random commented at 10:33 am on July 2, 2025: contributor

    cc @theuni @purpleKarrot
12. 
    theuni commented at 6:03 pm on July 2, 2025: contributor

    LGTM.

    A few thoughts:

    • Suggest renaming to SECP256K1_NO_FUNCTION_VISIBILITY_ATTRIBUTES because this does not affect SECP256K1_LOCAL_VAR.
    • To address @hebasto’s comment, a CMake option could be added to define SECP256K1_NO_FUNCTION_VISIBILITY_ATTRIBUTES during build, so that parent projects can use it rather than having to mess with cflags.
    • If you don’t want to go as far as reverting 3eef7362c45e5d4c0d4b06ec4d9af30be8642e5d, symbols could be hidden by default but override-able.

    Those two would add up to:

    0option(SECP256K1_ENABLE_VISIBILITY_ATTRIBUTES "Enable visibility attributes when compiling the library" ON)
    1if (NOT DEFINED CMAKE_C_VISIBILITY_PRESET)
    2  set(CMAKE_C_VISIBILITY_PRESET hidden)
    3endif()
    4...
    5if(NOT SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES)
    6  target_compile_definitions(secp256k1 PRIVATE SECP256K1_NO_FUNCTON_VISIBILITY_ATTRIBUTES)
    7endif() 
    

    But I noticed while testing combinations of the above that it leads to broken outcomes for test binaries when simply disabling the SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES option, since then by default we’d be building shared libs with hidden visibility and no default attributes for the abi.

    To be robust against that, we’d need something like:

    0option(SECP256K1_ENABLE_VISIBILITY_ATTRIBUTES "Enable visibility attributes when compiling the library" ON)
    1if (SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES AND NOT DEFINED CMAKE_C_VISIBILITY_PRESET)
    2  set(CMAKE_C_VISIBILITY_PRESET hidden)
    3endif()
    

    Which of course would defeat the purpose. So I propose that we just skip trying to set CMAKE_C_VISIBILITY_PRESET altogether. Parent projects can mess with that when they know what they’re doing.

    tl;dr: I think this is the best solution so far, but propose adding a helper option to make life easier for parent projects: https://github.com/theuni/secp256k1/commit/e22ed8ce8c29fef487c88be34a34cbff17e7e2c7

    Core would then set CMAKE_C_VISIBILITY_PRESET=hiddden and SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES=false and be all set.

13. 
    real-or-random commented at 7:09 pm on July 2, 2025: contributor

    Suggest renaming to SECP256K1_NO_FUNCTION_VISIBILITY_ATTRIBUTES because this does not affect SECP256K1_LOCAL_VAR.

    True, but we also apply it to variables, e.g., secp256k1_context_static. And the LOCAL_VAR are really internal.

    Perhaps SECP256K1_NO_API_VISIBILITY_ATTRIBUTES. Well, if someone has a shorter name that is still clear, please let me know. :)

14. 
    real-or-random commented at 7:13 pm on July 2, 2025: contributor

    (edit: nevermind, got it now)
15. 
    real-or-random commented at 7:22 pm on July 2, 2025: contributor

    But I noticed while testing combinations of the above that it leads to broken outcomes for test binaries when simply disabling the SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES option, since then by default we’d be building shared libs with hidden visibility and no default attributes for the abi.

    Right, that’s the just same problem as with your #1674.

    Which of course would defeat the purpose. So I propose that we just skip trying to set CMAKE_C_VISIBILITY_PRESET altogether. Parent projects can mess with that when they know what they’re doing.

    tl;dr: I think this is the best solution so far, but propose adding a helper option to make life easier for parent projects: theuni@e22ed8c

    Sounds good to me.

Contributors
real-or-random hebasto theuni

Labels
feature build refactor/smell