build: Add SECP256K1_FORCE_HIDDEN_VISIBILITY

[real-or-random]

Alternative to #1674.

[hebasto]

Mind sharing more details regarding "the visibility settings will be stored in the static library" please?

[real-or-random]

See https://stackoverflow.com/a/67473340/2725281. I don't think I have more to share, but I assume @theuni can confirm the behavior.

[real-or-random]

Like #1674, this defaults to "default" (haha) visibility. While not perfect for static builds -- we'd probably prefer a default of "hidden" there -- this avoids any attempt to detection in the preprocessor whether a static or a shared library is built. Some kind of detection is possible (see my comment in #1674), but it will most likely never be perfect, and some manual override will be necessary anyway. Defaulting to "default" avoids the need to detect and is thus much simpler.

[theuni]

Hmm.

There's a pretty common assumption that when using -DCMAKE_C_VISIBILITY_PRESET=hidden or CXXFLAGS="-fvisibility=hidden", necessary symbols will be override that setting and be set to default. That's the premise behind the de-facto guide, at least.

So I think many packagers would run into problems with this, as that flag (which is usually considered safe for visibility-aware libs) would essentially produce broken shared libs (except on Windows).

[real-or-random]

Hm, that assumption sounds misguided to me. If a packager expects a visibility-aware lib, they should let the lib set the flags...

Whatever, I think it's easy to address this concern by setting "default" in an else branch:

#ifndef SECP256K1_API
# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
#  if defined(SECP256K1_FORCE_HIDDEN_VISIBILITY)
#   define SECP256K1_API extern __attribute__ ((visibility ("hidden")))
#  else
#   define SECP256K1_API extern __attribute__ ((visibility ("default")))
#  endif
# endif
#endif

What do you think?

[theuni]

Hm, that assumption sounds misguided to me. If a packager expects a visibility-aware lib, they should let the lib set the flags...

libsecp's in a pretty unique position here because it has no private non-static functions and only a handful of private vars. Most libs have a huge pile of functions and marking them all as hidden would be infeasible. Hence the compiler flag/default override combo.

Edit:
Arguably the lib is setting the flags if it sets CMAKE_C_VISIBILITY_PRESET to "hidden". For the reason I just mentioned, for most libs, that's more feasible. A packager then having that on by default in their toolchain file is just a noop.

Whatever, I think it's easy to address this concern by setting "default" in an else branch:

#ifndef SECP256K1_API
# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
#  if defined(SECP256K1_FORCE_HIDDEN_VISIBILITY)
#   define SECP256K1_API extern __attribute__ ((visibility ("hidden")))
#  else
#   define SECP256K1_API extern __attribute__ ((visibility ("default")))
#  endif
# endif
#endif

What do you think?

Sure, it's not conventional, but it gets the job done :)

[real-or-random]

Sure, it's not conventional, but it gets the job done :)

Ok, I can update the PR tomorrow. :)

---

A packager then having that on by default in their toolchain file is just a noop.

Yep. So for visibility-aware libs, it's a noop, and for non-aware libs, it breaks the build. Sounds like an option you don't wanna have in your toolchain file. ;) Okay, just kidding, I guess there may be aware libs which leave setting the compiler detault to the user.

[TheCharlatan]

Not sure yet what to think of the two approaches, but it does strike me as very unfortunate that we are introducing more bespoke behaviour yet again. Can we at least only allow this being set when SECP256K1_DISABLE_SHARED is ON?

[real-or-random]

but it does strike me as very unfortunate that we are introducing more bespoke behaviour yet again.

I am not sure how bespoke this is, in the end. I think the ability to configure visibility for static builds is a valid use case in general, not specific to Bitcoin Core's planned use. It's just pretty niche... (That's different from #1678, which is merely a hack that works around a lack of functionality in CMake.)

edit: But if the goal is to avoid the need for any of these, then I believe the proper way is to abandon the idea of building a monolithic kernel. IIUC, then you won't need to the CMake workaround, and you can use -Wl,--exclude-libs,ALL, so you won't need a visibility override. But I assume there are good reasons to have a monolithic kernel?

Can we at least only allow this being set when SECP256K1_DISABLE_SHARED is ON?

In both PRs, the override is currently just an #if not exposed anywhere in the build systems. So if you set this via -D... on the command line, you need to expect that you're on your own. But as I said, I believe this is a valid feature, and so there's no good reason not to expose it in CMake and configure (could be done in a follow-up PR). And these will be the right places to raise an error when the override is used in a shared build.

[real-or-random]

Sure, it's not conventional, but it gets the job done :)

Ok, I can update the PR tomorrow. :)

Force-pushed this update.

Here's another variant of that approach: real-or-random@2d92ac3 Instead of an override to force "hidden", this has an override to do nothing. Then the user can use -fvisibility to set the visibility. That's a somewhat uncommon use of -fvisibility, but this variant has the advantage (?) that the override won't break the build for shared libs. But I'm not convinced that it's better in the end.

