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

Explicitly export buffer<char> for MSVC #3132

Closed
wants to merge 3 commits into from
Closed

Explicitly export buffer<char> for MSVC #3132

wants to merge 3 commits into from

Conversation

Youw
Copy link
Contributor

@Youw Youw commented Oct 11, 2022

Some MSVC-specific behavior:

Class ostream by inheriting detail::buffer implicitly exports it when fmt is built as a shared library. Unless os.h is included, the compiler assumes detail::buffer is not externally exported and instantiets a local copy of it, which causes ODR violation.

Some MSVC-specific behavior:

Class ostream by inheriting detail::buffer<char> implicitly exports it when fmt is built as a shared library.
Unless os.h is included, the compiler assumes detail::buffer<char> is not externally exported and instantiets a local copy of it,
which cases ODR violation.
@Youw
Copy link
Contributor Author

Youw commented Oct 11, 2022

So the scenario to trigger the ODR violation is a bit untrivial and gets reproduced only by MSVC, but still:

fmt.dll: is built and used as a shared library
my_application:
  main.cpp <- uses: fmt::ostream out = fmt::output_file(...)
  utest_logger.cpp <- uses fmt/format.h only

C++17 is used on all platforms.
It compiles and runs just fine on macOS(clang), Linux(GCC) and Android (Clang/NDK).
On Windows (MSVC2019) I get a linker (ODR violation) error:

fmt.lib(fmt.dll) : error LNK2005: "protected: __cdecl fmt::v9::detail::buffer<char>::buffer<char>(char *,unsigned __int64,unsigned __int64)" (??0?$buffer@D@detail@v9@fmt@@IEAA@PEAD_K1@Z) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "protected: void __cdecl fmt::v9::detail::buffer<char>::set(char *,unsigned __int64)" (?set@?$buffer@D@detail@v9@fmt@@IEAAXPEAD_K@Z) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: char * __cdecl fmt::v9::detail::buffer<char>::end(void)" (?end@?$buffer@D@detail@v9@fmt@@QEAAPEADXZ) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: unsigned __int64 __cdecl fmt::v9::detail::buffer<char>::size(void)const " (?size@?$buffer@D@detail@v9@fmt@@QEBA_KXZ) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: unsigned __int64 __cdecl fmt::v9::detail::buffer<char>::capacity(void)const " (?capacity@?$buffer@D@detail@v9@fmt@@QEBA_KXZ) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: char * __cdecl fmt::v9::detail::buffer<char>::data(void)" (?data@?$buffer@D@detail@v9@fmt@@QEAAPEADXZ) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: void __cdecl fmt::v9::detail::buffer<char>::try_resize(unsigned __int64)" (?try_resize@?$buffer@D@detail@v9@fmt@@QEAAX_K@Z) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: void __cdecl fmt::v9::detail::buffer<char>::try_reserve(unsigned __int64)" (?try_reserve@?$buffer@D@detail@v9@fmt@@QEAAX_K@Z) already defined in utest_logger.cpp.obj
fmt.lib(fmt.dll) : error LNK2005: "public: void __cdecl fmt::v9::detail::buffer<char>::push_back(char const &)" (?push_back@?$buffer@D@detail@v9@fmt@@QEAAXAEBD@Z) already defined in utest_logger.cpp.obj
target\bin\utest_core.exe : fatal error LNK1169: one or more multiply defined symbols found

NOTE: there is a workaround: include fmt/os.h in utest_logger.cpp. In that case no linker issues.
NOTE2: if ostream/output_file is not used (regardless either fmt/os.h is included or not) in main.cpp - there is no linker issues either.


This is the result running on current release 9.1.0.
On latest master there is less "already defined in" errors, but overall the issue persists.

include/fmt/core.h Outdated Show resolved Hide resolved
src/format.cc Outdated Show resolved Hide resolved
src/format.cc Outdated Show resolved Hide resolved
@Youw
Copy link
Contributor Author

Youw commented Oct 12, 2022

What I didn't do (and something that might be affected) - I didn't run the performance testing.
In theory this may prevent some linke optimisations and bring the runtime overhead.

Is there any automated tools/scripts to check the performance before/after the change?

@vitaut
Copy link
Contributor

vitaut commented Oct 12, 2022

Is the any automated tools/scripts to check the performance before/after the change?

Not automated but there are some benchmarks in https://github.com/fmtlib/format-benchmark. They might need a bit of an update.

@Youw
Copy link
Contributor Author

Youw commented Oct 13, 2022

It just came to my mind, but I think the alternative to this fix would be to aggregate detail::buffer<char> by the ostream instead of inheriting it.
But I need to test this assumption.

@vitaut
Copy link
Contributor

vitaut commented Oct 18, 2022

#3139 looks like a better workaround, let's go with it.

@vitaut vitaut closed this Oct 18, 2022
@Youw Youw deleted the msvc-shared-build branch October 18, 2022 21:54
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.

2 participants