Merge bitcoin-core/secp256k1#1367: build: Improvements to symbol visibility logic on Windows (attempt 3)

c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe ci: Add task for static library on Windows + CMake (Hennadii Stepanov) 020bf69a44ba700624d09de0c18ceb867369d24e build: Add extensive docs on visibility issues (Tim Ruffing) 0196e8ade16e2b2d8efadac01d8520205553ee39 build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov) 9f1b1904a358e4ce7248c6542e8c7ac143ba0e3f refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov) ae9db95ceaa2605138fac9c237c640acea3f3bd6 build: Introduce `SECP256K1_STATIC` macro for Windows users (Hennadii Stepanov) Pull request description: Previous attempts: - https://github.com/bitcoin-core/secp256k1/pull/1346 - https://github.com/bitcoin-core/secp256k1/pull/1362 The result is as follows: 1. Simple, concise and extensively documented code. 2. Explicitly documented use cases with no ambiguities. 3. No workarounds for linker warnings. 4. Solves one item in https://github.com/bitcoin-core/secp256k1/issues/1235. ACKs for top commit: real-or-random: utACK c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e
2023-07-03 18:53:24 +02:00 · 2023-07-03 18:53:24 +02:00 · 9e6d1b0e9b
commit 9e6d1b0e9b
parent 0aacf64352 c6cd2b15a0
10 changed files with 46 additions and 41 deletions
--- a/.cirrus.yml
+++ b/.cirrus.yml
@ -384,12 +384,17 @@ task:
    # Ignore MSBuild warning MSB8029.
    # See: https://learn.microsoft.com/en-us/visualstudio/msbuild/errors/msb8029?view=vs-2022
    IgnoreWarnIntDirInTempDetected: 'true'
+  matrix:
+    - env:
+        BUILD_SHARED_LIBS: ON
+    - env:
+        BUILD_SHARED_LIBS: OFF
  git_show_script:
    # Print commit to allow reproducing the job outside of CI.
    - git show --no-patch
  configure_script:
    - '%x64_NATIVE_TOOLS%'
-    - cmake -E env CFLAGS="/WX" cmake -G "Visual Studio 17 2022" -A x64 -S . -B build -DSECP256K1_ENABLE_MODULE_RECOVERY=ON -DSECP256K1_BUILD_EXAMPLES=ON
+    - cmake -E env CFLAGS="/WX" cmake -A x64 -B build -DSECP256K1_ENABLE_MODULE_RECOVERY=ON -DSECP256K1_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=%BUILD_SHARED_LIBS%
  build_script:
    - '%x64_NATIVE_TOOLS%'
    - cmake --build build --config RelWithDebInfo -- -property:UseMultiToolTask=true;CL_MPcount=5
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Document `doc/ellswift.md` which explains the mathematical background of the scheme.
   - The [paper](https://eprint.iacr.org/2022/759) on which the scheme is based.

+#### Changed
+ - When consuming libsecp256k1 as a static library on Windows, the user must now define the `SECP256K1_STATIC` macro before including `secp256k1.h`.
+
 ## [0.3.2] - 2023-05-13
 We strongly recommend updating to 0.3.2 if you use or plan to use GCC >=13 to compile libsecp256k1. When in doubt, check the GCC version using `gcc -v`.

--- a/Makefile.am
+++ b/Makefile.am
@ -153,7 +153,7 @@ endif
 if USE_EXAMPLES
 noinst_PROGRAMS += ecdsa_example
 ecdsa_example_SOURCES = examples/ecdsa.c
-ecdsa_example_CPPFLAGS = -I$(top_srcdir)/include
+ecdsa_example_CPPFLAGS = -I$(top_srcdir)/include -DSECP256K1_STATIC
 ecdsa_example_LDADD = libsecp256k1.la
 ecdsa_example_LDFLAGS = -static
 if BUILD_WINDOWS
@ -163,7 +163,7 @@ TESTS += ecdsa_example
 if ENABLE_MODULE_ECDH
 noinst_PROGRAMS += ecdh_example
 ecdh_example_SOURCES = examples/ecdh.c
-ecdh_example_CPPFLAGS = -I$(top_srcdir)/include
+ecdh_example_CPPFLAGS = -I$(top_srcdir)/include -DSECP256K1_STATIC
 ecdh_example_LDADD = libsecp256k1.la
 ecdh_example_LDFLAGS = -static
 if BUILD_WINDOWS
@ -174,7 +174,7 @@ endif
 if ENABLE_MODULE_SCHNORRSIG
 noinst_PROGRAMS += schnorr_example
 schnorr_example_SOURCES = examples/schnorr.c
-schnorr_example_CPPFLAGS = -I$(top_srcdir)/include
+schnorr_example_CPPFLAGS = -I$(top_srcdir)/include -DSECP256K1_STATIC
 schnorr_example_LDADD = libsecp256k1.la
 schnorr_example_LDFLAGS = -static
 if BUILD_WINDOWS
--- a/configure.ac
+++ b/configure.ac
@ -127,12 +127,6 @@ AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [
      SECP_TRY_APPEND_CFLAGS([-wd4267], $1) # Disable warning C4267 "'var' : conversion from 'size_t' to 'type', possible loss of data".
      # Eliminate deprecation warnings for the older, less secure functions.
      CPPFLAGS="-D_CRT_SECURE_NO_WARNINGS $CPPFLAGS"
-      # We pass -ignore:4217 to the MSVC linker to suppress warning 4217 when
-      # importing variables from a statically linked secp256k1.
-      # (See the libtool manual, section "Windows DLLs" for background.)
-      # Unfortunately, libtool tries to be too clever and strips "-Xlinker arg"
-      # into "arg", so this will be " -Xlinker -ignore:4217" after stripping.
-      LDFLAGS="-Xlinker -Xlinker -Xlinker -ignore:4217 $LDFLAGS" 
    fi
 ])
 SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS)
