build: Add SECP256K1_API_VAR to fix importing variables from DLLs

real-or-random commented at 8:46 pm on February 6, 2023: contributor

… and more Windows fixes, please see the individual commits.

The fixed issues were discovered in #1198.

This fixes a build issue with MSVC. While MSVC imports *functions*
from DLLs automatically when building a consumer of the DLL, it does
not import *variables* automatically. In these cases, we need an
explicit __declspec(dllimport).

This commit simply changes our logic to what the libtool manual
suggests, which has a very comprehensive writeup on the topic. Note
that in particular, this solution is carefully designed not to break
static linking. However, as described in the libtool manual,
statically linking the library with MSVC will output warning LNK4217.
This is still the best solution overall, because the warning is
merely a cosmetic issue.

914276e4d2

examples: Extend sig examples by call that uses static context

Besides improving the examples, this makes sure that the examples
import a variable (instead of a function), namely the static context,
from the library. This is helpful when testing MSVC builds, because
the MSVC linker tends to be awkward when importing variables.

739c53b19a

build: Suppress stupid MSVC linker warning

... and use correct format to pass linker flags

9a5a611a21

ci: Shutdown wineserver whenever CI script exits

Before: CI times out when a wine task fails.
After:  Wine tasks exit properly when they fail.

e4330341bd

real-or-random marked this as ready for review on Feb 6, 2023

real-or-random commented at 8:51 pm on February 6, 2023: contributor

Note that users will have the same issue that they see linker warning LNK4217 when they try to use MSVC to link statically against libsecp256k1 (but better a warning than a failing build.)

I think we should note that somewhere and recommend ignoring that warning by passing -ignore:4217 (but where should we note that?). We could alternatively offer the possibility suppress the dllimport() stuff when a macro SECP256K1_STATIC_LINKAGE is defined, but I don’t think that’s better than just documenting the issue. (The macro will also need documentation.)

sipa commented at 9:00 pm on February 6, 2023: contributor

If all this potential SECP256K1_STATIC_LINKAGE would do is result in not emitting a warning at link time that can be more directly ignored too, I don’t think there is much need for it.

sipa commented at 9:38 pm on February 6, 2023: contributor

utACK e4330341bd648e93b60fe70c631e311a98bce549

in configure.ac:121 in e4330341bd

114@@ -115,6 +115,12 @@ AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [
115     if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then
116       SECP_TRY_APPEND_CFLAGS([-W2 -wd4146], $1) # Moderate warning level, disable warning C4146 "unary minus operator applied to unsigned type, result still unsigned"
117       SECP_TRY_APPEND_CFLAGS([-external:anglebrackets -external:W0], $1) # Suppress warnings from #include <...> files
118+      # We pass -ignore:4217 to the MSVC linker to suppress warning 4217 when
119+      # importing variables from a statically linked secp256k1.
120+      # (See the libtool manual, section "Windows DLLs" for background.)
121+      # Unfortunately, libtool tries to be too clever and strips "-Xlinker arg"

hebasto commented at 2:40 pm on February 21, 2023:

Libtool vs CMake – 0:1 :smile:

hebasto commented at 9:29 am on February 22, 2023:

This patch has been mirrored in the CMake-based build system.

hebasto approved

hebasto commented at 2:41 pm on February 21, 2023: member

ACK e4330341bd648e93b60fe70c631e311a98bce549, tested on Windows using CMake (which means that the 3rd commit is reviewed only, but not tested). FWIW, LNK4217 warnings have been indeed observed.

real-or-random merged this on Feb 21, 2023

real-or-random closed this on Feb 21, 2023

hebasto cross-referenced this on Feb 22, 2023 from issue build: Add CMake-based build system by hebasto

jonasnick cross-referenced this on Mar 7, 2023 from issue Update Changelog by jonasnick

hebasto referenced this in commit 7c0cc5d976 on Mar 7, 2023

dhruv referenced this in commit a5df79db12 on Mar 7, 2023

dhruv referenced this in commit 77b510d84c on Mar 7, 2023

sipa referenced this in commit 763079a3f1 on Mar 8, 2023

hebasto commented at 4:27 pm on March 11, 2023: member

Sorry for touching this PR after 2 weeks, but I need to clarify a few things, at least for myself.

After @theuni’s #1105, when cross-compiling a library for Windows, the SECP256K1_API macro is defined to __declspec(dllexport) because Libtool uses -DDLL_EXPORT.

After this PR, that is no longer the case.

Are we skipping __declspec(dllexport) for cross-compiled DLL altogether?

real-or-random commented at 5:21 pm on March 11, 2023: contributor

After @theuni’s #1105, when cross-compiling a library for Windows, the SECP256K1_API macro is defined to __declspec(dllexport) because Libtool uses -DDLL_EXPORT.

After this PR, that is no longer the case.

I can’t follow. What’s the exact combination of defines under which this PR has changed something and which bothers you?

hebasto commented at 5:53 pm on March 11, 2023: member

I can’t follow. What’s the exact combination of defines under which this PR has changed something and which bothers you?

Nothing special. Just ./configure --host=x86_64-w64-mingw32 --disable-static. The SECP256K1_API macro is defined as follows:

before this PR:

0#   define SECP256K1_API __declspec(dllexport)

after this PR:

0#  define SECP256K1_API             __attribute__ ((visibility ("default")))

real-or-random commented at 7:39 pm on March 11, 2023: contributor

Ok, I see.

Are we skipping __declspec(dllexport) for cross-compiled DLL altogether?

Yes, at least when using MinGW. (I assume you call it cross-compiling when GNU tools are used to build a DLL?) As I understand https://www.gnu.org/software/libtool/manual/libtool.html#Windows-DLLs, this should work. I guess MinGW counts as GNU tool. I simply took the code from there because they seem to have thought that through.

hebasto commented at 8:08 pm on March 11, 2023: member

Yes, at least when using MinGW. (I assume you call it cross-compiling when GNU tools are used to build a DLL?)

Correct, I’m using MinGW.

As I understand https://www.gnu.org/software/libtool/manual/libtool.html#Windows-DLLs, this should work.

Apparently, it works :)

