build: Add SECP256K1_NO_EXPORTS option to avoid default visibility for static builds

theuni commented at 8:31 pm on May 29, 2025: contributor

Yep, yet another visiblity PR :)

Noticed this when building libbitcoinkernel for Linux. Core needs a way to link a static secp into a (shared or static) kernel without exporting the secp symbols.

autotools defines DLL_EXPORT for dll builds. CMake mimics this behavior and defines SECP256K1_DLL_EXPORT.

This means that currently Windows builds only export their symbols for shared libraries, and all other platforms export for shared AND static.

Unfortunately, there’s no way to make autotools define a special variable when building for other platforms.

CMake could define a variable for shared lib builds on all platforms, which the header could then use to decide whether or not to export symbols, but that would introduce a behavioral difference between the build-systems.

Instead, provide an escape hatch via SECP256K1_NO_EXPORTS which can be used when building secp for non-Windows targets when exports are not desired (when building a static lib).

in include/secp256k1.h:135 in 86a6ecd1c6 outdated

131@@ -132,7 +132,7 @@ typedef int (*secp256k1_nonce_function)(
132 #  if defined(DLL_EXPORT) || defined(SECP256K1_DLL_EXPORT)
133     /* Building libsecp256k1 as a DLL.
134      * 1. If using Libtool, it defines DLL_EXPORT automatically.
135-     * 2. In other cases, SECP256K1_DLL_EXPORT must be defined. */
136+     * 2. If using CMake, it defines SECP256K1_DLL_EXPORT automatically. */

real-or-random commented at 8:57 am on May 30, 2025:

I think what I had in mind here is rather this (and I agree this isn’t clear from the comment):

0     * 1. Libtool has a built-in -DDLL_EXPORT for all builds of DLLs.
1     * 2. Non-libtool builds should define SECP256K1_DLL_EXPORT. */

(… as we do in CMakeLists.txt)

in include/secp256k1.h:156 in 86a6ecd1c6 outdated

152@@ -153,7 +153,7 @@ typedef int (*secp256k1_nonce_function)(
153 #endif
154 #ifndef SECP256K1_API
155 /* All cases not captured by the Windows-specific logic. */
156-# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
157+# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD) && !defined(SECP256K1_NO_EXPORTS)

real-or-random commented at 8:58 am on May 30, 2025:

I think && !defined(SECP256K1_NO_EXPORTS) should also be added to line 131 for Windows. (If you set this for whatever reason, you probably expect it to work also on Windows.)

nit: SECP256K1_NO_EXPORT is a slightly better name because it matches (SECP256K1)_DLL_EXPORT.

theuni commented at 6:08 pm on May 30, 2025:

I added that to begin with but removed it because we can only reach this when building a Windows DLL, and it doesn’t make sense to do so without exporting.

I can add it back for symmetry if you’d like, but I’m not sure there’s any actual use for it?

real-or-random commented at 9:04 am on May 30, 2025: contributor

Sigh yeah.

Alternatively, we could wrap the entire Windows logic in an #ifndef SECP256K1_API, like the non-Windows logic, giving the user an ultimate override. That’s simpler. Is it better? I tend to say yes.

real-or-random commented at 9:09 am on May 30, 2025: contributor

Okay, actually… Let me take a step back. What do we (as libsecp256k1) want for static builds in general? Or is this a question that just cannot be answered properly because it depends on the user because some users would want to reexport, some won’t (e.g., kernel).

But even if the latter is true, it seems to me that the defaults should at least be consistent across platforms, no?

theuni commented at 6:24 pm on May 30, 2025: contributor

Okay, actually… Let me take a step back. What do we (as libsecp256k1) want for static builds in general? Or is this a question that just cannot be answered properly because it depends on the user because some users would want to reexport, some won’t (e.g., kernel).

But even if the latter is true, it seems to me that the defaults should at least be consistent across platforms, no?

IMO static builds should always be hidden by default and shared should be exported:

My initial attempt was something like:

 0diff --git a/include/secp256k1.h b/include/secp256k1.h
 1index 1d25a02a..8f718636 100644
 2--- a/include/secp256k1.h
 3+++ b/include/secp256k1.h
 4@@ -129,10 +129,10 @@ typedef int (*secp256k1_nonce_function)(
 5    * Attributes" in the GCC manual and the recommendations in
 6    * https://gcc.gnu.org/wiki/Visibility. */
 7 # if defined(SECP256K1_BUILD)
 8-#  if defined(DLL_EXPORT) || defined(SECP256K1_DLL_EXPORT)
 9+#  if defined(DLL_EXPORT) || defined(SECP256K1_EXPORT_SYMBOLS)
10     /* Building libsecp256k1 as a DLL.
11      * 1. If using Libtool, it defines DLL_EXPORT automatically.
12-     * 2. In other cases, SECP256K1_DLL_EXPORT must be defined. */
13+     * 2. In other cases, SECP256K1_EXPORT_SYMBOLS must be defined. */
14 #   define SECP256K1_API extern __declspec (dllexport)
15 #  else
16     /* Building libsecp256k1 as a static library on Windows.
17@@ -153,7 +153,7 @@ typedef int (*secp256k1_nonce_function)(
18 #endif
19 #ifndef SECP256K1_API
20 /* All cases not captured by the Windows-specific logic. */
21-# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
22+# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD) && defined(SECP256K1_EXPORT_SYMBOLS)
23    /* Building libsecp256k1 using GCC or compatible. */
24 #  define SECP256K1_API extern __attribute__ ((visibility ("default")))
25 # else
26diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
27index f31b8c8f..57bb7dae 100644
28--- a/src/CMakeLists.txt
29+++ b/src/CMakeLists.txt
30@@ -20,11 +20,9 @@ if(SECP256K1_ASM STREQUAL "arm32")
31   target_link_libraries(secp256k1_asm INTERFACE secp256k1_asm_arm)
32 endif()
33
34-if(WIN32)
35   # Define our export symbol only for shared libs.
36-  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL_EXPORT)
37+  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_EXPORT_SYMBOLS)
38   target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATIC>)
39-endif()
40
41 # Object libs don't know if they're being built for a shared or static lib.
42 # Grab the PIC property from secp256k1 which knows.

