Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use explicit symbol visibility. #312

Closed
wants to merge 2 commits into from
Closed

Use explicit symbol visibility. #312

wants to merge 2 commits into from

Conversation

gmaxwell
Copy link
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.

Also fix the nullness requirements for schnorr nonce-pair generation.
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
Copy link
Contributor Author

Fixes #305

@sipa
Copy link
Contributor

sipa commented Sep 21, 2015

Just to get this straight: -fvisibiblity changes the visibility across library boundaries, while static changes the visibility across module boundaries?

@sipa
Copy link
Contributor

sipa commented Sep 21, 2015

@theuni Comments?

@theuni
Copy link
Contributor

theuni commented Sep 21, 2015

Looks good to me. This replaces #223, but I like this approach better.

Only thing I'm iffy about is the win32+static case, where we won't want to set dllexport. libtool automatically sets -DDLL_EXPORT when building for a dll, I think we could take advantage of that like:

#  if defined(SECP256K1_BUILD) && defined(DLL_EXPORT)
#   define SECP256K1_EXPORT __declspec(dllexport)
#  else

@gmaxwell
Copy link
Contributor Author

@sipa Yes. static, as a side-effect, makes things invisible across a library boundary. But it does so by making it invisible across module boundaries. Right now, since the library is a single C file, static is sufficient. But win32 wants explicit visibility for libraries in all cases (either via a symbol file, or via the dll annotations this also adds), and we might want multiple modules in the future.

@theuni Probably the most interesting/challenges cases for DLL building are when people are using an alternative build system, though I suppose they could just set DLL_EXPORT too. What does setting dllexport on a win32 static do? I don't think I've ever seen any complaints from setting it there. If it's harmless-- might as well set it.

@theuni
Copy link
Contributor

theuni commented Sep 21, 2015

@gmaxwell presumably it would export libsecp256k1 functions from resulting binaries and helper libs.

@theuni
Copy link
Contributor

theuni commented Sep 21, 2015

@gmaxwell poking around with mingw, I can't manage to produce that result, so disregard the comment above. We can revisit if it ever surfaces.

@theuni
Copy link
Contributor

theuni commented Sep 21, 2015

whoops, s/WIN32/_WIN32/.

WIN32 isn't picked up by my mingw install.

@gmaxwell
Copy link
Contributor Author

@theuni Good spot. I think on many (?!) build environments both are set, but _WIN32 seems clearly safer. Updated.

@gmaxwell gmaxwell closed this Sep 21, 2015
@gmaxwell gmaxwell deleted the explicit_export branch September 21, 2015 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants