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

c6cd2b1 ci: Add task for static library on Windows + CMake (Hennadii Stepanov)
020bf69 build: Add extensive docs on visibility issues (Tim Ruffing)
0196e8a build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov)
9f1b190 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov)
ae9db95 build: Introduce `SECP256K1_STATIC` macro for Windows users (Hennadii Stepanov)

Pull request description:

  Previous attempts:
  - #1346
  - #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 #1235.

ACKs for top commit:
  real-or-random:
    utACK c6cd2b1

Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e

Author: real-or-random

.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

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`.
 

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

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)

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)

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.
  *

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
  *

include/secp256k1_ellswift.h

@@ -72,15 +72,15 @@ 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
  *  BIP340 tagged hash function with tag "bip324_ellswift_xonly_ecdh". Equivalent
  *  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.
  *

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.
  *

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.