[theuni]

Win32 needs this branch too, no? Otherwise it'll still require -fvisibility=hidden for static libs?

[theuni]

Actually, maybe #if defined(SECP256K1_FORCE_HIDDEN_VISIBILITY) could just go at the top, and keep with the #ifndef(SECP256K1_API) pattern below it?

[real-or-random]

Win32 needs this branch too, no? Otherwise it'll still require -fvisibility=hidden for static libs?

I don't think so (but I haven't tested this so far). As far as I understand, gcc defaults to hidden on Windows as soon as it sees an explicit __declspec(dllexport) annotation (and MSVC anyway exports only annotated symbols). At least this is what the libtool manual claims:

It should be noted that the GNU auto-export feature is turned off when an explicit __declspec(dllexport) is seen. The GNU tools do this to not make more symbols visible for projects that have already taken the trouble to decorate symbols.

https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html

Actually, maybe #if defined(SECP256K1_FORCE_HIDDEN_VISIBILITY) could just go at the top, and keep with the #ifndef(SECP256K1_API) pattern below it?

Not sure if I can follow. If you still think we'll need this for Windows, can you suggest/sketch a patch?

[theuni]

No worries.

[theuni]

ACK 42d3133

[real-or-random]

@TheCharlatan @hebasto If this is still needed for kernel, can you review this approach?

[fanquake]

cc @purpleKarrot

[purpleKarrot]

NACK

I find the deeply nested #ifdef code quite convoluted and hard to reason about. It can be much simplified if you first define a platform abstraction for how exporting/importing works:

#if defined(_WIN32) || defined(__CYGWIN__)
#  define SECP256K1_EXPORT __declspec(dllexport)
#  define SECP256K1_IMPORT __declspec(dllimport)
#else
#  define SECP256K1_EXPORT __attribute__ ((visibility ("default")))
#  define SECP256K1_IMPORT __attribute__ ((visibility ("default")))
#endif

And then use that abstraction for defining the API:

#if defined(SECP256K1_STATIC)
#  define SECP256K1_API
#elif defined(SECP256K1_BUILD)
#  define SECP256K1_API SECP256K1_EXPORT
#else
#  define SECP256K1_API SECP256K1_IMPORT
#endif

In CMake the two preprocessor definitions are set like this:

target_compile_definitions(libsecp256k1 PUBLIC
  $<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,STATIC_LIBRARY>:SECP256K1_STATIC>
)
set_target_properties(libsecp256k1 PROPERTIES
  DEFINE_SYMBOL SECP256K1_BUILD # if this is not set, it defaults to libsecp256k1_EXPORTS
)

Note that SECP256K1_STATIC is defined as PUBLIC. This means that it affects both the library itself, but also other targets that use the library. The generator expression evaluates to nothing when the library is build as a shared object.

SECP256K1_BUILD only affects the library itself. This could be achieved as well by setting a PRIVATE compile definition, but setting the DEFINE_SYMBOL has the advantage of overriding the builtin default of <target>_EXPORTS.

The result is that there are three modes that the header file can be interpreted in:

• Building a shared library: The API is exported.
• Using a shared library: The API is imported.
• Building / using a static library: The API is neither imported nor exported.

The advantage of that approach is that the information of how clients need to consume the library is tied to the library interface. There are no extra flags that clients need to set. It is impossible for them to get to a situation where the -D and -L flags mismatch. They just add a dependency to their target, and it will correctly import the API iff needed.

I don't see a reason why this approach shouldn't works for autotools just as well. All that needs to be done is add -DSECP256K1_STATIC to the .pc file when the package is built with static libraries.

[purpleKarrot]

To avoid name collisions when libraries form diamond of death dependencies, it might be good to provide a way for clients to define their own "namespace" (prefix) for public symbols. An example library that supports not one, but two approaches for this use case is https://gitlab.kitware.com/utils/kwsys. To understand how that is used, look at https://github.com/Kitware/CMake/blob/master/Source/kwsys/String.h.in for example.

[hebasto]

While testing this PR as part of an integration with Bitcoin Core, I've noticed some inconvenience in having to set the extra -DSECP256K1_FORCE_HIDDEN_VISIBILITY flag from the parent CMake-based project. Could this be handled in a slightly more developer-friendly way?

[purpleKarrot]

Could this be handled in a slightly more developer-friendly way?

This is exactly what I meant. Downstream users need to match the preprocessor define and the linked library. There are four possible combinations, and two of them cause problems. Usage requirements should be tied to the library.

[real-or-random]

Building / using a static library: The API is neither imported nor exported.

Well, the goal of this PR and of #1674 is precisely to enable the user to control visibility for static builds.

[real-or-random]

I don't see a reason why this approach shouldn't works for autotools just as well. All that needs to be done is add -DSECP256K1_STATIC to the .pc file when the package is built with static libraries.

The fact that the packager uses autotools does not imply that the consumer will use pkgconfig.

