build: Improvements to symbol visibility logic on Windows

real-or-random commented at 3:01 pm on June 27, 2023: contributor

This is a somewhat more elaborate alternative to #1346.

As I said there:

Many hours of researching and experimenting went into that piece of code, so I’ll probably follow up with a PR that adds extensive comments.

build: Use dllexport also for Cygwin builds

As recommended by https://gcc.gnu.org/wiki/Visibility

85aecf463c

real-or-random added the label documentation on Jun 27, 2023

real-or-random added the label build on Jun 27, 2023

build: Add #ifdefs for requesting import as DLL vs static explicitly 297c85ac06

real-or-random force-pushed on Jun 28, 2023

real-or-random commented at 10:21 am on June 28, 2023: contributor

Ok, ready for review (if CI is happy).

This PR also addresses this item in #1235 now:

[ ] Consider changing DLL_EXPORT TO SECP256k1_DLL_EXPORT (https://github.com/bitcoin-core/secp256k1/pull/1113#discussion_r1126797102)

I’ve chosen to use the SECP256K1_DLL macro instead. The same macro is also used for requesting that one wants to consume libsecp256k1 as a DLL, i.e., when !defined(SECP256K_BUILD). I think this is convenient for the user. If a user builds a DLL, then it’s likely that the user also intends to consume the DLL in a program. (This is just for convenience. In the end, building the library and consuming the library are two entirely different build processes, so the reusing the macro name is never ambiguous on a technical level.)

real-or-random marked this as ready for review on Jun 28, 2023

real-or-random commented at 10:23 am on June 28, 2023: contributor

cc @theuni

hebasto commented at 10:30 am on June 28, 2023: member

Draft because I still need to incorporate the cmake/autotools changes from #1346.

Outdated?

hebasto cross-referenced this on Jun 28, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto

hebasto commented at 10:35 am on June 28, 2023: member

Out of scope of this PR, but shouldn’t CI has a Cygwin build as well?

build: Add extensive docs on visibility issues d2117f08b2

build: Accept SECP256K1_DLL in addition to DLL_EXPORT in lib builds

Addresses one item in #1235.

69918b823f

build: Use `SECP256K1_STATICLIB` macro instead of warning suppressions

This makes uses of the freshly introduced `SECP256K1_STATICLIB` macro
instead of ignoring MSVC linker warnings LNK4217 and LNK4286.

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

5267c8b5ad

real-or-random force-pushed on Jun 28, 2023

real-or-random commented at 10:37 am on June 28, 2023: contributor

Draft because I still need to incorporate the cmake/autotools changes from #1346.

Outdated?

Yes, I removed it from the initial comment.

Out of scope of this PR, but shouldn’t CI has a Cygwin build as well?

In general, I guess more testing never hurts. Is Cygwin still in wide use?

hebasto commented at 10:43 am on June 28, 2023: member

Out of scope of this PR, but shouldn’t CI has a Cygwin build as well?

In general, I guess more testing never hurts. Is Cygwin still in wide use?

I don’t have any particular numbers, but it seems WSL undermined Cygwin’s purpose :)

Anyway, if code written for a platform that platform should be tested. If it not feasible/reasonable to test a platform, no need to maintain platform-specific code, no?

real-or-random commented at 12:11 pm on June 28, 2023: contributor

Ok, yeah, the reason I included the Cygwin commit is that checking defined(__CYGWIN__) is suggested in https://gcc.gnu.org/wiki/Visibility (and Cygwin does not define _WIN32).

Anyway, if code written for a platform that platform should be tested.

I think that’s a good rule of thumb, but I also think that in this case maintaining half a line of code is okay as a best-effort thing, even if we don’t test it. In particular, because it’s just build system code. (If we used different instructions on Cygwin, then we would need a test for sure.)

in include/secp256k1.h:170 in 5267c8b5ad

172+    * the following is that it may provoke linker warnings LNK4217 and LNK4286.
173+    * See "Windows DLLs" in the libtool manual. */
174 #  define SECP256K1_API
175 #  define SECP256K1_API_VAR  extern __declspec (dllimport)
176-# elif defined DLL_EXPORT
177+# elif defined(DLL_EXPORT)

hebasto commented at 12:17 pm on June 28, 2023:

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

