Use explicit symbol visibility.

gmaxwell commented at 9:48 pm on September 21, 2015: contributor

The use of static makes this somewhat redundant currently, though if we later have multiple compilation units it will be needed.

This also sets the dllexport needed for shared libraries on win32.

gmaxwell commented at 9:49 pm on September 21, 2015: contributor

Fixes #305. See #312 for discussion (new pull req because of an administrative mistake on my part).

gmaxwell commented at 3:02 pm on September 22, 2015: contributor

@theuni care to RE: LGTM over here?

sipa commented at 6:02 pm on September 22, 2015: contributor

If I remove the SECP256K1_EXPORT from an API definition, everything keeps working. I would expect that since the visibility is default and not overridden anymore, external calls (from benchmarks) would fail?

sipa commented at 6:06 pm on September 22, 2015: contributor

Ah, I guess the benchmarks linking the library is not considered to be across the library boundary. objdump -x on the .so file shows that doing so makes the API call go from ‘g’ to ’l’ (global to local?).

This raises the question: how can we test this? Nothing will catch a missing SECP256K1_EXPORT line.

Other than that, Tested ACK.

sipa commented at 7:48 pm on September 22, 2015: contributor

Needs rebase.

theuni commented at 9:16 pm on September 22, 2015: contributor

@gmaxwell LGTM after rebase. @sipa nm -D file.so should show the symbols as “T” if they’re properly exported. The tests work regardless of the exports because they’re linked statically.

Off the top of my head, we could test by:

adding a build-only test which links to the shared lib using -Wl,--no-undefined. It could run as well, but we’d have to use LD_LIBRARY_PATH or some libtool tricks.
Add a test which dlopens the lib. Probably not a bad test to have anyway.
Other language binding (java/go/php/etc.) tests will test inherently.

gmaxwell commented at 9:19 pm on September 22, 2015: contributor

@sipa Merge this and my next PR will be changing all the benchmarks to not be -static which then tests this. As I mentioned, I can’t see any reason to -static the bench, as it works with static only builds even without it (thanks to libtool).

sipa commented at 10:01 pm on September 22, 2015: contributor

@gmaxwell Will merge after rebase :)

gmaxwell commented at 10:29 pm on September 22, 2015: contributor

OKAY, rebased.

gmaxwell cross-referenced this on Sep 23, 2015 from issue Add DLL exports. by evoskuil

gmaxwell commented at 8:51 pm on September 23, 2015: contributor

Added the win32 dllimport. It took me a while to determine what this actually did, most of the MSFT documentation is pretty uninformative. A useful description is here: https://msdn.microsoft.com/en-us/library/aa271769%28v=vs.60%29.aspx

sipa commented at 5:39 pm on September 24, 2015: contributor

@chfast @evoskuil Comments?

sipa commented at 5:59 pm on September 24, 2015: contributor

Ultra nit: SECP256K1_EXPORT may be interpreted as “try to export”, while its behaviour depends on whether we’re in the library or not. Suggest SECP256K1_API?

evoskuil commented at 6:17 pm on September 24, 2015: contributor

@sipa see https://github.com/bitcoin/secp256k1/pull/223#discussion_r39462741 for reasoning behind change from _API to _EXPORT

sipa commented at 6:56 pm on September 24, 2015: contributor

@evoskuil I’m confused. #223 uses _API and not _EXPORT. I think that _API a clearer name.

chfast commented at 7:53 pm on September 24, 2015: none

I’m for renaming SECP256K1_EXPORT to SECP256K1_API too. And also SECP256K1_BUILD to SECP256K1_EXPORTS as this name is a convention cmake is using for shared library builds (I mentioned about that in the other PR).

No support for static build (other than predefining empty SECP256K1_EXPORT)?

evoskuil commented at 8:24 pm on September 24, 2015: contributor

@sipa Sorry, I got confused between the names used in the two pull requests. My preference is SECP256K1_API, which was unchanged in https://github.com/bitcoin/secp256k1/pull/223. The change was from SECP256K1_DLL to SECP256K1_EXPORTS, because of @chfast comment….

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

Also, I agree with @chfast that there should be a SECP256K1_STATIC, which amounts to what is currently in https://github.com/bitcoin/secp256k1/pull/223…

 0# if defined _MSC_VER || defined __CYGWIN__
 1#  define SECP256K1_HELPER_LOCAL
 2#  define SECP256K1_HELPER_DLL_IMPORT __declspec(dllimport)
 3#  define SECP256K1_HELPER_DLL_EXPORT __declspec(dllexport)
 4# else
 5#  if __GNUC__ >= 4
 6#   define SECP256K1_HELPER_LOCAL      __attribute__ ((visibility ("hidden")))
 7#   define SECP256K1_HELPER_DLL_IMPORT __attribute__ ((visibility ("default")))
 8#   define SECP256K1_HELPER_DLL_EXPORT __attribute__ ((visibility ("default")))
 9#  else
10#   define SECP256K1_HELPER_LOCAL
11#   define SECP256K1_HELPER_DLL_IMPORT
12#   define SECP256K1_HELPER_DLL_EXPORT
13#  endif
14# endif
15
16# if defined SECP256K1_STATIC
17#  define SECP256K1_API SECP256K1_HELPER_LOCAL
18# elif defined SECP256K1_EXPORTS
19#  define SECP256K1_API SECP256K1_HELPER_DLL_EXPORT
20# else
21#  define SECP256K1_API SECP256K1_HELPER_DLL_IMPORT
22# endif

gmaxwell commented at 9:47 pm on September 24, 2015: contributor

No support for static build (other than predefining empty SECP256K1_EXPORT)?

Static builds are supported just fine (unless dllimport breaks them on windows, in which case I think we should drop dllimport). In fact they are currently the built by default and used by the benchmarks by default in the current configuration.

I currently disagree with the separate static define. It is messy, increases the already nearly untestably large ifdef combinitoric complexity, and unnecessary as far as I can tell.

Include public module headers when compiling modules.

Also fix the nullness requirements for schnorr nonce-pair generation.

4e64608082

gmaxwell commented at 10:07 pm on September 24, 2015: contributor

Changed the name to _API.

sipa commented at 10:12 pm on September 24, 2015: contributor

Sounds like dllimport may break static linking (or at least make it less efficient).

evoskuil commented at 10:43 pm on September 24, 2015: contributor

There are three sceanrios (actually four, but two can be collapsed), which requires more than one flag.

When building a DLL, one defines SECP256K1_EXPORTS (initially this was defined as SECP256K1_DLL) and gets dllexport.
When linking to a DLL, one defines nothing (and gets dllimport)
How does one build (or link) to a static lib?

The ifdef does get more complex, which is why the above pattern is used to keep it visually verifiable.

gmaxwell commented at 10:51 pm on September 24, 2015: contributor

evoskuil: When building the library the visibility annotations are set on the prototypes. This is mediated by the singular SECP256K1_BUILD macro which we also need for other reasons.

Then using the library they are not. On *nix with libtool the library isn’t even built twice for static vs dynamic: both outputs are created from the same intermediate result.

You haven’t explained why you are treating static specially. It is not special on unix (beyond the visibility policy being a no-op), is it actually special on win32 in some other way?

evoskuil commented at 11:26 pm on September 24, 2015: contributor

@gmaxwell I don’t see where SECP256K1_BUILD is defined, but I assume its definition implies that the library is being built (as DLL or static) as opposed to being linked (as DLL or static).

Based on that assumption and this construction…

0# if defined(_WIN32)
1#  ifdef SECP256K1_BUILD
2#   define SECP256K1_API __declspec(dllexport)
3#  else
4#   define SECP256K1_API __declspec(dllimport)
5#  endif
6...
7# endif

… how does _WIN32 static use of the library avoid __declspec(import|export)? I believe this was the point of @sipa ’s question.

sipa commented at 11:34 pm on September 24, 2015: contributor

SECP256K1_BUILD is being set by the build system.

Having a special case for static building is incompatible with the fact that the shared and static library are being built from the same .o.file.

evoskuil commented at 11:35 pm on September 24, 2015: contributor

Nevertheless, it is not possible to build or link statically for _WIN32 as it stands.

sipa commented at 11:51 pm on September 24, 2015: contributor

