Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Jul 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae9db95
build: Introduce `SECP256K1_STATIC` macro for Windows users
hebasto Jun 30, 2023
9f1b190
refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API`
hebasto Jun 29, 2023
0196e8a
build: Introduce `SECP256k1_DLL_EXPORT` macro
hebasto Jun 29, 2023
020bf69
build: Add extensive docs on visibility issues
real-or-random Jun 27, 2023
c6cd2b1
ci: Add task for static library on Windows + CMake
hebasto Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .cirrus.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,12 +383,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
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.

Expand Down
6 changes: 3 additions & 3 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
6 changes: 0 additions & 6 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions examples/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
46 changes: 26 additions & 20 deletions include/secp256k1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@hebasto hebasto Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theuni This also removes this branch:

# elif defined _MSC_VER
#  define SECP256K1_API
#  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.

* 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

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
*
Expand Down
4 changes: 2 additions & 2 deletions include/secp256k1_ecdh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down
4 changes: 2 additions & 2 deletions include/secp256k1_ellswift.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
2 changes: 1 addition & 1 deletion include/secp256k1_schnorrsig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
6 changes: 3 additions & 3 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked.

# 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.
Expand Down