real-or-random commented at 12:46 pm on June 28, 2023:

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

real-or-random commented at 6:44 am on June 29, 2023:

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

hebasto commented at 7:37 am on June 29, 2023:

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.

in include/secp256k1.h:167 in 5267c8b5ad

168+   /* No method requested explicitly. The following works on MSVC for both
169+    * static and dynamic linking, as long as if at least one function is
170+    * imported (i.e., not only variables are imported), which should be the case
171+    * for any meaningful program that uses the libsecp256k1 API. The drawback of
172+    * the following is that it may provoke linker warnings LNK4217 and LNK4286.
173+    * See "Windows DLLs" in the libtool manual. */

hebasto commented at 12:18 pm on June 28, 2023:

Add a recommendation to use the SECP256K1_STATICLIB macro?

real-or-random commented at 12:47 pm on June 28, 2023:

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.

hebasto commented at 12:21 pm on June 28, 2023: member

Concept ACK.

[ ] Consider changing DLL_EXPORT TO SECP256k1_DLL_EXPORT (#1113 (comment))

Does it make sense to replace s/DLL_EXPORT/SECP256k1_DLL/ in the src/CMakeLists.txt?

real-or-random commented at 12:48 pm on June 28, 2023: contributor

Does it make sense to replace s/DLL_EXPORT/SECP256k1_DLL/ in the src/CMakeLists.txt?

Good point, will do.

in include/secp256k1.h:168 in 5267c8b5ad

169+    * static and dynamic linking, as long as if at least one function is
170+    * imported (i.e., not only variables are imported), which should be the case
171+    * for any meaningful program that uses the libsecp256k1 API. The drawback of
172+    * the following is that it may provoke linker warnings LNK4217 and LNK4286.
173+    * See "Windows DLLs" in the libtool manual. */
174 #  define SECP256K1_API

hebasto commented at 1:14 pm on June 28, 2023:

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.

real-or-random commented at 1:52 pm on June 28, 2023:

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

The specific combination

0#  define SECP256K1_API
1#  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.

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

real-or-random cross-referenced this on Jun 28, 2023 from issue build: Add SECP256K1_API_VAR to fix importing variables from DLLs by real-or-random

hebasto commented at 9:15 pm on June 28, 2023: member

Approach ACK 5267c8b5ad2b1fb3879074ac6a5b01a250945685.

While testing a static library using MSVC, the following warning has been noticed:

0LINK : warning LNK4217: symbol 'secp256k1_ellswift_xdh_hash_function_bip324' defined in 'libsecp256k1.lib(secp256k1.obj)' is imported by 'bench.obj' in function 'bench_ellswift_xdh' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build\src\bench.vcxproj]

which is a new one after merging #1129.

To tackle with it, suggesting the following diff (+ #1362 (comment)):

 0diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
 1index a314c17..e2ea473 100644
 2--- a/examples/CMakeLists.txt
 3+++ b/examples/CMakeLists.txt
 4@@ -6,9 +6,6 @@ target_link_libraries(example INTERFACE
 5   secp256k1
 6   $<$<PLATFORM_ID:Windows>:bcrypt>
 7 )
 8-if(NOT BUILD_SHARED_LIBS AND MSVC)
 9-  target_compile_definitions(example INTERFACE SECP256K1_STATICLIB)
10-endif()
11 
12 add_executable(ecdsa_example ecdsa.c)
13 target_link_libraries(ecdsa_example example)
14diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
15index 0bba199..4e62f61 100644
16--- a/src/CMakeLists.txt
17+++ b/src/CMakeLists.txt
18@@ -20,10 +20,9 @@ if(SECP256K1_ASM STREQUAL "arm32")
19   target_link_libraries(secp256k1_asm INTERFACE secp256k1_asm_arm)
20 endif()
21 
22-# Define our export symbol only for Win32 and only for shared libs.
23-# This matches libtool's usage of DLL_EXPORT
24 if(WIN32)
25-  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL "DLL_EXPORT")
26+  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL)
27+  target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATICLIB>)
28 endif()
29 
30 # Object libs don't know if they're being built for a shared or static lib.

hebasto commented at 8:03 am on June 29, 2023: member

I’m still thinking about user’s definitions during the consuming of the library.

