build: Refactor visibility logic and add override #1696

pull real-or-random wants to merge 3 commits into bitcoin-core:master from real-or-random:202506-noexport3 changing 3 files +57 −38
  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. real-or-random force-pushed on Jul 2, 2025
  9. real-or-random commented at 10:33 am on July 2, 2025: contributor
    cc @theuni @purpleKarrot
  10. 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.

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

  12. real-or-random commented at 7:13 pm on July 2, 2025: contributor
    (edit: nevermind, got it now)
  13. 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.

  14. fanquake commented at 1:09 pm on July 3, 2025: member
    cc @purpleKarrot
  15. purpleKarrot commented at 1:47 pm on July 3, 2025: contributor

    I don’t get why additional #defines or cmake-options are necessary. As I pointed out in #1677 (comment), there are three different modes the header file needs to be interpreted as. To distinguish between these three cases, two defines are needed: SECP256K1_STATIC and SECP256K1_BUILD.

    SECP256K1_BUILD needs to be defined when building the library, no matter whether the library is static or dynamic. This is defined in src/secp256k1.c before including secp256k1.h. The current approach is build system agnostic. No further change needed.

    SECP256K1_STATIC needs to be defined when a static library is used or consumed. Both Makefile.am and CMakeLists.txt already mention this name. The only necessary change is to drop the “if (windows)” condition.

    (Both cmake and libtool set a define symbol when building a shared library. This symbol alone is insufficient to distinguish between three different cases. When using custom defines, the symbols set by those tools is redundant. The approach of using SECP256K1_STATIC and SECP256K1_BUILD is portable and build system independent.)

  16. hebasto commented at 2:15 pm on July 3, 2025: member

    I’ve spent some time re-reading all the other competing PRs, including #1674, #1677, #1695), and discussing the topic offline with @purpleKarrot.

    Concept ACK on this one.

    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.

    +1.

  17. purpleKarrot commented at 3:11 pm on July 3, 2025: contributor

    I am failing to reason about all the different paths through that #ifdef-maze and whether they are truly all covered by actual requirements. Exactly what is the use case this new code wants to support without breaking what other requirements? I am sure such a list exits, even if not explicitly written down, otherwise we would not have this discussion. Extracting a platform abstraction as suggested in #1677 (comment) and https://gcc.gnu.org/wiki/Visibility (see the Step-by-step guide) could help comparing the code with those requirements.

    The latter might be the right thing in a new library …

    I am a bit concerned that new libraries might follow the example set here. There should be a big disclaimer that points people to that GCC wiki page.

  18. in include/secp256k1.h:124 in 82b7412024 outdated 
    148-   * library on Windows. */
