Don’t export symbols in static libraries #1105

1. 
   theuni commented at 8:19 pm on May 4, 2022: contributor

   For context, Bitcoin Core has recently merged libbitcoin-kernel, a small library that intends to eventually minimally encompass Core’s validation engine. This kernel lib includes a static libsecp256k1. Without this change, because libsecp256k1.a ends up with exported symbols, we end up with libsecp256k1 symbols exported by our libbitcoin-kernel library (which causes unrelated problems not worth getting into here).

   libtool takes care of building both object versions, and it automatically builds objects for shared libs with -DDLL_EXPORT. We just need to opt-in to its functionality.

   I can’t imagine this having any negative impact on any current statically-linking applications, if anything they’ll just be a tiny bit smaller because they can now strip unused symbols.

2. abi: Don't export symbols in static Windows libraries

   libtool takes care of building both object versions, we just need to pick the
   right one to export symbols.

   6f6cab9989
3. theuni cross-referenced this on May 4, 2022 from issue `libbitcoinkernel`: Building `mingw-w64` dll's broken by dongcarl
4. 
   theuni commented at 9:29 pm on May 4, 2022: contributor

   In case it’s not obvious because a git grep DLL_EXPORT will come up otherwise empty…

   It’s a libtool convention. make V=1 shows libtool appending it:

   libtool: compile: x86_64-w64-mingw32-gcc -DHAVE_CONFIG_H -I. -I./src -I./include -I./src -I/home/cory/dev/bitcoin2/depends/x86_64-w64-mingw32/include/ -O2 -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wcast-align=strict -fvisibility=hidden -pipe -O2 -MT src/libsecp256k1_la-secp256k1.lo -MD -MP -MF src/.deps/libsecp256k1_la-secp256k1.Tpo -c src/secp256k1.c -DDLL_EXPORT -DPIC -o src/.libs/libsecp256k1_la-secp256k1.o

5. theuni renamed this:
   Don't export symbols in static Windows libraries
   Don't export symbols in static libraries
   on May 4, 2022
6. 
   theuni commented at 10:34 pm on May 4, 2022: contributor

   ~It turns out the same thing was happening for non-windows libraries as well, but I wasn’t seeing it because behavior was masked by an unrelated linker flag.~

   ~I’ve gone ahead and added a commit to fixup the non-windows paths as well. Title has been updated accordingly.~

   ~I can squash the commits if they make more sense that way.~

   Re-scoping to Windows only for this PR.

7. 
   sipa commented at 10:46 pm on May 4, 2022: contributor

   You have angered the CI gods.
8. 
   theuni commented at 11:06 pm on May 4, 2022: contributor

   You have angered the CI gods.

   Yeah, this might take some exploring. I pushed ~a fix~ some nonsense to ~de-grumpify~ confuse the gods by addressing totally the wrong thing.

   DLL_EXPORT is a Windows-only convention.

   Converting to a draft while I poke around.

9. theuni marked this as a draft on May 4, 2022
10. theuni force-pushed on May 4, 2022
11. 
    real-or-random commented at 7:38 am on May 5, 2022: contributor

    Not sure what other changes made compilation on CI fail but Concept ACK 6f6cab9989a4d3f4a28e3cdbfacc4e3e1e55c843. This should be entirely harmless.
12. 
    fanquake commented at 8:37 am on May 5, 2022: member

    Concept ACK
13. 
    hebasto commented at 8:46 am on May 5, 2022: member

    Concept ACK.
14. 
    theuni commented at 3:58 pm on May 5, 2022: contributor

    Not sure what other changes made compilation on CI fail but Concept ACK 6f6cab9. This should be entirely harmless.

    Thanks for having a look. Agreed the first commit (the one that’s there now) is correct and I’ll leave the non-Windows side alone for now.

    I may follow-up with a PR for an escape-hatch define for non-Windows static libs if we need one for Core, but only if it turns out to actually be necessary.

    Sorry for the noise, marking as ready for review again.

15. theuni marked this as ready for review on May 5, 2022
16. real-or-random approved
17. 
    real-or-random commented at 4:02 pm on May 5, 2022: contributor

    utACK 6f6cab9989a4d3f4a28e3cdbfacc4e3e1e55c843
18. 
    sipa commented at 4:07 pm on May 17, 2022: contributor

    utACK 6f6cab9989a4d3f4a28e3cdbfacc4e3e1e55c843
19. 
    laanwj commented at 6:58 pm on May 18, 2022: member

    This makes sense (also for non-windows platforms, but i guess we’ll get to that later). utACK 6f6cab9989a4d3f4a28e3cdbfacc4e3e1e55c843
20. real-or-random merged this on May 19, 2022
21. real-or-random closed this on May 19, 2022

    ---

22. fanquake referenced this in commit ef62dad39c on May 30, 2022
23. fanquake referenced this in commit 910bca8285 on May 30, 2022
24. fanquake cross-referenced this on May 30, 2022 from issue Consolidate Windows ASLR workarounds for upstream secp256k1 changes by fanquake
25. fanquake referenced this in commit c41bfd1070 on Jun 11, 2022
26. MarcoFalke referenced this in commit b91055ea55 on Jun 13, 2022
27. sidhujag referenced this in commit 3704ef4fcf on Jun 15, 2022
28. patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022
29. patricklodder referenced this in commit 03002a9013 on Jul 28, 2022
30. janus referenced this in commit 8a9469f3f4 on Aug 4, 2022
31. hebasto cross-referenced this on Mar 11, 2023 from issue build: Add SECP256K1_API_VAR to fix importing variables from DLLs by real-or-random
32. str4d referenced this in commit ca00f27013 on Apr 21, 2023
33. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
34. vmta referenced this in commit 8f03457eed on Jul 1, 2023
35. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115 by jonasnick
36. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115 by jonasnick
37. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115, 1116, 1120, 1122, 1121, 1128, 1131, 1144, 1150, 1146 by jonasnick

Contributors
theuni sipa real-or-random fanquake hebasto laanwj