build: Improvements to symbol visibility logic on Windows (attempt 3)

hebasto commented at 3:53 pm on June 29, 2023: member

Previous attempts:

#1346
#1362

The result is as follows:

Simple, concise and extensively documented code.
Explicitly documented use cases with no ambiguities.
No workarounds for linker warnings.
Solves one item in #1235.

hebasto cross-referenced this on Jun 29, 2023 from issue build: Improvements to symbol visibility logic on Windows by real-or-random

in include/secp256k1.h:157 in edec49c2f0 outdated

164+   * library. */
165+# elif !defined(SECP256K1_STATIC)
166+   /* Consuming libsecp256k1 as a DLL.
167+    * It does not depend on the Libtool's assumption that if a DLL is being built
168+    * (DLL_EXPORT is defined) then that DLL is going to consume any dependent libraries
169+    * as DLLs. See "Windows DLLs" in the Libtool manual. */

real-or-random commented at 3:57 pm on June 29, 2023:

Do we need these three comment lines if we anyway don’t follow that recommendation?

hebasto commented at 4:11 pm on June 29, 2023:

Removed.

in include/secp256k1.h:164 in edec49c2f0 outdated

173 #ifndef SECP256K1_API
174 # if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
175-#  define SECP256K1_API             __attribute__ ((visibility ("default")))
176-#  define SECP256K1_API_VAR  extern __attribute__ ((visibility ("default")))
177+   /* Building libsecp256k1 on non-Windows using GCC or compatible. */
178+#  define SECP256K1_API __attribute__ ((visibility ("default")))

real-or-random commented at 3:58 pm on June 29, 2023:

I think it will be slightly simpler to include extern in SECP256K1_API. Then we don’t need to add it for variables. (And it doesn’t hurt in case of functions.)

hebasto commented at 4:12 pm on June 29, 2023:

Updated.

hebasto force-pushed on Jun 29, 2023

in include/secp256k1.h:152 in 93ebfe88ca outdated

159-#  define SECP256K1_API_VAR  extern __declspec (dllimport)
160-# elif defined DLL_EXPORT
161-#  define SECP256K1_API             __declspec (dllimport)
162-#  define SECP256K1_API_VAR  extern __declspec (dllimport)
163+  /* The user must define SECP256K1_STATIC when consuming libsecp256k1 as a static
164+   * library. */

real-or-random commented at 4:00 pm on June 29, 2023:

0  /* The user must define SECP256K1_STATIC when consuming libsecp256k1 as a static
1   * library on Windows. */

hebasto commented at 4:12 pm on June 29, 2023:

Updated.

in include/secp256k1.h:145 in 93ebfe88ca outdated

146+   * exactly equivalent to) __attribute__ ((visibility("default"))), and so we
147+   * actually want __declspec even on GCC, see "Microsoft Windows Function
148+   * Attributes" in the GCC manual and the recommendations in
149+   * https://gcc.gnu.org/wiki/Visibility. */
150+# if defined(SECP256K1_BUILD)
151+#  if defined(DLL_EXPORT) || defined(SECP256k1_DLL_EXPORT)

real-or-random commented at 4:01 pm on June 29, 2023:

0#  if defined(DLL_EXPORT) || defined(SECP256K1_DLL_EXPORT)

capital “K” :) Also in the comment below.

hebasto commented at 4:14 pm on June 29, 2023:

Thanks! Fixed.

in include/secp256k1.h:147 in 93ebfe88ca outdated