@evoskuil Does specifying dllexport break static linking?

If yes, you’re right, and we’ll need to compile the windows dll separate from the shared library.
If no, there is not really a problem.

evoskuil commented at 11:52 pm on September 24, 2015: contributor

Yes, and I’m pretty sure dllimport will as well. I’ll run a test in a bit just to make absolutely sure.

gmaxwell commented at 11:55 pm on September 24, 2015: contributor

Can you explain how (e.g. what error results?)

The original macro set I used (which doesn’t set dllimport) is the same as I’ve used in other projects running on hundreds of millions of windows devices (and most built with the microsoft toolchain of some vintage or another). I’m unclear on the mechanism that would cause it to fail, as the microsoft documentation is vague. It works in mingw32 for me at least.

gmaxwell commented at 11:58 pm on September 24, 2015: contributor

@sipa SECP256K1_BUILD is set by secp256k1.c. FWIW.

sipa commented at 0:00 am on September 25, 2015: contributor

From what I read, not setting dllimport is always safe, just slightly less efficient. The inefficiency is a single extra jump to a stub. Given that our fastest runtime APIs already take several microseconds, I think the effect is negligable.

If dllimport is not causing any problems… sure, but if setting it is a problem in static builds, it means the caller code needs to be aware of whether it’s linking libsecp256k1 statically or dynamically (not only the linrary code), which seems pretty ugly.

evoskuil commented at 0:12 am on September 25, 2015: contributor

The problem is with __declspec(dllimport). If this is defined on the API then static linking will produce

0warning C4273: '[function name]' : inconsistent dll linkage

for all functions so defined. More info here: https://msdn.microsoft.com/en-us/library/35bhkfb6.aspx

Given your objectives I’d recommend just not setting dllimport.

sipa commented at 0:18 am on September 25, 2015: contributor

So is the avoidance of a jump when calling the library worth:

Needing to know whether the library is being linked statically or not
Complicating the macros in our public header
Needing logic in the build system to compile the library separately for dynamic and static linking usage, with separate compilation flags?

evoskuil commented at 0:33 am on September 25, 2015: contributor

@sipa It is sort of ugly that the caller has to make this distinction, but it’s also extremely common in Windows builds. I try to minimize it to having to only define a symbol when linking statically, and doing nothing when linking dynamically. Most of the work I do is in C++, where this distinction is more significant.

I don’t think the first two issues you list are significant, but achieving your objective of a single compile seems to outweigh the small runtime hit.

sipa commented at 0:39 am on September 25, 2015: contributor

I don’t really care about the compile time. More about the fact that the build system needs to be aware of this. @cfields how hard would that be, you think?

sipa commented at 0:40 am on September 25, 2015: contributor

Oops, @theuni ^

Use explicit symbol visibility.

The use of static makes this somewhat redundant currently, though if
 we later have multiple compilation units it will be needed.

This also sets the dllexport needed for shared libraries on win32.

118cd8210f

gmaxwell commented at 5:47 am on September 25, 2015: contributor

dropped the import. It’s not just our own build system, or even alternative build-systems for our library that are my concern, but it’s the fact that any downstream users’ build-system would be impacted by the static/dynamic difference. I could see this being especially irritating for other software which is primarily developed on *nix, then mysteriously failing on windows.

in include/secp256k1.h: in 118cd8210f