Sadly, that would make the CMake and Autotools behaviors diverge as there’s no way to get autotools to define anything for a shared build. But since Non-Windows+CMake is likely to be the most popular build config eventually (if it isn’t already), maybe we could accept that divergence and provide an additional opt-out for autotools builders?

It’s ideal though imo, as it does the right thing by default in all cases, and would allow -DSECP256K1_EXPORT_SYMBOLS to be manually specified for anyone building statically and wanting exports.

As mentioned above, I’m not sure there’s any utility in allowing the opposite.. building shared and disabling exports.

theuni commented at 6:30 pm on May 30, 2025: contributor

Btw, this is only becoming an issue now because we (the kernel working group) were discussing a monolithic libbitcoinkernel.a yesterday, which would include secp: https://github.com/theuni/bitcoin/commit/4f43838247fa62b8495133b3924ebb64f08dd0ec

While testing that, the shared version (libbitcoinkernel.so) is now comprised of the secp objects rather than libsecp256k1.a. That means that Core’s trick of using "-Wl,--exclude-libs,ALL" to exclude secp’s symbols no longer works, as it only applies to static libraries. With that config, we need a secp actually built with hidden symbols. Though I would argue that the fact that they’re currently visible is a bug anyway.

real-or-random commented at 7:29 am on May 31, 2025: contributor

While testing that, the shared version (libbitcoinkernel.so) is now comprised of the secp objects rather than libsecp256k1.a.

Is this what you mean by monolithic? What would be the benefit of this?

