Symbol visibility #1181

issue real-or-random openend this issue on December 19, 2022
  1. real-or-random commented at 4:21 pm on December 19, 2022: contributor

    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 #1242)
    • Should we make all test functions static? (solved by #1190)
    • How to ensure correct visibility going forward (e.g., #1135)
  2. real-or-random commented at 6:00 pm on December 21, 2022: contributor
    • 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). :shrug: 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.

  3. real-or-random added the label build on Jan 5, 2023
  4. hebasto cross-referenced this on Mar 13, 2023 from issue Set ARM ASM symbol visibility to `hidden` by hebasto
  5. hebasto commented at 9:16 am on March 15, 2023: member
    • on ARM, all the functions in the .s assembly files are visible but none should be visible

    Solved by @theuni in #1242.

  6. theuni commented at 6:18 pm on March 16, 2023: contributor
    • 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.

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

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

  7. hebasto commented at 8:09 pm on March 16, 2023: member

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

  8. theuni commented at 8:25 pm on March 16, 2023: contributor

    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.

  9. hebasto commented at 8:57 pm on March 16, 2023: member

    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.

  10. hebasto cross-referenced this on Mar 16, 2023 from issue build: Add SECP256K1_API_VAR to fix importing variables from DLLs by real-or-random
  11. hebasto commented at 9:08 pm on March 16, 2023: member
    • 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.

  12. real-or-random commented at 7:51 am on March 17, 2023: contributor

    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…

  13. theuni commented at 3:36 pm on March 17, 2023: contributor

    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.

  14. theuni commented at 3:38 pm on March 17, 2023: contributor

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

  15. real-or-random referenced this in commit 464a9115b4 on Mar 26, 2023
  16. theuni cross-referenced this on May 4, 2023 from issue abi: Use dllexport for mingw builds by theuni
  17. real-or-random referenced this in commit 926dd3e962 on Jun 24, 2023
  18. hebasto commented at 6:29 pm on June 24, 2023: member

    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…

  19. hebasto cross-referenced this on Jun 26, 2023 from issue Fix symbol visibility issues, add test for it by hebasto

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 08:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me