Now, at the master branch, none of them are required or expected from the user. The _MSC_VER is defined by the compiler, the DLL_EXPORT by the Libtool.

This PR (5267c8b5ad2b1fb3879074ac6a5b01a250945685) introduces two optional macros: SECP256K1_STATICLIB and SECP256K1_DLL which are mutually exclusive. Then we have three code paths to handle.

Another approach is to make the SECP256K1_STATICLIB macro obligatory when consuming the static library. Not defining it should be considered as consuming the shared library. In this approach the elif defined(_MSC_VER) branch might be dropped altogether. And two macros SECP256K1_API and SECP256K1_API_VAR can be merged into one.

What drawbacks did I miss here?

real-or-random commented at 8:21 am on June 29, 2023: contributor

Doesn’t the following set SECP256K1_DLL even if we’re building a static lib?

0 if(WIN32)
1  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL)
2  [...]
3 endif()

And the current code on master does the same despite the comment saying “only for shared libs”. I assume having dllexport just doesn’t hurt in this case? Or am I getting this wrong?

hebasto commented at 8:24 am on June 29, 2023: member

Doesn’t the following set SECP256K1_DLL even if we’re building a static lib?
0 if(WIN32)
1  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL)
2  [...]
3 endif()
And the current code on master does the same despite the comment saying “only for shared libs”. I assume having dllexport just doesn’t hurt in this case? Or am I getting this wrong?

From CMake docs:

DEFINE_SYMBOL sets the name of the preprocessor symbol defined when compiling sources in a shared library.

real-or-random commented at 8:45 am on June 29, 2023: contributor

Another approach is to make the SECP256K1_STATICLIB macro obligatory when consuming the static library. Not defining it should be considered as consuming the shared library. In this approach the elif defined(_MSC_VER) branch might be dropped altogether. And two macros SECP256K1_API and SECP256K1_API_VAR can be merged into one.

What drawbacks did I miss here?

That’s indeed simpler. And I don’t think you missed any drawbacks. The only drawback is that MSVC users are required to define SECP256K1_STATICLIB when consuming a static lib. In other words, the advantage of the current approach is that it “just works” on MSVC. I think that’s neat, but on the other hand, I tend to agree: it was never truly clean due to the linker warnings (which are hard to debug for the user etc…) and I argued above that explicit is better than implicit.

I wonder how other libraries are doing this?

real-or-random marked this as a draft on Jun 29, 2023

hebasto commented at 9:06 am on June 29, 2023: member

I wonder how other libraries are doing this?

CMake-based projects might exploit the GenerateExportHeader module and use the ${LIB}_STATIC_DEFINE, for instance: https://github.com/google/benchmark/issues/1372#issuecomment-1068986946.

hebasto commented at 9:30 am on June 29, 2023: member

… the elif defined(_MSC_VER) branch might be dropped altogether.

Here are two branches to prove this concept:

And I believe that “Explicit is better than implicit” and “In the face of ambiguity, refuse the temptation to guess.”.

“Simple is better than complex.” :)

real-or-random commented at 9:30 am on June 29, 2023: contributor

I wonder how other libraries are doing this?

By the way, grep -10 dllimport -r /usr/include on a production system is good to get a feeling for this…. Most libraries have some macro that the user needs to set, and only very few follow the libtool recommendation.

Also, grep -10 dllimport -r /usr/include | grep STATIC shows that suffix _STATIC is more common than _STATICLIB. (https://gitlab.kitware.com/cmake/cmake/-/issues/23195#note_1139808 agrees with this.)

I’ll rework the PR based on these observations, but probably not this week. I’d also be happy to pass it back to you @hebasto if you’re willing to work on this.

hebasto commented at 9:33 am on June 29, 2023: member

I’d also be happy to pass it back to you @hebasto if you’re willing to work on this.

I’ll take it :)

hebasto cross-referenced this on Jun 29, 2023 from issue build: Improvements to symbol visibility logic on Windows (attempt 3) by hebasto

hebasto commented at 3:53 pm on June 29, 2023: member

I’d also be happy to pass it back to you @hebasto if you’re willing to work on this.

I’ll take it :)

Done in #1367.

real-or-random closed this on Jun 29, 2023

real-or-random referenced this in commit 9e6d1b0e9b on Jul 3, 2023

build: Improvements to symbol visibility logic on Windows #1362