It feels to me that if you consume the .o files directly (while I assume you have a perfectly fine reason to do this), this is a violation of libsecp interface, so I’m not sure if we should support this in general. Or, more pragmatically, this is too niche for us to support. (At least for other users, of course, our primary concern should be to support Core’s use of libsecp256k1.)

Though I would argue that the fact that they’re currently visible is a bug anyway.

But yes, I agree. I tend to agree that hidden is a sane default for static libs, and that reexporting should be asked for explicitly.

I tend to like your SECP256K1_EXPORT_SYMBOLS suggestion.

It’s ideal though imo, as it does the right thing by default in all cases, and would allow -DSECP256K1_EXPORT_SYMBOLS to be manually specified for anyone building statically and wanting exports.

Indeed.

Sadly, that would make the CMake and Autotools behaviors diverge as there’s no way to get autotools to define anything for a shared build. But since Non-Windows+CMake is likely to be the most popular build config eventually (if it isn’t already), maybe we could accept that divergence and provide an additional opt-out for autotools builders?

The question for our autotools build will be: Should it define SECP256K1_EXPORT_SYMBOLS or not? If we don’t set it, any users who want to reexport will have a problem. (Are there any?) If we set it, then we have a non-optimal default. But we could make it configurable. While this adds a knob, it’s a simple one and it would also keep the two build systems on par.

By the way, for anyone wondering what visibility is supposed to do anyway in a static build (because visibility is a concept that applies only to shared libs): The only way visibility influences a static lib is that the visibility information is kept in the static lib, solely for the potential case that this static library will be linked into a shared lib (and the visibility will apply there again). Please see this excellent answer on SO: https://stackoverflow.com/a/67473340/2725281

real-or-random commented at 7:30 am on May 31, 2025: contributor

cc @hebasto

real-or-random added the label build on May 31, 2025

real-or-random added the label refactor/smell on May 31, 2025

cmake: don't set default visibility for static builds

define SECP256K1_CMAKE_SHARED_BUILD for all CMake shared lib builds, not just
Windows. This allows us to only set default symbol visibility for shared lib builds.

For static libs built with CMake, or Autotools builds for Windows, static
builds will prefer to hide symbols by default. Autotools does not have the
ability to set a define for non-Windows shared builds, so the best we can do
there is guess. SECP256K1_NO_EXPORT_SYMBOLS can be defined as an escape-hatch
in case a non-Windows CMake builder wants default visibility.

c5f2b46185

Revert "build: Drop no longer needed `-fvisibility=hidden` compiler option"

This reverts commit d1478763a5f400fafb42af5911db4a9460dd4a5d.

Hidden visiblity is still useful for builders including a static libsecp.

3eef7362c4

theuni force-pushed on Jun 2, 2025

theuni commented at 7:19 pm on June 2, 2025: contributor

While testing that, the shared version (libbitcoinkernel.so) is now comprised of the secp objects rather than libsecp256k1.a.

Is this what you mean by monolithic? What would be the benefit of this?

It feels to me that if you consume the .o files directly (while I assume you have a perfectly fine reason to do this), this is a violation of libsecp interface, so I’m not sure if we should support this in general. Or, more pragmatically, this is too niche for us to support. (At least for other users, of course, our primary concern should be to support Core’s use of libsecp256k1.)

See the commit linked above. This came out of a discussion about having a self-contained libbitcoinkernel.a which requires no other dependencies. As things stand right now, when you build a static kernel in Core and try to use it (for a new binary or shared lib), you have to link in the other static libs as well at that time. Sadly, CMake won’t “link” (combine) one static lib into another, it simply notes the dependency. For our internal libs (crypto, leveldb, etc.) that’s easily solvable by linking them in as objects instead.

The only solution that seems to work is to do the same with secp: linking in its objects. I agree this is a violation of the interface as Core shouldn’t have to know anything about secp’s internals. Ideally there would be a convenience library to handle this, something like secp256k1_objects. I’ve pushed a demo commit for that here: https://github.com/theuni/secp256k1/commit/8bb10fe04908041a104a66d8ceab2dd97395e314 . That lets Core’s kernel lib link against secp’s objects without having to know anything about them. What do you think about PRing that here?

