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

cmake windows shared builds do not export symbols #72

Open
sigmoidal opened this issue Feb 18, 2018 · 11 comments
Open

cmake windows shared builds do not export symbols #72

sigmoidal opened this issue Feb 18, 2018 · 11 comments

Comments

@sigmoidal
Copy link
Contributor

When building shared libs on windows, using cmake and -DBUILD_SHARED_LIBS=On, no .lib file is produced, which in turn doesn't allow a consumer to link against cctz.

Ideally, symbols should be properly exported (see https://msdn.microsoft.com/en-us/library/a90k134d.aspx), then a lib file will be generated.

A not-so-elegant, but working solution may be to enable: CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS, which I had success with, however for optimal results you should only export what should be exported.

This workaround is as simple as adding:

set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS YES CACHE BOOL "Export all symbols")

or running the cmake config command with -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=True

thanks.

@Sarcasm
Copy link
Contributor

Sarcasm commented Feb 18, 2018

Until this is officially supported by cctz in general, I think the correct way is to use the CMake command line option: -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=True.
Overriding the cache from the CMakeLists.txt should be avoided, if the CMakeLists.txt needs to be touched, it would be slightly cleaner to specify the target property WINDOWS_EXPORT_ALL_SYMBOLS to the targets.

I think the real fix is to introduce some export macros so that cctz is a better Windows citizen.
I expect the Bazel build could also benefit from this.

Maybe something along these lines (though I'm not Windows expert):

#if defined(WIN32) || defined(_WIN32)
#define CCTZ_EXPORT __declspec(dllexport)
#else
#define CCTZ_EXPORT [[gnu::visibility("default")]]
#endif

...

class CCTZ_EXPORT time_zone { ... };

By the way, do you have a minimal example to test what is not working here?

@sigmoidal
Copy link
Contributor Author

I think the solution to export symbols is the right way to go, just like most libraries.

I don't have an example project to offer as I use upstream's example1.cc with a conan recipe, since I am helping package cctz for the conan package manager (https://github.com/bincrafters/conan-cctz), thus we run libs through CI and use them as dependencies for other projects, so problems become quickly apparent.

There isn't anything special I am doing, if you try to compile cctz on windows with -DBUILD_SHARED_LIBS=On you will notice no lib is created, thus one cannot link against it.

I'd be happy to help if necessary.

@devbww
Copy link
Contributor

devbww commented Feb 21, 2018

This "declspec" question has come up before, but perhaps this time there is enough expertise out there to be able to come to some conclusion.

So, my question is, "where would this CCTZ_EXPORT be applied?"

It seems to me that every current external symbol would need to be exported, so I'm not sure why we shouldn't just make that the default, like it is for non-Windows platforms.

@sigmoidal
Copy link
Contributor Author

I want to clarify that I don't use cctz currently, so I don't have a clear picture of the use cases and respective functions that may be candidates for the exported symbol table.

You can do something like have this in a header file:

#define _CCTZ_EXPORTS_H__

#ifdef _WIN32
    #ifdef CCTZ_EXPORTS
        #define CCTZ_API __declspec(dllexport)
    #else
        #define CCTZ_API __declspec(dllimport)
    #endif
#else
    #define CCTZ_API
#endif

#endif 

a) decorate classes/functions to be exported with CCTZ_API: e.g. class CCTZ_API time_zone { ... };
b) When building the library using CCTZ_EXPORTS the dll export table will be generated.
c) consumers won't have to do anything (they don't specify CCTZ_EXPORTS usually), thus functions will be imported.

@Sarcasm
Copy link
Contributor

Sarcasm commented Feb 21, 2018

IIUC, Bradley basically suggested we use WINDOWS_EXPORT_ALL_SYMBOLS.
Maybe it makes sense to do that now and use something a finer grained method later if needed.

@devbww
Copy link
Contributor

devbww commented Feb 22, 2018

IIUC, Bradley basically suggested we use WINDOWS_EXPORT_ALL_SYMBOLS.

Right. The direction to "decorate classes/functions to be exported with CCTZ_API" seems brittle and noisy when every external symbol should be, well, external.

I'm also unsure why shared libraries and "non-shared" libraries should be different.

@sigmoidal
Copy link
Contributor Author

I consider this a solved issue as far as the original problem is concerned which was that no symbols were exported at all.

I don't however agree that we are discussing external symbols here and I also disagree that what I am proposing is brittle and noisy. We are talking about exported symbols.

The discussion as far as I understand is about (a) exporting every single function and class in the library (which the cmake approach offers for simplicity) vs (b) a clean set of functions/classes intended for use by consumer end-user software.

As far as the end users are concerned I doubt what I am proposing will produce libs that are different in functionality between their shared and static versions. Is there something specific about cctz that would cause this?

Since include/cctz is the public interface of cctz, why would the exports table include all the internal functionality from src/*.h, especially when these internal headers are not distributed with the generated lib.

I understand this may be low priority or unnecessary at this point for cctz, but thanks for discussing it.

@devbww
Copy link
Contributor

devbww commented Apr 17, 2018

Sorry for being slow to respond.

I'm happy to admit that my "every external symbol should be, well, external" assertion was incorrect. There are plenty of "internal" symbols that are only external because they are defined-in and referenced-from different translation units. Having them appear to be internal to the final library, be it shared or not, would be fine.

So, your suggestion stands as (given a suitable definition for CCTZ_API) "decorate classes/functions to be exported with CCTZ_API". OK. I'll revamp a previous question then as to whether anyone knows exactly where those decorations need to go.

Thanks.

@sigmoidal
Copy link
Contributor Author

Thanks for responding and especially for taking this positive look at my suggestion.

@olivier-fs
Copy link

Hi!

Bumping this old but still open issue.

Of course using CMake CLI option -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=True works.
But it exports tons of symbols that should probably have been kept private/internal.
With latest CMake 3.27.4 I get 220 exported symbols in cctz.dll.

#define CCTZ_API __declspec(dllexport)

  • would probably reduce that list a lot.
  • would make it clear what is an API in source/header files.

This xxx_API / xxx_EXPORTS approach is used by almost every library that has to deal with the Windows linker "dynamic loading but still static linking" old but forever annoying problem with DLLs.

Useless entries in DLL's symbol table slows down the startup of processes for no reason.

There hasn't been a cctz release for a lonnnng time.
I agree that it is a low priority issue, but may be this could be closed now by sprinkling those CCTZ_API.

Thanks!

@devbww
Copy link
Contributor

devbww commented Aug 28, 2023

it is a low priority issue, but may be this could be closed now by sprinkling those CCTZ_API.

Agreed. And I would happily merge a PR from a Windows developer who can determine what to sprinkle and where, and how to test it.

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

No branches or pull requests

4 participants