149-# elif !defined(SECP256K1_STATIC)
150-   /* Consuming libsecp256k1 as a DLL. */
151-#  define SECP256K1_API extern __declspec (dllimport)
152-# endif
153+#if !defined(SECP256K1_API) && defined(SECP256K1_NO_VISIBILITY_ATTRIBUTES)
    hebasto commented at 3:32 pm on July 3, 2025:

    My apologies if this was discussed earlier and I missed it, but it seems that SECP256K1_NO_VISIBILITY_ATTRIBUTES should only be considered only when building the library, not when consuming it:

    0#if !defined(SECP256K1_API) && defined(SECP256K1_BUILD) && defined(SECP256K1_NO_VISIBILITY_ATTRIBUTES)
    real-or-random commented at 6:16 am on July 15, 2025:
    I don’t think that it hurts to apply it when consuming the library. Or does it have any undesirable effects?
    hebasto commented at 11:49 am on July 18, 2025:

    I don’t think that it hurts to apply it when consuming the library. Or does it have any undesirable effects?

    None of those. I only found it easier to read the code.

  19. jonasnick added this to the milestone 0.7.0 on Jul 14, 2025
  20. real-or-random commented at 6:39 am on July 15, 2025: contributor

    SECP256K1_STATIC needs to be defined when a static library is used or consumed. Both Makefile.am and CMakeLists.txt already mention this name. The only necessary change is to drop the “if (windows)” condition.

    This changes the meaning of SECP256K1_STATIC. So far, it’s a consume-only macro (as explained in the code comments here). I’d like to avoid the need to set SECP256K1_STATIC when building a static library, and only in Windows. (In earlier versions, we didn’t even require the user to set SECP256K1_STATIC when consuming the library, but this was not possible without accepting that MSVC emits warnings, so it was changed.)

    Why avoid the need to set SECP256K1_STATIC when building? First, it’s more convenient for anyone who runs a build. Second, that’s how it always was, so it’s in particular more convenient for people who upgrade from earlier versions. Third, the existing logic is well-tested and has worked so far – we’re not fighting a bug, we’re responding to a (somewhat unusual) feature request from Bitcoin Core – so I’d like to avoid semantic changes that could introduce bugs. (But yes, refactoring seems a good idea.) Fourth, I don’t know what the implications for autotools builds are: The ./configure script by default creates a build system that builds both the shared and the static library. If you ask me, that’s a bad design choice in autoconf, but there’s not much we can do about it except force the user to select one of the builds, which again, is not convenient and also unexpected to users familiar with autotools.

    (Both cmake and libtool set a define symbol when building a shared library.)

    I doubt that’s true. As explained by @theuni in #1674, there’s no such macro. There’s just EXPORT_DLL, but this is set only on Windows.

    The approach of using SECP256K1_STATIC and SECP256K1_BUILD is portable and build system independent.

    Sure, writing it from scratch would lead to simpler code but my point is that the aforementioned arguments are stronger.

  21. real-or-random force-pushed on Jul 15, 2025
  22. real-or-random commented at 6:52 am on July 15, 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.

    Renamed to SECP256K1_API_FUNCTION_VISIBILITY_ATTRIBUTES

    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

    Cherry-pick (and renamed the option accordingly).

    It would be nice to get reviews by the end of the week, then we may get this into 0.7.0.

  23. in include/secp256k1.h:129 in 60c6bd8c38 outdated 
    163+#if !defined(SECP256K1_API)
164+#    if defined(SECP256K1_BUILD)
165+         /* On Windows, assume a shared library only if explicitly requested.
166+          *   1. If using Libtool, it defines DLL_EXPORT automatically.
167+          *   2. In other cases, SECP256K1_DLL_EXPORT must be defined. */
168+#        if defined(_WIN32) && defined(SECP256K1_DLL_EXPORT) || defined(DLL_EXPORT)
    theuni commented at 9:09 pm on July 17, 2025:
    Nit: mind adding parentheses here make it easier to parse with my simple brain? :)
    real-or-random commented at 6:54 am on July 18, 2025:
    oh sure, done
  24. theuni approved
  25. theuni commented at 9:15 pm on July 17, 2025: contributor

    Looks good to me.

    I’m really not too concerned about the implementation as long as we have the escape hatch. Thanks for adding the CMake option, that should make Core’s integration nice and clean.

  26. build: Refactor visibility logic e5297f6d79
  27. real-or-random force-pushed on Jul 18, 2025
  28. hebasto commented at 11:37 am on July 18, 2025: member

    (Both cmake and libtool set a define symbol when building a shared library.)

    I doubt that’s true.

    It is. By default, CMake defines -D{lib_name}_EXPORTS. It happens to be overridden here:https://github.com/bitcoin-core/secp256k1/blob/cbbbf3bd6e9bcad5f24feee2d2da351b92de1d89/src/CMakeLists.txt#L73-L75

  29. hebasto commented at 11:45 am on July 18, 2025: member

    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.

    Renamed to SECP256K1_API_FUNCTION_VISIBILITY_ATTRIBUTES

    Which change exactly are you referring to?

  30. build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES ce7923874f
  31. in CMakeLists.txt:45 in ef008ce105 outdated 
    41@@ -42,6 +42,8 @@ endif()