Though I would argue that the fact that they’re currently visible is a bug anyway.

But yes, I agree. I tend to agree that hidden is a sane default for static libs, and that reexporting should be asked for explicitly.

I tend to like your SECP256K1_EXPORT_SYMBOLS suggestion.

It’s ideal though imo, as it does the right thing by default in all cases, and would allow -DSECP256K1_EXPORT_SYMBOLS to be manually specified for anyone building statically and wanting exports.

Indeed.

Sadly, that would make the CMake and Autotools behaviors diverge as there’s no way to get autotools to define anything for a shared build. But since Non-Windows+CMake is likely to be the most popular build config eventually (if it isn’t already), maybe we could accept that divergence and provide an additional opt-out for autotools builders?

The question for our autotools build will be: Should it define SECP256K1_EXPORT_SYMBOLS or not? If we don’t set it, any users who want to reexport will have a problem. (Are there any?) If we set it, then we have a non-optimal default. But we could make it configurable. While this adds a knob, it’s a simple one and it would also keep the two build systems on par.

I’ve pushed a revised version of this. It guesses that non-Windows Autotools builders always want default visibility, and provides an opt-out to override.

SECP256K1_EXPORT_SYMBOLS was becoming overloaded, so I changed the defines to describe the actual conditions instead: SECP256K1_CMAKE_SHARED_BUILD, SECP256K1_AUTOTOOLS_BUILD, and SECP256K1_NO_EXPORT_SYMBOLS (the opt-out for both). SECP256K1_DLL_EXPORT is retained.

By the way, for anyone wondering what visibility is supposed to do anyway in a static build (because visibility is a concept that applies only to shared libs): The only way visibility influences a static lib is that the visibility information is kept in the static lib, solely for the potential case that this static library will be linked into a shared lib (and the visibility will apply there again). Please see this excellent answer on SO: https://stackoverflow.com/a/67473340/2725281

Yes, exactly, thanks for the link :)

Additionally, d1478763a5f400fafb42af5911db4a9460dd4a5d is reverted here as that’s needed to actually do the hiding.

real-or-random commented at 11:24 am on June 3, 2025: contributor

Additionally, d147876 is reverted here as that’s needed to actually do the hiding.

Wait. This is the story on non-Windows:

So first we had -fvisibility=hidden to set gcc’s default to “hidden” together with __attribute__ ((visibility ("default"))) to override it for our API exports.
Then https://github.com/bitcoin-core/secp256k1/pull/1359/commits/88548058b3e88f7f1ce7c1abbe12da8105a858cc got rid of the -fvisibility=hidden default by adding __attribute__ ((visibility ("hidden"))) to all non-static definitions that should not be exported.

But this means we also won’t need to set __attribute__ ((visibility ("default"))) anywhere. This makes it much simpler… See #1677.

 0# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
 1   /* Building libsecp256k1 using GCC or compatible.
 2    *
 3    * We would like to set default visibility if building a shared library and
 4    * hidden visibility if building a static library.
 5    * 1. Libtool has a builtin -DPIC (and -fPIC on targets that support it).
 6    * 2. Our CMake build defines SECP256K1_SHARED_BUILD automatically.
 7    * 3. In all other cases, we guess based on __PIC__, which is (always defined
 8    *    and) greater than 0 if -fpic or -fPIC is used. */
 9#  if defined(PIC) || defined(SECP256K1_EXPORT_SYMBOLS) || (defined(__PIC__) && __PIC__ > 0)
10#   define SECP256K1_API extern __attribute__ ((visibility ("default")))
11#  elif defined(SECP256K1_NO_EXPORT_SYMBOLS)
12#   define SECP256K1_API extern __attribute__ ((visibility ("hidden")))
13#  endif
14# endif

real-or-random commented at 11:34 am on June 3, 2025: contributor