118@@ -119,6 +119,20 @@ typedef int (*secp256k1_nonce_function)(
119 #  define SECP256K1_INLINE inline
120 # endif
121 
122+#ifndef SECP256K1_API
123+# if defined(_WIN32)
124+#  ifdef SECP256K1_BUILD

chfast commented at 2:11 pm on September 25, 2015:

Can you rename SECP256K1_BUILD to SECP256K1_EXPORTS? It is weird that SECP256K1_BUILD is defined for dynamic builds but not for static builds.

gmaxwell commented at 3:47 pm on September 25, 2015:

No. SECP256K1_BUILD does not differ for static vs dynamic build. There is no preprocessor differences between static and dynamic builds. This define is set during the build of the library itself, only during the build of library itself, and generally should not be touched by users.

sipa commented at 3:47 pm on September 25, 2015:

SECP256K1_BUILD is always set when building the library.

in include/secp256k1.h: in 118cd8210f

118@@ -119,6 +119,20 @@ typedef int (*secp256k1_nonce_function)(
119 #  define SECP256K1_INLINE inline
120 # endif
121 
122+#ifndef SECP256K1_API

chfast commented at 2:11 pm on September 25, 2015:

Why is that checked in the first place?

gmaxwell commented at 3:44 pm on September 25, 2015:

To avoid redefinition if it has been set already, e.g. by another header.

evoskuil commented at 6:46 pm on September 25, 2015: contributor

@gmaxwell I get that it’s an objective is to avoid the conditional compilation of a static and dynamic build, and to avoid exposing that distinction to the library consumer. But what I don’t understand is how this is actually accomplished.

When intending to load a DLL a project is initially linked against a static (stub) lib. Since the stub forwards calls to the DLL it is necessarily a different build than the (full) static lib. In VC the stub and full static lib have the same name and define the same symbols, but I don’t see how the full static lib can possibly be used as a stub and therefore how distinct builds can be avoided.

It makes sense that one can link statically against the stub lib, built with dllexport, but this doesn’t constitute a static link of the main lib, as the DLL would still be required. It’s not just a question of visibility. This is why the conditionality is always exposed to the library consumer in Windows. If you’ve got a way around this I’d like to understand so I can simplify some repos.

sipa commented at 6:54 pm on September 25, 2015: contributor

@evoskuil The static and dynamic library are not the same. But they are both constructed from the same object file. The C compiler doesn’t need to know how the compiled code will be used, only the linker that turns it either into a static or dynamic library.

Given that @gmaxwell’s header preamble works in Opus on Windows, here is my assumption:

Setting dllexport when building a static library has no effect. The function symbol is marked for exporting, but since no dll is being built, it is ignored.
Not setting dllimport when linking a dynamic library does not hurt, except by causing one extra indirection.

evoskuil commented at 7:10 pm on September 25, 2015: contributor

@sipa Are you saying that the build system produces a .dll with a stub .lib and a full static .lib as well?

sipa commented at 7:22 pm on September 25, 2015: contributor

@evoskuil I can’t test on Windows, but that’s what I assume, yes. In Linux it works that way approximately.

evoskuil commented at 7:24 pm on September 25, 2015: contributor

@sipa In order for it to work properly I believe it must be this way. But this of course means you cannot avoid exposing the conditionality to the consumer, even if you don’t define dllimport, as it is necessary to select the appropriate library.

sipa commented at 7:25 pm on September 25, 2015: contributor

@evoskuil Well obviously something in the build system needs to know what it is linking to. But the compiler doesn’t need to know. Not on the library side, not on user side.

Even better, the one building the library can choose whether to build the static version, the dynamic version, or both. And the user doesn’t even need to care what was produced - only find the library.

sipa commented at 7:28 pm on September 25, 2015: contributor

Anyway, back on track: is there are reason to think that the current pull request will not work?

evoskuil commented at 7:30 pm on September 25, 2015: contributor

@sipa As long as you are exposing the three build outputs I described above, there’s no problem. Sharing .obj files is not an issue as long as there are no compiler conditions that differ between static and dynamic builds, and avoiding dllimport seems like a reasonable trade-off in a C project.

sipa merged this on Sep 25, 2015

sipa closed this on Sep 25, 2015

sipa referenced this in commit 357f8cd8f5 on Sep 25, 2015

sipa commented at 7:36 pm on September 25, 2015: contributor

@evoskuil I believe this will work, but feel free to report Windows-related problems.

theuni commented at 9:33 pm on September 25, 2015: contributor

Heh, @evoskuil Is bringing up my original concern here (see https://github.com/bitcoin/secp256k1/pull/312#issuecomment-142094928). @sipa Fyi, libtool does build 2 separate objects with different sets of defines as necessary. DLL_EXPORT controls whether we’re building for a shared lib or not, so that symbols can be exported as necessary.

gmaxwell deleted the branch on Oct 28, 2015

real-or-random cross-referenced this on Apr 19, 2021 from issue mingw builds by real-or-random

Use explicit symbol visibility. #314