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

build: Improvements to symbol visibility logic on Windows #1362

Closed
wants to merge 5 commits into from
Closed

build: Improvements to symbol visibility logic on Windows #1362

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
85aecf4
build: Use dllexport also for Cygwin builds
real-or-random Jun 27, 2023
297c85a
build: Add #ifdefs for requesting import as DLL vs static explicitly
real-or-random Jun 27, 2023
d2117f0
build: Add extensive docs on visibility issues
real-or-random Jun 27, 2023
69918b8
build: Accept SECP256K1_DLL in addition to DLL_EXPORT in lib builds
real-or-random Jun 28, 2023
5267c8b
build: Use `SECP256K1_STATICLIB` macro instead of warning suppressions
hebasto Jun 13, 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
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_STATICLIB
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_STATICLIB
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_STATICLIB
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
2 changes: 1 addition & 1 deletion examples/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ target_link_libraries(example INTERFACE
$<$<PLATFORM_ID:Windows>:bcrypt>
)
if(NOT BUILD_SHARED_LIBS AND MSVC)
target_link_options(example INTERFACE /IGNORE:4217)
target_compile_definitions(example INTERFACE SECP256K1_STATICLIB)
endif()

add_executable(ecdsa_example ecdsa.c)
Expand Down
43 changes: 36 additions & 7 deletions include/secp256k1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,27 +133,56 @@ 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 */
#if defined(_WIN32)
# ifdef SECP256K1_BUILD
# ifdef DLL_EXPORT
/* Symbol visibility. */
#if defined(_WIN32) || defined(__CYGWIN__)
# if defined(SECP256K1_STATICLIB) && defined(SECP256K1_DLL)
# error "At most one of SECP256K1_STATICLIB and SECP256K1_DLL must be defined."
# endif
/* GCC for Windows (e.g., MinGW) and for Cygwin accept 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(SECP256K1_DLL) || defined(DLL_EXPORT)
/* Building libsecp256k1 as a DLL. (DLL_EXPORT is a libtool convention.) */
# define SECP256K1_API __declspec (dllexport)
# define SECP256K1_API_VAR extern __declspec (dllexport)
# endif
# elif defined _MSC_VER
# elif defined(SECP256K1_STATICLIB)
/* Linking against static libsecp256k1 requested explicitly. */
# define SECP256K1_API
# define SECP256K1_API_VAR extern
# elif defined(SECP256K1_DLL)
/* Linking against a libsecp256k1 DLL requested explicitly. */
# define SECP256K1_API __declspec (dllimport)
# define SECP256K1_API_VAR extern __declspec (dllimport)
# elif defined(_MSC_VER)
/* No method requested explicitly. The following works on MSVC for both
* static and dynamic linking, as long as if at least one function is
* imported (i.e., not only variables are imported), which should be the case
* for any meaningful program that uses the libsecp256k1 API. The drawback of
* the following is that it may provoke linker warnings LNK4217 and LNK4286.
* See "Windows DLLs" in the libtool manual. */
Comment on lines +166 to +167
Copy link
Member

Choose a reason for hiding this comment

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

Add a recommendation to use the SECP256K1_STATICLIB macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a sentence in an earlier version, but I removed it because it's probably clear by "No method requested explicitly." Let me bring it back.

# define SECP256K1_API
Copy link
Member

Choose a reason for hiding this comment

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

While touching this code, we can exploit one more optimization.

From Microsoft docs:

Using __declspec(dllimport) is optional on function declarations, but the compiler produces more efficient code if you use this keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, but we shouldn't touch it here in the # elif defined(_MSC_VER) branch.

The specific combination

#  define SECP256K1_API
#  define SECP256K1_API_VAR  extern __declspec (dllimport)

is precisely what always works for MSVC (if one is willing to accept the warning):

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.

https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html#Windows-DLLs

But in general, yes, the introduction of SECP256K1_DLL here will make it possible for the user to force __declspec(dllimport) on functions, which is faster according to the docs. We could add this to our linking jobs in autotools/CMake, though I doubt that it's measurable, given that API calls for us typically involve some expensive cryptographic operation anyway -- saving a few CPU instructions is probably not a big deal.

# define SECP256K1_API_VAR extern __declspec (dllimport)
# elif defined DLL_EXPORT
# elif defined(DLL_EXPORT)
Copy link
Member

@hebasto hebasto Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shoudn't it be OR'ed with defined(SECP256k1_DLL)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if SECP256K1 is defined, then we would have hit the #elif defined(SECP256K1_DLL) above.

An alternative option is to drop this "guess" based on EXPORT_DLL entirely. We took this from the libtool manual which justifies it as follows:

For older GNU tools and other proprietary tools there is no generic way to make it possible to consume either of the DLL or the static library without user intervention, the tools need to be told what is intended. One common assumption is that if a DLL is being built (‘DLL_EXPORT’ is defined) then that DLL is going to consume any dependent libraries as DLLs. If that assumption is made everywhere, it is possible to select how an end-user application is consuming libraries by adding a single flag ‘-DDLL_EXPORT’ when a DLL build is required. This is of course an all or nothing deal, either everything as DLLs or everything as static libraries.

(https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html#Windows-DLLs)

But with this PR, we get better methods for telling the tools what is intended. And I believe that "Explicit is better than implicit" and "In the face of ambiguity, refuse the temptation to guess.".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think? Should we remove this branch entirely?

Copy link
Member

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

Remove it. As a Libtool-only convention, using the DLL_EXPORT macro when consuming the library looks weird.

But with this PR, we get better methods for telling the tools what is intended.

Indeed.

/* No method requested explicitly and we're not on MSVC. We make an educated
* guess based on libtool's DLL_EXPORT convention: If the importing program
* is itself a DLL, then it is likely that it also wants to consume
* libsecp256k1 as a DLL. See "Windows DLLs" in the libtool manual. */
# define SECP256K1_API __declspec (dllimport)
# define SECP256K1_API_VAR extern __declspec (dllimport)
# endif
#endif
#ifndef SECP256K1_API
# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD)
/* Building libsecp256k1 on non-Windows using GCC or compatible. */
# define SECP256K1_API __attribute__ ((visibility ("default")))
# define SECP256K1_API_VAR extern __attribute__ ((visibility ("default")))
# else
/* All cases not captured above. */
# define SECP256K1_API
# define SECP256K1_API_VAR extern
# endif
Expand Down