Add DLL exports. #223

pull evoskuil wants to merge 3 commits into bitcoin-core:master from evoskuil:dll-exports changing 5 files +62 −0

evoskuil commented at 10:34 PM on February 23, 2015: contributor

No description provided.
sipa commented at 10:34 AM on February 24, 2015: contributor

@theuni Comments?
gmaxwell added this to the milestone initial release on Aug 31, 2015
Add visibility hidden attribute to non-MSVC statics, rebase. 36daecf5c6

in include/secp256k1.h:None in d29e19bebe outdated

  53 | +#  endif
  54 | +# endif
  55 | +
  56 | +# if defined SECP256K1_STATIC
  57 | +#  define SECP256K1_API
  58 | +# elif defined SECP256K1_DLL

chfast commented at 2:16 PM on September 14, 2015:

CMake has a convention to add macro MyLibrary_EXPORTS when building a DLL. Is it possible to rename SECP256K1_DLL to SECP256K1_EXPORTS?

evoskuil commented at 6:05 PM on September 14, 2015:

Certainly, the symbol is otherwise arbitrary.

theuni commented at 10:45 PM on September 14, 2015:

Sounds good to me.

I think this should be __attribute__ ((visibility ("hidden"))) in the SECP256K1_STATIC case though, since it wouldn't need to be externally visible

evoskuil commented at 11:06 PM on September 14, 2015:

Good idea...

    define SECP256K1_HELPER_DLL_EXPORT __declspec(dllexport)
else
    if __GNUC__ >= 4
        define SECP256K1_HELPER_DLL_IMPORT __attribute__ ((visibility ("default")))
        define SECP256K1_HELPER_DLL_EXPORT __attribute__ ((visibility ("default")))
        define SECP256K1_HELPER_STATIC __attribute__ ((visibility ("hidden")))
    else
        define SECP256K1_HELPER_DLL_IMPORT
        define SECP256K1_HELPER_DLL_EXPORT
        define SECP256K1_HELPER_STATIC
    endif
endif

if defined SECP256K1_STATIC
    define SECP256K1_API SECP256K1_HELPER_STATIC
elif defined SECP256K1_DLL

in include/secp256k1.h:None in d29e19bebe outdated
```
  39 | @@ -40,6 +40,26 @@ extern "C" {
  40 |  #  define SECP256K1_ARG_NONNULL(_x)
  41 |  # endif
  42 |  
  43 | +# if defined _MSC_VER || defined __CYGWIN__
  44 | +#  define SECP256K1_HELPER_DLL_IMPORT __declspec(dllimport)
```
chfast commented at 10:49 PM on September 14, 2015:

dllimport is only useful in case of global variables and C++ classes, not in case of functions. Removing dllimport support would simplify the patch.

evoskuil commented at 11:32 PM on September 14, 2015:

I'm not sure this is the case. This is an old reference, but I have to assume it still holds:

You do not need to use __declspec(dllimport) for your code to compile correctly, but doing so allows the compiler to generate better code.

https://msdn.microsoft.com/en-us/library/aa271769(v=vs.60).aspx

chfast commented at 8:50 PM on September 16, 2015:

@evoskuil, you are right. I've checked the Visual Studio 2013 and it generates better code when dllimport is used (as described in the article).
evoskuil force-pushed on Sep 14, 2015
Rename static export helpers and public export symbol, export extern vars. 5c571f3f72
Export ecdh, recovery and schnorr APIs. 58fd107baa
in include/secp256k1.h:None in 58fd107baa
```
 152 | +# elif defined SECP256K1_EXPORTS
 153 | +#  define SECP256K1_API SECP256K1_HELPER_DLL_EXPORT
 154 | +# else
 155 | +#  define SECP256K1_API SECP256K1_HELPER_DLL_IMPORT
 156 | +# endif
 157 | +
```
evoskuil commented at 12:14 AM on September 15, 2015:

The above matches closely the GCC example (bottom of page). There is deviation in the public symbols SECP256K1_STATIC and SECP256K1_EXPORTS. The latter is modified for better CMake integration (see comment by @chfast). WRT the former I consider "static" more explicit than "local" and therefore better for publication.

in src/secp256k1.c:None in 58fd107baa

 253 | @@ -254,7 +254,10 @@ static int nonce_function_rfc6979(unsigned char *nonce32, const unsigned char *m
 254 |     return 1;
 255 |  }
 256 |  
 257 | +SECP256K1_API
 258 |  const secp256k1_nonce_function_t secp256k1_nonce_function_rfc6979 = nonce_function_rfc6979;
 259 | +
 260 | +SECP256K1_API
 261 |  const secp256k1_nonce_function_t secp256k1_nonce_function_default = nonce_function_rfc6979;

evoskuil commented at 12:16 AM on September 15, 2015:

I have not had a chance to verify these two lines, but according to documentation it appears correct for both VC and GCC.

theuni cross-referenced this on Sep 21, 2015 from issue Use explicit symbol visibility. by gmaxwell
gmaxwell commented at 12:12 AM on September 23, 2015: contributor

Please take a look at #314
evoskuil commented at 2:20 AM on September 23, 2015: contributor

I prefer the readability of the gcc example layout, and the use of dllimport, but not a big deal in either case.
chfast commented at 7:32 AM on September 23, 2015: none

@gmaxwell #314 looks like a subset of this one. With the except GCC visibility is not set to hidden by default here.
sipa commented at 1:49 PM on September 23, 2015: contributor

This defines SECP256K1_STATIC and SECP256K1_EXPORT, but they aren't being set anywhere?

If in #314, in the non-SECP256K1_BUILD branch for windows, declspec(dllimport) is set rather than nothing, do we get the best of both worlds?
sipa commented at 1:51 PM on September 23, 2015: contributor

I also think that this fails to set dllexport/import when compiling with MinGW?
evoskuil cross-referenced this on Sep 24, 2015 from issue Use explicit symbol visibility. by gmaxwell
sipa commented at 7:37 PM on September 25, 2015: contributor

Superseded by #314.
sipa closed this on Sep 25, 2015

Contributors

Milestone
stable release (1.0.0-rc.1)

Linked (view graph)

#312 Use explicit symbol visibility.#314 Use explicit symbol visibility.