The only solution that seems to work is to do the same with secp: linking in its objects. I agree this is a violation of the interface as Core shouldn’t have to know anything about secp’s internals. Ideally there would be a convenience library to handle this, something like secp256k1_objects. I’ve pushed a demo commit for that here: theuni@8bb10fe . That lets Core’s kernel lib link against secp’s objects without having to know anything about them. What do you think about PRing that here?

Yeah, why not. If Core needs this somewhere, then it’s easier to maintain it here.

theuni commented at 4:07 pm on June 3, 2025: contributor

Additionally, d147876 is reverted here as that’s needed to actually do the hiding.

Wait. This is the story on non-Windows:
1. So first we had `-fvisibility=hidden` to set gcc's default to "hidden" together with `__attribute__ ((visibility ("default")))` to override it for our API exports.

2. Then [8854805](https://github.com/bitcoin-core/secp256k1/commit/88548058b3e88f7f1ce7c1abbe12da8105a858cc) got rid of the `-fvisibility=hidden` default by adding  `__attribute__ ((visibility ("hidden")))` to all non-static definitions that should not be exported.
But this means we also won’t need to set __attribute__ ((visibility ("default"))) anywhere. This makes it much simpler… See #1677.

Yep, I agree with everything here. As I mentioned in #1677, this would generally do the right thing, but it would break builds for shared lib builders who compile with -fvisibility=hidden. Though that’s unnecessary for secp, it’s pretty common :(

theuni commented at 4:20 pm on June 3, 2025: contributor

For posterity, it is possible to check whether a shared library is built by libtool. This is an earlier attempt of mine:

Unfortunately, PIC isn’t reliable for detecting shared libs either :(

See Core’s autoconf depends builds for example: https://github.com/bitcoin/bitcoin/blob/master/depends/funcs.mk#L187

Even when building static libs, we need to compile PIC as the objects will eventually be used in PIE binaries.

real-or-random commented at 6:49 pm on June 3, 2025: contributor

Unfortunately, PIC isn’t reliable for detecting shared libs either :(

Good point. Though I still think that defined(PIC), which seems to be a separate libtool (legacy?) thing, would make it possible to detect libtool shared builds. Note that this is just -DPIC on the command line, and entirely separate from __PIC__ which represents the -fPIC flag.

I agree that checking __PIC__ is just a bad heuristic. You wouldn’t only get false positives as in the example mention, but possibly also false negative. On some platforms, you can have shared libs without PIC. So if you can’t resist the temptation to guess based on __PIC__, you’ll need a two-sided manual override in addition to it…

Anyway, let’s just avoid the need to detect shared builds.

hebasto commented at 7:35 am on June 4, 2025: member

The only solution that seems to work is to do the same with secp: linking in its objects. I agree this is a violation of the interface as Core shouldn’t have to know anything about secp’s internals. Ideally there would be a convenience library to handle this, something like secp256k1_objects.

CMake’s OBJECT library is a drop-in replacement for Libtool’s “convenience” library, isn’t it? Neither is intended for installation, nor can either be regarded as part of a package’s public interface.

Both also work well when a parent project integrates a subproject at the build system level.

hebasto commented at 3:19 pm on June 5, 2025: member

I haven’t tested it, but this seems like a plausible approach: set(CMAKE_C_VISIBILITY_PRESET hidden) in the parent project that consumes the static library, and make no changes in this repo?

real-or-random commented at 7:15 pm on June 5, 2025: contributor

I haven’t tested it, but this seems like a plausible approach: set(CMAKE_C_VISIBILITY_PRESET hidden) in the parent project that consumes the static library, and make no changes in this repo?

I doubt that will work. If you set set(CMAKE_C_VISIBILITY_PRESET hidden), then you’ll get -fvisibility=hidden on the command line. But if we don’t change the library here, the API symbols will have an __attribute__((visbility("default")) which overrides the -fvisibility=hidden.

build: Add SECP256K1_NO_EXPORTS option to avoid default visibility for static builds #1674