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

[theuni]

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

[real-or-random]

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]

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

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

(... as we do in CMakeLists.txt)

[real-or-random]

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]

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]

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]

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:

diff --git a/include/secp256k1.h b/include/secp256k1.h
index 1d25a02a..8f718636 100644
--- a/include/secp256k1.h
+++ b/include/secp256k1.h
@@ -129,10 +129,10 @@ typedef int (*secp256k1_nonce_function)(
    * Attributes" in the GCC manual and the recommendations in
    * https://gcc.gnu.org/wiki/Visibility. */
 # if defined(SECP256K1_BUILD)
-#  if defined(DLL_EXPORT) || defined(SECP256K1_DLL_EXPORT)
+#  if defined(DLL_EXPORT) || defined(SECP256K1_EXPORT_SYMBOLS)
     /* Building libsecp256k1 as a DLL.
      * 1. If using Libtool, it defines DLL_EXPORT automatically.
-     * 2. In other cases, SECP256K1_DLL_EXPORT must be defined. */
+     * 2. In other cases, SECP256K1_EXPORT_SYMBOLS must be defined. */
 #   define SECP256K1_API extern __declspec (dllexport)
 #  else
     /* Building libsecp256k1 as a static library on Windows.
@@ -153,7 +153,7 @@ typedef int (*secp256k1_nonce_function)(
 #endif
 #ifndef SECP256K1_API
 /* All cases not captured by the Windows-specific logic. */
-# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
+# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD) && defined(SECP256K1_EXPORT_SYMBOLS)
    /* Building libsecp256k1 using GCC or compatible. */
 #  define SECP256K1_API extern __attribute__ ((visibility ("default")))
 # else
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index f31b8c8f..57bb7dae 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -20,11 +20,9 @@ if(SECP256K1_ASM STREQUAL "arm32")
   target_link_libraries(secp256k1_asm INTERFACE secp256k1_asm_arm)
 endif()

-if(WIN32)
   # Define our export symbol only for shared libs.
-  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL_EXPORT)
+  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_EXPORT_SYMBOLS)
   target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATIC>)
-endif()

 # Object libs don't know if they're being built for a shared or static lib.
 # 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]

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: theuni/bitcoin@4f43838

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]

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]

cc @hebasto

[theuni]

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

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, d147876 is reverted here as that's needed to actually do the hiding.

[real-or-random]

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 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.

For posterity, it is possible to check whether a shared library is built by libtool.

This is an earlier attempt of mine:

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

[real-or-random]

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]

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]

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.