Use explicit symbol visibility. #314

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.

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

@theuni care to RE: LGTM over here?

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?

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.

Needs rebase.

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

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

@gmaxwell Will merge after rebase :)

OKAY, rebased.

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

@chfast @evoskuil Comments?

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?

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

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

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

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

# if defined _MSC_VER || defined __CYGWIN__
#  define SECP256K1_HELPER_LOCAL
#  define SECP256K1_HELPER_DLL_IMPORT __declspec(dllimport)
#  define SECP256K1_HELPER_DLL_EXPORT __declspec(dllexport)
# else
#  if __GNUC__ >= 4
#   define SECP256K1_HELPER_LOCAL      __attribute__ ((visibility ("hidden")))
#   define SECP256K1_HELPER_DLL_IMPORT __attribute__ ((visibility ("default")))
#   define SECP256K1_HELPER_DLL_EXPORT __attribute__ ((visibility ("default")))
#  else
#   define SECP256K1_HELPER_LOCAL
#   define SECP256K1_HELPER_DLL_IMPORT
#   define SECP256K1_HELPER_DLL_EXPORT
#  endif
# endif

# if defined SECP256K1_STATIC
#  define SECP256K1_API SECP256K1_HELPER_LOCAL
# elif defined SECP256K1_EXPORTS
#  define SECP256K1_API SECP256K1_HELPER_DLL_EXPORT
# else
#  define SECP256K1_API SECP256K1_HELPER_DLL_IMPORT
# endif

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.

Changed the name to _API.

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

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.

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?

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

# if defined(_WIN32)
#  ifdef SECP256K1_BUILD
#   define SECP256K1_API __declspec(dllexport)
#  else
#   define SECP256K1_API __declspec(dllimport)
#  endif
...
# endif

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

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.

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

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

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

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.

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

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.

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

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

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?

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

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?

Oops, @theuni ^

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.

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

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

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

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

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

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

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

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

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

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.