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.

Sign up for GitHub

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

Fix DLL visibility of explicit instantiation "declaration" of internal::basic_data<void> in header format.h and the explicit instantiation "definition" in format.cc #1134

Merged
merged 8 commits into from
May 2, 2019
Merged

Fix DLL visibility of explicit instantiation "declaration" of internal::basic_data<void> in header format.h and the explicit instantiation "definition" in format.cc #1134

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
01bb881
Update format.cc
denchat Apr 28, 2019
3944deb
Mirror visibility of explicit instantiation declaration
denchat Apr 29, 2019
e4bd8c1
Eliminate `__declspec(dllexport)` designation on extern template inte…
denchat Apr 30, 2019
4f919b8
Add `FMT_EXTERN_TEMPLATE_API` for designate DLL export `extern template`
denchat May 2, 2019
c6e6bfe
Delete whole `FMT_USE_EXTERN_TEMPLATES` block and its condition
denchat May 2, 2019
b4a4dcd
Add `#define FMT_EXTERN extern` only when not `FMT_HEADER_ONLY`
denchat May 2, 2019
7d0dd76
Replace `extern` on basic_data<void> with the `FMT_EXTERN` condition …
denchat May 2, 2019
c3d87c5
replace misspelled if !define() with ifndef
denchat May 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ template <typename T = void> struct FMT_API basic_data {
};

#if FMT_USE_EXTERN_TEMPLATES
extern template struct basic_data<void>;
extern template struct FMT_API basic_data<void>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right, e.g. https://godbolt.org/z/kGqqeS gives

<source>(6): warning C4910: 'basic_data<void>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. You are right. It seems the instantiation declaration implicitly has got the same __declspec(dllexport) from the class template definition.

So we can resolve this at the class template definition alone.

It looks like travis-ci compilation failed by some reason. How can I make them recompile?

Copy link
Contributor Author

@denchat denchat Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My problem is when I tried to compile this code on Windows

#include <cstdio>
#include <fmt/printf.h>

int main()
{
  fmt::printf("Hello, %s!\n", "world");
}

The linker can not find internal::basic_data<void>::ZERO_OR_POWERS_OF_10_32

lld-link: error: undefined symbol: __declspec(dllimport) public: static unsigned int const *const fmt::v5::internal::basic_data<void>::ZERO_OR_POWERS_OF_10_32

This is because we were trying to dllexport : explicit class template instantiation "declaration"

template <typename T = void> struct __declspec(dllexport) basic_data {
  static const uint64_t POWERS_OF_10_64[];
  static const uint32_t ZERO_OR_POWERS_OF_10_32[];
  static const uint64_t ZERO_OR_POWERS_OF_10_64[];
  static const uint64_t POW10_SIGNIFICANDS[];
  static const int16_t POW10_EXPONENTS[];
  static const char DIGITS[];
  static const char HEX_DIGITS[];
  static const char FOREGROUND_COLOR[];
  static const char BACKGROUND_COLOR[];
  static const char RESET_COLOR[5];
  static const wchar_t WRESET_COLOR[5];
};

#if FMT_USE_EXTERN_TEMPLATES
extern template struct basic_data<void>;
#endif

but not the explicit class template instantiation "definition" in format.cc

template struct internal::basic_data<void>;

When doing DLL on extern template instantiation, we have to do something like this.

#ifdef XXXX_BUILD
    #define XXXX_EXPORT __declspec(dllexport)
    #define XXXX_EXTERN
#else
    #define XXXX_EXPORT __declspec(dllimport)
    #define XXXX_EXTERN extern
#endif
....
XXXX_EXTERN template class XXXX_EXPORT YourClass<double>;

REF: https://stackoverflow.com/a/46392757/6370128

So we want something like this

#define FMT_API __declspec(dllexport)

template <typename T = void> struct basic_data {};

extern template struct basic_data<void>;

//
//  ...far far away in the same translation unit (in case of format.cc)
//  (or expecting code below in another translation unit (in case of posix.cc) )
//

template struct FMT_API basic_data<void>;

godbolt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think extern is actually useful here, so I suggest just removing it together with the whole FMT_USE_EXTERN_TEMPLATES block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extern is telling other translation units that they all could rely on the definition of internal::basic_data<void> in format.cc. So that all translation units using internet::basic_dat<void>, except format.cc, don't need to instantiate it by themselves. This makes compilation faster. This is the purpose of c++11 explicit class template declaration feature.

If extern in format.h is removed, so it becomes the definition itself. All translation units using format.h will be required to instantiate internal::basic_data<vod> every single time which that doesn't need to, only if using c++11 explicit class template declaration feature.

Besides, what about the definition in format.cc? Now there would be already another definition in format.h, so the definition in format.cc must be removed altogether with extern then.

However, if you are affirmed that the explicit class template declaration feature would make lesser benefits than the complexity it'd gain here, I will make the definition stay in format.h as you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it's better to keep extern template, but please remove the macro FMT_USE_EXTERN_TEMPLATES. We already require C++11 and this feature is widely available. Hopefully this will simplify the logic around FMT_API a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already removed the whole FMT_USE_EXTERN_TEMPLATES block.

Anyway, I guess

  • __declspec(DLL) api of the template definition having any extern declarations must have its own condition other than FMT_API.
  • the extern in front of extern template declaration itself must also have its own condition as well.

I'm not any good at naming. I only could come up with FMT_EXTERN_TEMPLATE_API and FMT_EXTERN respectively and make them added in core.h. Any suggestions are welcome.

#endif

// This is a struct rather than a typedef to avoid shadowing warnings in gcc.
Expand Down
2 changes: 1 addition & 1 deletion src/format.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include "fmt/format-inl.h"

FMT_BEGIN_NAMESPACE
template struct internal::basic_data<void>;
template struct FMT_API internal::basic_data<void>;

// Workaround a bug in MSVC2013 that prevents instantiation of grisu_format.
bool (*instantiate_grisu_format)(double, internal::buffer<char>&, int, unsigned,
Expand Down