148+   * Attributes" in the GCC manual and the recommendations in
149+   * https://gcc.gnu.org/wiki/Visibility. */
150+# if defined(SECP256K1_BUILD)
151+#  if defined(DLL_EXPORT) || defined(SECP256k1_DLL_EXPORT)
152+    /* Building libsecp256k1 as a DLL.
153+     * 1. If using Libtool, it defines DLL_EXPORT internally.

real-or-random commented at 4:01 pm on June 29, 2023:

0     * 1. If using Libtool, it defines DLL_EXPORT automatically.

nit: I think this is slightly clearer. “Internally” sounds like we don’t see the definition here.

hebasto commented at 4:15 pm on June 29, 2023:

Done.

real-or-random commented at 4:02 pm on June 29, 2023: contributor

ACK mod nits

real-or-random added the label documentation on Jun 29, 2023

real-or-random added the label build on Jun 29, 2023

hebasto force-pushed on Jun 29, 2023

hebasto commented at 4:11 pm on June 29, 2023: member

Thanks @real-or-random! Your comments have been addressed.

hebasto force-pushed on Jun 29, 2023

hebasto cross-referenced this on Jun 29, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto

in src/CMakeLists.txt:23 in 07b1037f47 outdated

19@@ -20,10 +20,9 @@ if(SECP256K1_ASM STREQUAL "arm32")
20   target_link_libraries(secp256k1_asm INTERFACE secp256k1_asm_arm)
21 endif()
22 
23-# Define our export symbol only for Win32 and only for shared libs.

theuni commented at 5:19 pm on June 29, 2023:

Why remove the “shared libs” comment? That’s a huge hint imo for behavior here that’s quite subtle.

hebasto commented at 5:44 pm on June 29, 2023:

Reworked.

in include/secp256k1.h:151 in 27b5d95bae outdated

140@@ -141,10 +141,10 @@ typedef int (*secp256k1_nonce_function)(
141 #   define SECP256K1_API            __declspec (dllexport)
142 #   define SECP256K1_API_VAR extern __declspec (dllexport)
143 #  endif
144-# elif defined _MSC_VER
145-#  define SECP256K1_API
146-#  define SECP256K1_API_VAR  extern __declspec (dllimport)
147-# elif defined DLL_EXPORT
148+  /* The user must define SECP256K1_STATIC when consuming libsecp256k1 as a static

theuni commented at 5:26 pm on June 29, 2023:

This isn’t really a must, right? Afaics nothing is changing except adding a new opt-in for dllimport for mingw?

hebasto commented at 5:50 pm on June 29, 2023:

It is. Just try (with the reverted change in Makefile.am):

0./configure --host=x86_64-w64-mingw32 --enable-examples && make

real-or-random commented at 8:03 am on June 30, 2023:

@theuni This also removes this branch:

0# elif defined _MSC_VER
1#  define SECP256K1_API
2#  define SECP256K1_API_VAR  extern __declspec (dllimport)

This was libtool’s hack for making the build work on MSVC with static and dynamic linkage and without explicit config by the user. But this comes at the expense of confusing linker warnings with static linkage and other potential issues:

With Microsoft tools you typically get away with always compiling the code such that variables are expected to be imported from a DLL and functions are expected to be found in a static library. The tools will then automatically import the function from a DLL if that is where they are found. If the variables are not imported from a DLL as expected, but are found in a static library that is otherwise pulled in by some function, the linker will issue a warning (LNK4217) that a locally defined symbol is imported, but it still works. In other words, this scheme will not work to only consume variables from a library. There is also a price connected to this liberal use of imports in that an extra indirection is introduced when you are consuming the static version of the library. That extra indirection is unavoidable when the DLL is consumed, but it is not needed when consuming the static library.

https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html#Windows-DLLs @hebasto and I now believe that it’s cleaner and simpler to require the user to set SECP256K1_STATIC explicitly when static linkage is desired. Such a macro seems to be common on Windows.

real-or-random approved

real-or-random commented at 5:27 pm on June 29, 2023: contributor

utACK 73a39f6cf84e1e54ea6b68ee586b94d6ded2ad80

in src/CMakeLists.txt:27 in 27b5d95bae outdated

23@@ -24,6 +24,7 @@ endif()
24 # This matches libtool's usage of DLL_EXPORT
25 if(WIN32)
26   set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL "DLL_EXPORT")
27+  target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATICLIB>)

theuni commented at 5:30 pm on June 29, 2023:

I assume this should be SECP256K1_STATIC ?

hebasto commented at 5:34 pm on June 29, 2023:

Oh, indeed!

hebasto commented at 5:44 pm on June 29, 2023:

Thank you @theuni! Fixed.

real-or-random commented at 8:05 am on June 30, 2023:

The Windows CMake task didn’t catch this mistake (https://cirrus-ci.com/task/6074245453709312) because it uses DLLs. Would it make sense to add a CI task that uses static linkage?

hebasto commented at 8:45 am on June 30, 2023:

The Windows CMake task didn’t catch this mistake (cirrus-ci.com/task/6074245453709312) because it uses DLLs. Would it make sense to add a CI task that uses static linkage?

The similar idea came to my head :) Implemented.

theuni commented at 5:32 pm on June 29, 2023: contributor

I haven’t followed the other attempts, but as far as I can see this doesn’t break anything or make me grumpy :)

Left some notes, including a real bug I believe.

hebasto force-pushed on Jun 29, 2023

hebasto commented at 5:43 pm on June 29, 2023: member

Addresses @theuni’s comments:

in CHANGELOG.md:18 in bdcd794ef6 outdated

13@@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
14    - Document `doc/ellswift.md` which explains the mathematical background of the scheme.
15    - The [paper](https://eprint.iacr.org/2022/759) on which the scheme is based.
16 
17+#### Changed
18+ - The user must define the `SECP256K1_STATIC` macro when consuming libsecp256k1 as a static library for their Windows builds.

real-or-random commented at 8:14 am on June 30, 2023:

nit:

0 - The user must now define the `SECP256K1_STATIC` macro before including `secp256k1.h` when consuming libsecp256k1 as a static library for their Windows builds.

Or stylistic variant that is easier to parse:

0 - When consuming libsecp256k1 as a static library on Windows, the user must now define the `SECP256K1_STATIC` macro before including `secp256k1.h`.

hebasto commented at 8:45 am on June 30, 2023:

Thanks! Updated.

real-or-random cross-referenced this on Jun 30, 2023 from issue Build not triggered by real-or-random

real-or-random commented at 8:21 am on June 30, 2023: contributor

(Hm, your force-push didn’t trigger a CI build for whatever reason. I triggered one manually now. Reported as https://github.com/cirruslabs/cirrus-ci-docs/issues/1183)

hebasto force-pushed on Jun 30, 2023

hebasto commented at 8:44 am on June 30, 2023: member

Updated bdcd794ef6d6a2e634b9bcc83b0b09fb4e6b5cd1 -> b688658d0a6449702448f2c0ff4f7f1ef9c2bfd1 (pr1367.05 -> pr1367.07, diff).

Recent @real-or-random’s comments have been addressed:

real-or-random approved

real-or-random commented at 3:16 pm on June 30, 2023: contributor

utACK b688658d0a6449702448f2c0ff4f7f1ef9c2bfd1

theuni commented at 3:34 pm on June 30, 2023: contributor

utACK b688658d0a6449702448f2c0ff4f7f1ef9c2bfd1.

It’s a lot to keep paged-in at once, so I can’t say I’m completely convinced, but generally lgtm and I appreciate the research and testing the two of you have been doing :)

Hopefully we can put this behind us now, heh.

hebasto force-pushed on Jul 3, 2023

hebasto commented at 8:25 am on July 3, 2023: member

Rebased due to the conflict with the recently merged #1368.

build: Introduce `SECP256K1_STATIC` macro for Windows users

It is a non-Libtool-specific way to explicitly specify the user's
intention to consume a static `libseck256k1`.

This change allows to get rid of MSVC linker warnings LNK4217 and
LNK4286. Also, it makes possible to merge the `SECP256K1_API` and
`SECP256K1_API_VAR` into one.

ae9db95cea

refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` 9f1b1904a3

build: Introduce `SECP256k1_DLL_EXPORT` macro

This change provides a way to build a shared library that is not tired
to the Libtool-specific `DLL_EXPORT` macro.

0196e8ade1

build: Add extensive docs on visibility issues 020bf69a44

ci: Add task for static library on Windows + CMake c6cd2b15a0

hebasto force-pushed on Jul 3, 2023

hebasto commented at 12:58 pm on July 3, 2023: member

Rebased due to the conflict with the recently merged #1369.

real-or-random approved

real-or-random commented at 1:41 pm on July 3, 2023: contributor

utACK c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe

real-or-random commented at 4:53 pm on July 3, 2023: contributor

Merging this, we can always adjust if those changes turn out to be trouble. Though I agree with @theuni, I hope this is the final word on this DLL stuff…

real-or-random merged this on Jul 3, 2023

real-or-random closed this on Jul 3, 2023

hebasto deleted the branch on Jul 4, 2023

hebasto cross-referenced this on Jul 13, 2023 from issue cmake: Set `ENVIRONMENT` property for examples on Windows by hebasto

fanquake referenced this in commit 56c05c5ec4 on Jul 17, 2023

fanquake referenced this in commit 7f4fac4a1f on Jul 18, 2023

fanquake cross-referenced this on Jul 18, 2023 from issue subtree: update libsecp256k1 to latest master by fanquake

fanquake referenced this in commit ff061fde18 on Jul 18, 2023

fanquake referenced this in commit 5080c9c25f on Jul 18, 2023

fanquake referenced this in commit 84c5416b03 on Jul 19, 2023

sidhujag referenced this in commit 381fa93615 on Jul 19, 2023

hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023

jonasnick cross-referenced this on Jul 27, 2023 from issue Upstream PRs 1357, 1345, 1358, 1368, 1369, 1323, 1364, 1370, 1367, 1344, 1371, 1363, 1340, 1366, 1313, 1378, 1298, 1382, 1383, 1384, 1386 by jonasnick

build: Improvements to symbol visibility logic on Windows (attempt 3) #1367