42 
43 option(SECP256K1_INSTALL "Enable installation." ${PROJECT_IS_TOP_LEVEL})
44 
45+option(SECP256K1_ENABLE_API_VISIBILITY_ATTRIBUTES "Enable visibility attributes in the API." ON)
    hebasto commented at 11:47 am on July 18, 2025:
    But SECP256K1_ENABLE_VISIBILITY_ATTRIBUTES is used in the code below.
    real-or-random commented at 11:55 am on July 18, 2025:
    …. oops, fixed.
  32. real-or-random force-pushed on Jul 18, 2025
  33. real-or-random commented at 11:56 am on July 18, 2025: contributor

    Which change exactly are you referring to?

    I’m not entirely sure what your question is, but the name should now both contain API in the C code now and in CMake. I added API to the name to emphasize that the setting applies only to symbols in the public API.

  34. in CMakeLists.txt:317 in 715d4eb4f9 outdated 
    313@@ -312,6 +314,7 @@ else()
314   set(cross_status "FALSE")
315 endif()
316 message("Cross compiling ....................... ${cross_status}")
317+message("Visibility attributes ................. ${SECP256K1_ENABLE_API_VISIBILITY_ATTRIBUTES}")
    hebasto commented at 11:57 am on July 18, 2025: 
    0message("API visibility attributes ............. ${SECP256K1_ENABLE_API_VISIBILITY_ATTRIBUTES}")
  35. in src/CMakeLists.txt:57 in 715d4eb4f9 outdated 
    53@@ -54,6 +54,10 @@ add_library(secp256k1_precomputed OBJECT EXCLUDE_FROM_ALL
54 # from being exported.
55 target_sources(secp256k1 PRIVATE secp256k1.c $<TARGET_OBJECTS:secp256k1_precomputed>)
56 
57+if(NOT SECP256K1_ENABLE_VISIBILITY_ATTRIBUTES)
    hebasto commented at 11:57 am on July 18, 2025: 
    0if(NOT SECP256K1_ENABLE_API_VISIBILITY_ATTRIBUTES)
  36. real-or-random commented at 11:58 am on July 18, 2025: contributor

    (Both cmake and libtool set a define symbol when building a shared library.)

    I doubt that’s true.

    It is. By default, CMake defines -D{lib_name}_EXPORTS. It happens to be overridden here:

    https://github.com/bitcoin-core/secp256k1/blob/cbbbf3bd6e9bcad5f24feee2d2da351b92de1d89/src/CMakeLists.txt#L73-L75

    Indeed, but my claim is that there is no thing for libtool or autotools. The only define that libtool sets is EXPORT_DLL, but this is restricted to Windows.

  37. real-or-random force-pushed on Jul 18, 2025
  38. build: add CMake option for disabling symbol visibility attributes 
    Co-authored-by: Tim Ruffing <me@real-or-random.org>
    c82d84bb86
  39. real-or-random force-pushed on Jul 18, 2025
  40. hebasto approved
  41. hebasto commented at 1:37 pm on July 18, 2025: member

    ACK c82d84bb86f9769a4f9d958f40e86cd99e071bc7, I have reviewed the code and it looks OK.

    Tested on Ubuntu 25.04 with Bitcoin Core in the “monokernel” integration scenario with additional set(CMAKE_C_VISIBILITY_PRESET hidden) and set(SECP256K1_ENABLE_API_VISIBILITY_ATTRIBUTES OFF CACHE BOOL "" FORCE).

    UPD. Additionally, the same scenario has been successfully tested on macOS Sequoia 15.5.

  42. real-or-random merged this on Jul 21, 2025
  43. real-or-random closed this on Jul 21, 2025


real-or-random hebasto theuni fanquake purpleKarrot

Labels
feature build refactor/smell

Milestone
0.7.0

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: 2025-07-31 06:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me