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