build: Add SECP256K1_FORCE_HIDDEN

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

Alternative to #1674.

real-or-random added the label build on Jun 3, 2025

real-or-random added the label refactor/smell on Jun 3, 2025

real-or-random force-pushed on Jun 3, 2025

in include/secp256k1.h:160 in 5f8dbfefce outdated

155+  * a static library. (You want to ./configure with --disable-shared if using
156+  * Autotools.)
157+  *
158+  * While visibility is a concept that applies only to shared libraries, setting
159+  * visibility will still make a difference when building a static library: the
160+  * visibility settings will be stored in the static library, solely for the

hebasto commented at 11:31 am on June 3, 2025:

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

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

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 commented at 11:32 am on June 3, 2025: contributor

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 commented at 3:56 pm on June 3, 2025: contributor

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 commented at 6:57 pm on June 3, 2025: contributor

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:

0#ifndef SECP256K1_API
1# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
2#  if defined(SECP256K1_FORCE_HIDDEN_VISIBILITY)
3#   define SECP256K1_API extern __attribute__ ((visibility ("hidden")))
4#  else
5#   define SECP256K1_API extern __attribute__ ((visibility ("default")))
6#  endif
7# endif
8#endif

What do you think?

theuni commented at 7:10 pm on June 3, 2025: contributor

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:

0#ifndef SECP256K1_API
1# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
2#  if defined(SECP256K1_FORCE_HIDDEN_VISIBILITY)
3#   define SECP256K1_API extern __attribute__ ((visibility ("hidden")))
4#  else
5#   define SECP256K1_API extern __attribute__ ((visibility ("default")))
6#  endif
7# endif
8#endif

What do you think?

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

real-or-random commented at 7:33 pm on June 3, 2025: contributor

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 commented at 9:19 pm on June 3, 2025: none

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 commented at 5:53 am on June 4, 2025: contributor

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.

build: Add SECP256K1_FORCE_HIDDEN_VISIBILITY

The name should make it clear that this is intended for ELF platforms
only.

42d3133421

real-or-random force-pushed on Jun 4, 2025

real-or-random commented at 6:15 am on June 4, 2025: contributor

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: https://github.com/real-or-random/secp256k1/commit/2d92ac3de4e3a51d62ab25aa828bf6305bd3bbb6 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.

in include/secp256k1.h:173 in 42d3133421

175+     * setting visibility will still make a difference when building a static
176+     * library: the visibility settings will be stored in the static library,
177+     * solely for the potential case that the static library will be linked into
178+     * a shared library. In that case, the stored visibility settings will
179+     * resurface and be honored for the shared library. */
180+#   define SECP256K1_API extern __attribute__ ((visibility("hidden")))

theuni commented at 3:53 pm on June 4, 2025:

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

theuni commented at 3:56 pm on June 4, 2025:

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 commented at 7:45 am on June 5, 2025:

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 commented at 6:02 pm on June 11, 2025:

No worries.

theuni approved

theuni commented at 6:04 pm on June 11, 2025: contributor

ACK 42d3133421500c73a5df0e9ea95d9254d0761196

build: Add SECP256K1_FORCE_HIDDEN_VISIBILITY #1677