build: Improvements to symbol visibility logic on Windows

[real-or-random]

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.

[real-or-random]

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 (#1113 (comment))

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]

cc @theuni

[hebasto]

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

Outdated?

[hebasto]

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

[real-or-random]

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]

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]

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

[hebasto]

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?

[hebasto]

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

[real-or-random]

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]

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

[hebasto]

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.

[hebasto]

Add a recommendation to use the SECP256K1_STATICLIB macro?

[real-or-random]

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.

[real-or-random]

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

Good point, will do.

[hebasto]

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]

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.

[hebasto]

Approach ACK 5267c8b.

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

LINK : 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)):

diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
index a314c17..e2ea473 100644
--- 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_compile_definitions(example INTERFACE SECP256K1_STATICLIB)
-endif()
 
 add_executable(ecdsa_example ecdsa.c)
 target_link_libraries(ecdsa_example example)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 0bba199..4e62f61 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -20,10 +20,9 @@ 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")
+  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL)
+  target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATICLIB>)
 endif()
 
 # Object libs don't know if they're being built for a shared or static lib.

[hebasto]

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 (5267c8b) 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]

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

 if(WIN32)
  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL)
  [...]
 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]

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

 if(WIN32)
  set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL)
  [...]
 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]

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?

[hebasto]

I wonder how other libraries are doing this?

CMake-based projects might exploit the GenerateExportHeader module and use the ${LIB}_STATIC_DEFINE, for instance: google/benchmark#1372 (comment).

[hebasto]

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

Here are two branches to prove this concept:

• https://github.com/hebasto/secp256k1/tree/master.shared.4 -- CI MSVC task
• https://github.com/hebasto/secp256k1/tree/master.static.4 -- CI MSVC task

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]

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]

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]

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.