--- a/examples/CMakeLists.txt
+++ b/examples/CMakeLists.txt
@ -6,9 +6,6 @@ target_link_libraries(example INTERFACE
  secp256k1
  $<$<PLATFORM_ID:Windows>:bcrypt>
 )
-if(NOT BUILD_SHARED_LIBS AND MSVC)
-  target_link_options(example INTERFACE /IGNORE:4217)
-endif()

 add_executable(ecdsa_example ecdsa.c)
 target_link_libraries(ecdsa_example example)
--- a/include/secp256k1.h
+++ b/include/secp256k1.h
@ -133,29 +133,35 @@ typedef int (*secp256k1_nonce_function)(
 # define SECP256K1_NO_BUILD
 #endif

-/* Symbol visibility. See https://gcc.gnu.org/wiki/Visibility */
-/* DLL_EXPORT is defined internally for shared builds */
+/* Symbol visibility. */
 #if defined(_WIN32)
-# ifdef SECP256K1_BUILD
-#  ifdef DLL_EXPORT
-#   define SECP256K1_API            __declspec (dllexport)
-#   define SECP256K1_API_VAR extern __declspec (dllexport)
+  /* GCC for Windows (e.g., MinGW) accepts the __declspec syntax
+   * for MSVC compatibility. A __declspec declaration implies (but is not
+   * exactly equivalent to) __attribute__ ((visibility("default"))), and so we
+   * actually want __declspec even on GCC, see "Microsoft Windows Function
+   * Attributes" in the GCC manual and the recommendations in
+   * https://gcc.gnu.org/wiki/Visibility. */
+# if defined(SECP256K1_BUILD)
+#  if defined(DLL_EXPORT) || defined(SECP256K1_DLL_EXPORT)
+    /* Building libsecp256k1 as a DLL.
+     * 1. If using Libtool, it defines DLL_EXPORT automatically.
+     * 2. In other cases, SECP256K1_DLL_EXPORT must be defined. */
+#   define SECP256K1_API extern __declspec (dllexport)
 #  endif
-# elif defined _MSC_VER
-#  define SECP256K1_API
-#  define SECP256K1_API_VAR  extern __declspec (dllimport)
-# elif defined DLL_EXPORT
-#  define SECP256K1_API             __declspec (dllimport)
-#  define SECP256K1_API_VAR  extern __declspec (dllimport)
+  /* The user must define SECP256K1_STATIC when consuming libsecp256k1 as a static
+   * library on Windows. */
+# elif !defined(SECP256K1_STATIC)
+   /* Consuming libsecp256k1 as a DLL. */
+#  define SECP256K1_API extern __declspec (dllimport)
 # endif
 #endif
 #ifndef SECP256K1_API
 # if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
-#  define SECP256K1_API             __attribute__ ((visibility ("default")))
-#  define SECP256K1_API_VAR  extern __attribute__ ((visibility ("default")))
+   /* Building libsecp256k1 on non-Windows using GCC or compatible. */
+#  define SECP256K1_API extern __attribute__ ((visibility ("default")))
 # else
-#  define SECP256K1_API
-#  define SECP256K1_API_VAR  extern
+   /* All cases not captured above. */
+#  define SECP256K1_API extern
 # endif
 #endif

@ -227,10 +233,10 @@ typedef int (*secp256k1_nonce_function)(
 *
 *  It is highly recommended to call secp256k1_selftest before using this context.
 */
-SECP256K1_API_VAR const secp256k1_context *secp256k1_context_static;
+SECP256K1_API const secp256k1_context *secp256k1_context_static;

 /** Deprecated alias for secp256k1_context_static. */
-SECP256K1_API_VAR const secp256k1_context *secp256k1_context_no_precomp
+SECP256K1_API const secp256k1_context *secp256k1_context_no_precomp
 SECP256K1_DEPRECATED("Use secp256k1_context_static instead");

 /** Perform basic self tests (to be used in conjunction with secp256k1_context_static)
@ -627,10 +633,10 @@ SECP256K1_API int secp256k1_ecdsa_signature_normalize(
 * If a data pointer is passed, it is assumed to be a pointer to 32 bytes of
 * extra entropy.
 */
-SECP256K1_API_VAR const secp256k1_nonce_function secp256k1_nonce_function_rfc6979;
+SECP256K1_API const secp256k1_nonce_function secp256k1_nonce_function_rfc6979;

 /** A default safe nonce generation function (currently equal to secp256k1_nonce_function_rfc6979). */
-SECP256K1_API_VAR const secp256k1_nonce_function secp256k1_nonce_function_default;
+SECP256K1_API const secp256k1_nonce_function secp256k1_nonce_function_default;

 /** Create an ECDSA signature.
 *
--- a/include/secp256k1_ecdh.h
+++ b/include/secp256k1_ecdh.h
@ -27,11 +27,11 @@ typedef int (*secp256k1_ecdh_hash_function)(

 /** An implementation of SHA256 hash function that applies to compressed public key.
 * Populates the output parameter with 32 bytes. */
-SECP256K1_API_VAR const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_sha256;
+SECP256K1_API const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_sha256;

 /** A default ECDH hash function (currently equal to secp256k1_ecdh_hash_function_sha256).
 * Populates the output parameter with 32 bytes. */
-SECP256K1_API_VAR const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default;
+SECP256K1_API const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default;

 /** Compute an EC Diffie-Hellman secret in constant time
 *
--- a/include/secp256k1_ellswift.h
+++ b/include/secp256k1_ellswift.h
@ -72,7 +72,7 @@ typedef int (*secp256k1_ellswift_xdh_hash_function)(
 /** An implementation of an secp256k1_ellswift_xdh_hash_function which uses
 *  SHA256(prefix64 || ell_a64 || ell_b64 || x32), where prefix64 is the 64-byte
 *  array pointed to by data. */
-SECP256K1_API_VAR const secp256k1_ellswift_xdh_hash_function secp256k1_ellswift_xdh_hash_function_prefix;
+SECP256K1_API const secp256k1_ellswift_xdh_hash_function secp256k1_ellswift_xdh_hash_function_prefix;

 /** An implementation of an secp256k1_ellswift_xdh_hash_function compatible with
 *  BIP324. It returns H_tag(ell_a64 || ell_b64 || x32), where H_tag is the
@ -80,7 +80,7 @@ SECP256K1_API_VAR const secp256k1_ellswift_xdh_hash_function secp256k1_ellswift_
 *  to secp256k1_ellswift_xdh_hash_function_prefix with prefix64 set to
 *  SHA256("bip324_ellswift_xonly_ecdh")||SHA256("bip324_ellswift_xonly_ecdh").
 *  The data argument is ignored. */
-SECP256K1_API_VAR const secp256k1_ellswift_xdh_hash_function secp256k1_ellswift_xdh_hash_function_bip324;
+SECP256K1_API const secp256k1_ellswift_xdh_hash_function secp256k1_ellswift_xdh_hash_function_bip324;

 /** Construct a 64-byte ElligatorSwift encoding of a given pubkey.
 *
--- a/include/secp256k1_schnorrsig.h
+++ b/include/secp256k1_schnorrsig.h
@ -61,7 +61,7 @@ typedef int (*secp256k1_nonce_function_hardened)(
 *  Therefore, to create BIP-340 compliant signatures, algo must be set to
 *  "BIP0340/nonce" and algolen to 13.
 */
-SECP256K1_API_VAR const secp256k1_nonce_function_hardened secp256k1_nonce_function_bip340;
+SECP256K1_API const secp256k1_nonce_function_hardened secp256k1_nonce_function_bip340;

 /** Data structure that contains additional arguments for schnorrsig_sign_custom.
 *
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@ -20,10 +20,10 @@ if(SECP256K1_ASM STREQUAL "arm32")
  target_link_libraries(secp256k1_asm INTERFACE secp256k1_asm_arm)
 endif()

-# Define our export symbol only for Win32 and only for shared libs.
-# This matches libtool's usage of DLL_EXPORT
 if(WIN32)
-  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL "DLL_EXPORT")
+  # Define our export symbol only for shared libs.
+  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL_EXPORT)
+  target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATIC>)
 endif()

 # Object libs don't know if they're being built for a shared or static lib.