abi: Use dllexport for mingw builds #1295

1. 
   theuni commented at 4:29 pm on May 4, 2023: contributor

   Addresses the first part of #1181. See the discussion there for more context and history.

   After this, all that remains is a (platform-independent) exports checker for c-i. Or perhaps a linker script or .def file could be tricked into testing as a side-effect.

   This should fix mingw exports, specifically hiding the following: secp256k1_pre_g_128 secp256k1_pre_g secp256k1_ecmult_gen_prec_table

   This changes our visibility macros to look more like gcc’s recommendation.

   Edit: Note that we could further complicate this by supporting __attribute__ ((dllexport)) as well, though I didn’t bother as I’m not sure what compiler combo would accept that but not the bare dllexport syntax.

   Edit2: As the title implies, this affects this ABI and could affect downstream libs/apps in unintended ways (though it’s hard to imagine any real downside). Though because it’s win32 only, I’m imagining very little real-world impact at all.

2. 
   theuni commented at 4:30 pm on May 4, 2023: contributor

   Ping @hebasto
3. 
   theuni commented at 5:20 pm on May 4, 2023: contributor

   @real-or-random We should probably add a changelog entry for this (if you agree with it, and once we have a test for it):

   Symbol visibility is now believed to be handled properly on supported platforms and is now considered to be part of the ABI. Please report any improperly exported symbols as a bug.

4. 
   hebasto commented at 5:21 pm on May 4, 2023: member

   This should fix mingw exports,

   Concept ACK on this.

5. in include/secp256k1.h:136 in ac3c47bac3 outdated

   133@@ -134,7 +134,7 @@ typedef int (*secp256k1_nonce_function)(
   134 #endif
   135 
   136 /* Symbol visibility. See libtool manual, section "Windows DLLs". */
   

   ---

   ---

   

   real-or-random commented at 8:01 am on May 5, 2023:

   Can you update this comment to say that we slightly deviate from the suggestion in the libtool manual because the mingw linker apparently respects _declspec (dllexport) but not _attribute__ ((visibility ("default"))?

   ---

   

   theuni commented at 3:59 pm on May 5, 2023:

   @real-or-random Sure thing. Can you link me to that? I remember you posting a link in one of these issues but I’ve lost it now.

   ---

   

   real-or-random commented at 9:45 pm on May 5, 2023:

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

   ---

   

   theuni commented at 3:26 pm on May 8, 2023:

   Updated to include our reference as opposed to what we don’t use. This ok?
6. 
   real-or-random commented at 8:02 am on May 5, 2023: contributor

   Concept ACK
7. 
   real-or-random commented at 8:12 am on May 5, 2023: contributor

   As the title implies, this affects this ABI and could affect downstream libs/apps in unintended ways (though it’s hard to imagine any real downside). Though because it’s win32 only, I’m imagining very little real-world impact at all.

   Yeah, I wouldn’t consider it an ABI or an API change. IIUC the worst that can happen is that someone (wrongly) used one of these symbols in the past and now the dynamic linker can’t find it anymore.

8. 
   hebasto commented at 10:43 am on May 5, 2023: member

   I’ve verified the cross-compiled DLLs on Windows using the dumpbin /exports command. I can confirm that this PR indeed hides the mentioned symbols.
9. khademreza688 approved
10. 
    hebasto commented at 11:04 am on May 5, 2023: member

    When cross-compiling for Windows, in which cases is the SECP256K1_BUILD macro defined, and DLL_EXPORT is not defined?
11. 
    hebasto commented at 1:58 pm on May 5, 2023: member

    FWIW, I’ve rebased #1135 on top of this PR and integrated it into CI: https://github.com/hebasto/secp256k1/tree/pr1295/0505.test.2

    Only “{x86_64|i686} (MSVC): Windows (Debian stable, Wine…)” jobs fail.

12. 
    theuni commented at 2:29 pm on May 5, 2023: contributor

    When cross-compiling for Windows, in which cases is the SECP256K1_BUILD macro defined, and DLL_EXPORT is not defined?

    When building a static libsecp256k1.

13. 
    hebasto commented at 2:38 pm on May 5, 2023: member

    ACK ac3c47bac37d2d51092897d6a663cf3989de590c
14. 
    theuni commented at 2:42 pm on May 5, 2023: contributor

    FWIW, I’ve rebased #1135 on top of this PR and integrated it into CI: https://github.com/hebasto/secp256k1/tree/pr1295/0505.test.2

    Only “{x86_64|i686} (MSVC): Windows (Debian stable, Wine…)” jobs fail.

    It’s not clear to me what the error is here?

15. 
    hebasto commented at 2:44 pm on May 5, 2023: member

    FWIW, I’ve rebased #1135 on top of this PR and integrated it into CI: hebasto/secp256k1@pr1295/0505.test.2 Only “{x86_64|i686} (MSVC): Windows (Debian stable, Wine…)” jobs fail.

    It’s not clear to me what the error is here?

    Symbols are still exported:

    0python3 ./tools/symbol-check.py .libs/secp256k1-2.dll
    1.libs/secp256k1-2.dll: export of function ?_OptionsStorage@?1??__local_stdio_printf_options@@9@9 not expected
    2.libs/secp256k1-2.dll: export of function secp256k1_ecmult_gen_prec_table not expected
    3.libs/secp256k1-2.dll: export of function secp256k1_pre_g not expected
    4.libs/secp256k1-2.dll: export of function secp256k1_pre_g_128 not expected
    5.libs/secp256k1-2.dll: failed EXPORTED_FUNCTIONS
    

    But it is not mingw compiler.

16. abi: Use dllexport for mingw builds

    This should fix mingw exports, specifically hiding the following:
    secp256k1_pre_g_128
    secp256k1_pre_g
    secp256k1_ecmult_gen_prec_table
    
    This changes our visibility macros to look more like gcc's recommendation:
    https://gcc.gnu.org/wiki/Visibility#How_to_use_the_new_C.2B-.2B-_visibility_support

    bc7c8db179
17. theuni force-pushed on May 8, 2023
18. 
    hebasto commented at 3:28 pm on May 8, 2023: member

    re-ACK bc7c8db179a56cf7273f3c4c0decd10543a10521, only a comment has been adjusted since my recent review,
19. 
    theuni commented at 7:13 pm on June 23, 2023: contributor

    Ping @real-or-random. Anything left to do here?
20. real-or-random approved
21. 
    real-or-random commented at 8:37 am on June 24, 2023: contributor

    utACK bc7c8db179a56cf7273f3c4c0decd10543a10521
22. real-or-random merged this on Jun 24, 2023
23. real-or-random closed this on Jun 24, 2023

    ---

24. vmta referenced this in commit 8f03457eed on Jul 1, 2023
25. fanquake referenced this in commit 56c05c5ec4 on Jul 17, 2023
26. fanquake referenced this in commit ff061fde18 on Jul 18, 2023
27. hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023
28. jonasnick cross-referenced this on Jul 26, 2023 from issue Upstream PRs 1314, 1317, 1318, 1316, 1327, 1310, 1328, 1333, 1330, 1334, 1337, 1341, 1339, 1350, 1349, 1338, 1129, 1347, 1336, 1295, 1354, 1355, 1356 by jonasnick
29. delta1 referenced this in commit 3f32c20932 on Aug 8, 2023
30. delta1 referenced this in commit 31ac0c1081 on Aug 31, 2023
31. real-or-random added the label needs-changelog on Sep 4, 2023
32. real-or-random removed the label needs-changelog on Sep 4, 2023

Contributors
theuni hebasto real-or-random khademreza688