Our current logic attempts to make it possible to consume the library (at least on Unix-like) as a static library or as a dynamic library without the need to set any defines.

This is broadly useful for library consumers, independent of their build system and of the build system of the library.

The current code is based on https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html and https://gcc.gnu.org/wiki/Visibility, but has been refined since. I agree that the logic is convoluted, and it may be possible to improve this. But I think that's partly due to the fact that the build toolchains are convoluted, in particular if you want to support many platforms.

[real-or-random]

While testing this PR as part of an integration with Bitcoin Core, I've noticed some inconvenience in having to set the extra -DSECP256K1_FORCE_HIDDEN_VISIBILITY flag from the parent CMake-based project. Could this be handled in a slightly more developer-friendly way?

So this PR is based on the assumption that static library packagers/builders typically don't want hidden visibility. Moreover, we didn't set hidden in the past for static builds, so keeping this behavior is a bit more conservative because it won't break existing downstream builds that rely on this (if there are any, I don't know.) If Core wants different. It needs to set a flag, I think that's okay.

But perhaps the assumption is wrong? I'm not sure, and I think either way is somewhat sane. Consider some libxyz that uses libsecp256k1. Then it's probably okay for it not to export our public API. But also, there's nothing really wrong with re-exporting our public API. The code is there anyway, so why not make it available to the consumer of libxyz? The only problem that may occur is that libxyz also wants to use libsecp256k1, and it wants to use a different version. Then you'll get name collisions, and this is what @purpleKarrot mentioned above... But if you really run into this problem, then you still get a flag -DSECP256K1_FORCE_HIDDEN_VISIBILITY.

[purpleKarrot]

There is no valid use case of # define SECP256K1_API extern __attribute__ ((visibility("hidden"))).

On Windows, the situation is clear: When building a .dll, the public API needs to be exported, and when building against a .dll, the public API needs to be imported.

On other platforms that support visibility, there are three options:

1. You decorate the public API with __attribute__ ((visibility("default"))) and change the visibility preset to "hidden". This gets you the same behavior as on Windows.
2. You decorate the private API with __attribute__ ((visibility("hidden"))) and leave the visibility preset to "default". This is a valid approach when most of the functions are public and you want to save some typing.
3. You decorate all functions explicitly with either "default" or "hidden". In this case, it does not matter what the visibility preset is.

But when you use the above define and keep the compiler's default visibility preset, it could lead to a situation where you export all the private functions and none of the public ones. Further, if portability to Windows is important, then option 1 is the only possible approach anyway.

[purpleKarrot]

Our current logic attempts to make it possible to consume the library (at least on Unix-like) as a static library or as a dynamic library without the need to set any defines.

I see. On Windows this is impossible, because clients need to know whether they need to import or not. On Unix-like, this is not possible either, when clients want to change the visibility preset (aka opt into Windows behavior), which is exactly what is experienced in core, that a -D flag becomes necessary.

Maybe that goal should be dropped in favor of a both simpler and more portable approach?

[purpleKarrot]

Building / using a static library: The API is neither imported nor exported.

Well, the goal of this PR and of #1674 is precisely to enable the user to control visibility for static builds.

Let me rephrase: The API is neither dll-imported nor dll-exported.

The libtool manual states:

One common assumption is that if a DLL is being built (‘DLL_EXPORT’ is defined) then that DLL is going to consume any dependent libraries as DLLs.

The use case that we are talking about is to statically link secp256k1 into a library without exposing any of secp256k1's symbols. We don't want to dll-import libsecp's symbols (we want to use the static library) and we don't want to dll-export the symbols (we only want to define our own API).

[theuni]

Maybe that goal should be dropped in favor of a both simpler and more portable approach?

This part I agree with. secp defining symbols as hidden is convenient in many cases, but violates the principle of least surprise and makes this all harder to reason about.

Agree with @hebasto about this being annoying for downstreams. To be clear, I acked this PR because it solves the problem we (Core static kernel builders) are facing. I don't think it's the most elegant or comprehensive solution, but it does work.

[real-or-random]

Maybe that goal should be dropped in favor of a both simpler and more portable approach?

This part I agree with. secp defining symbols as hidden is convenient in many cases, but violates the principle of least surprise and makes this all harder to reason about.

I think this has two parts. I agree that setting hidden is strange. But I don't agree that we should throw away the entire logic and start from scratch like in #1695. The library and also this code is too mature to do that.

Agree with @hebasto about this being annoying for downstreams. To be clear, I acked this PR because it solves the problem we (Core static kernel builders) are facing. I don't think it's the most elegant or comprehensive solution, but it does work.

Yep, both this PR and your #1674 will need some manual -D. I don't see an alternative that is better in every regard. The approach in #1695 does not need an -D override, but it needs an explicit -DSECP256K1_STATIC in any case. And sure, the latter can be set automatically if both downstream uses CMake, but we also want to support autotools and manual builds.

See #1696 for yet another approach that we can hopefully all agree on.