Symbol visibility

[real-or-random]

In certain cases, we export too many symbols:

• With CFLAGS=-fvisibility=default, there's a few exported variables : secp256k1_ecmult_gen_prec_table, secp256k1_pre_g, secp256k1_pre_g_128
• on ARM, all the functions in the .s assembly files are visible but none should be visible (solved by Set ARM ASM symbol visibility to hidden #1242)
• Should we make all test functions static? (solved by Make all non-API functions (except main) static #1190)
• How to ensure correct visibility going forward (e.g., Add tools/symbol-check.py #1135)

[real-or-random]

• With CFLAGS=-fvisibility=default, there's a few exported variables : secp256k1_ecmult_gen_prec_table, secp256k1_pre_g, secp256k1_pre_g_128

The reason is that these three variables are not in the main translation unit, in which they're merely declared extern. (We cannot simply add static, because that would imply internal linkage which conflicts which the external linkage that we need due to the use of multiple compilation units.)

There's no generic C language feature to fix this. But we can fix this in both GCC and clang by any of the following (in descending order of my personal preference).

1. Add __attribute__ ((visibility ("hidden"))) to the extern declarations
2. Wrap the extern declarations in #pragma GCC visibility push(hidden) ... #pragma GCC visibility pop.
   1. We could in fact do this with the entire code base (excluding where we include foreign headers). 🤷 Not entirely sure if it doesn't create further problems...
3. Add -Wl,--exclude-libs,libsecp256k1_precomputed.a to LDFLAGS but that's kind of pointless to cover the case of default visibility, which typically occurs when the user invokes the compiler manually without even caring about -fvisibility.

In any case, -fvisibility=hidden would not be strictly necessary on the command line anymore (except for rather exotic compilers such as icc which understand it but not the attribute or the #pragma. This would then help manual builds of shared libraries.

[hebasto]

• on ARM, all the functions in the .s assembly files are visible but none should be visible

Solved by @theuni in #1242.

[theuni]

• With CFLAGS=-fvisibility=default, there's a few exported variables : secp256k1_ecmult_gen_prec_table, secp256k1_pre_g, secp256k1_pre_g_128

The reason is that these three variables are not in the main translation unit, in which they're merely declared extern. (We cannot simply add static, because that would imply internal linkage which conflicts which the external linkage that we need due to the use of multiple compilation units.)

There's no generic C language feature to fix this. But we can fix this in both GCC and clang by any of the following (in descending order of my personal preference).

1. Add `__attribute__ ((visibility ("hidden")))` to the `extern` declarations

2. Wrap the `extern` declarations in `#pragma GCC visibility push(hidden)` ... `#pragma GCC visibility pop`.

I'm afraid you're chasing after some ghosts here because as far as I can tell from my tests it's perfectly possible to use the export keyword without forcing default visibility despite what some of the docs say.

Notice that these exports aren't present in the linux shared libraries, only mingw.

The problem as I see it is that mingw is currently using __attribute__ ((visibility ("default"))) to mark its external symbols. However the linker machinery seems to only be activated by __declspec (dllexport). I had always kinda assumed they're equivalent, but minimal test-cases show that the differences are exactly our problems :)

For me, making that switch (along with #1242) solves all current export problems that I'm aware of.

diff --git a/include/secp256k1.h b/include/secp256k1.h
index 325f35eb..ed5bbc9a 100644
--- a/include/secp256k1.h
+++ b/include/secp256k1.h
@@ -148,3 +148,3 @@ typedef int (*secp256k1_nonce_function)(
 /* Symbol visibility. See libtool manual, section "Windows DLLs". */
-#if defined(_WIN32) && !defined(__GNUC__)
+#if defined(_WIN32)
 # ifdef SECP256K1_BUILD

(Note that I haven't tested for Darwin though, it's possible that export visibility is different there)

[hebasto]

@theuni

For me, making that switch (along with #1242) solves all current export problems that I'm aware of.

It does not change the output of x86_64-w64-mingw32-nm -g .libs/libsecp256k1-2.dll for me. What am I doing wrong?

[theuni]

Hmm, it definitely does for me. The difference is more clear with objdump -p.

Before:
objdump -p .libs/libsecp256k1-2.dll:

    ...
    ...
    [  60] secp256k1_xonly_pubkey_serialize
    [  61] secp256k1_xonly_pubkey_tweak_add
    [  62] secp256k1_xonly_pubkey_tweak_add_check

After the patch:
objdump -p .libs/libsecp256k1-2.dll:

    ...
    ...
    [  57] secp256k1_xonly_pubkey_serialize
    [  58] secp256k1_xonly_pubkey_tweak_add
    [  59] secp256k1_xonly_pubkey_tweak_add_check

Where the missing 3 are: secp256k1_pre_g_128, secp256k1_pre_g, and secp256k1_ecmult_gen_prec_table.

[hebasto]

Hmm, it definitely does for me. The difference is more clear with objdump -p.

Confirming that it works for me as well. And the -fvisibility=hidden C compiler flag becomes redundant.

[hebasto]

• With CFLAGS=-fvisibility=default, there's a few exported variables : secp256k1_ecmult_gen_prec_table, secp256k1_pre_g, secp256k1_pre_g_128

Introduced in #1209.

[real-or-random]

For me, making that switch (along with #1242) solves all current export problems that I'm aware of.

Nice. https://gcc.gnu.org/wiki/Visibility#How_to_use_the_new_C.2B-.2B-_visibility_support also suggests this. It would be nice to document this deviation from the libtool recommendation in the comment.

Interestingly the gcc man page says

with -fvisibility=hidden and "attribute ((visibility("default")))" instead of "__declspec(dllexport)" you
get almost identical semantics with identical syntax.

Yep, almost identical...

[theuni]

I had always kinda assumed they're equivalent

Heh, I guess this is why:

Interestingly the gcc man page says

with -fvisibility=hidden and "attribute ((visibility("default")))" instead of "__declspec(dllexport)" you
get almost identical semantics with identical syntax.

Yep, almost identical...

Yeah, it would seem those docs are wrong, given that we're seeing obvious differences in behavior.

It may be that default visibility is intended to be an alias for dllexport on windows, but if that's the case and this is a bug I doubt they'd change the behavior now. So I'd guess the best approach would be to submit an upstream PR to remove that line in the docs.

[theuni]

Nice. https://gcc.gnu.org/wiki/Visibility#How_to_use_the_new_C.2B-.2B-_visibility_support also suggests this. It would be nice to document this deviation from the libtool recommendation in the comment.

Will do. I'll PR this as a fix, but I'll wait until the current PR onslaught slows down a bit to avoid piling on :)

[hebasto]

But we can fix this in both GCC and clang by any of the following (in descending order of my personal preference).

1. Add __attribute__ ((visibility ("hidden"))) to the extern declarations

2. Wrap the extern declarations in #pragma GCC visibility push(hidden) ... #pragma GCC visibility pop.

FWIW, neither works for macOS's DYLIB.

Are we happy with a non-macOS solution?

UPD. It seems, there is a solution...