div72 referenced this in commit 945b094575 on Mar 14, 2023

hebasto commented at 9:07 pm on March 16, 2023: member

Thanks to @theuni it is clear now that this PR did introduce a regression that listed in #1181:

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

I’ve verified that @theuni’s suggestion fixes this regression.

hebasto cross-referenced this on Mar 16, 2023 from issue Symbol visibility by real-or-random

sipa cross-referenced this on May 6, 2023 from issue ElligatorSwift + integrated x-only DH by sipa

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

hebasto cross-referenced this on Jun 13, 2023 from issue build: Use `SECP256K1_STATICLIB` macro instead of warning suppressions by hebasto

hebasto commented at 1:48 pm on June 28, 2023: member

Sorry for the late question :)

I’m curios about rationale of putting the extern keyword into the macro?

real-or-random commented at 2:14 pm on June 28, 2023: contributor

I’m curios about rationale of putting the extern keyword into the macro?

Ha, the simple answer is that I took this from the libtool manual and it just worked.

If you’re asking why we need extern at all, then this is a bit ugly in C: The rules are different for functions are variables: https://port70.net/~nsz/c/c11/n1570.html#6.2.2p5 In my own words: extern is always the default for functions, so you don’t have to write it. For variables, extern is only the default if they appear in “file scope”/top level.

So if some user code had #include <secp256k1.h> within a function (for whatever reason), our variable declarations wouldn’t work without extern. (See https://gcc.godbolt.org/z/9h1qqEjT7, also try enabling linking and look at the linker errors).

hebasto commented at 2:17 pm on June 28, 2023: member

I’m curios about rationale of putting the extern keyword into the macro?

Ha, the simple answer is that I took this from the libtool manual and it just worked.

If you’re asking why we need extern at all…

My point is if we keep extern in their place in the source code, then the SECP256K1_API_VAR macro becomes equal to SECP256K1_API.

real-or-random commented at 2:24 pm on June 28, 2023: contributor

I’m curios about rationale of putting the extern keyword into the macro?

Ha, the simple answer is that I took this from the libtool manual and it just worked. If you’re asking why we need extern at all…

My point is if we keep extern in their place in the source code, then the SECP256K1_API_VAR macro becomes equal to SECP256K1_API.

Oh, I see. In the # elif defined _MSC_VER case, it’s simply not true that SECP256K1_API_VAR = extern + SECP256K1_API. See also my response here: #1362 (review)

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 20, 2023 from issue Upstream PRs 1160, 1193, 1169, 1190, 1192, 1194, 1196, 1195, 1170, 1172, 1200, 1199, 1203, 1201, 1206, 1078, 1209, 979, 1212, 1218, 1217, 1221, 1222 by jonasnick

build: Add SECP256K1_API_VAR to fix importing variables from DLLs #1209