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

Fix "undefined reference to `fmt::v7::detail::basic_data<void>::digits'" #2333

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

phprus
Copy link
Contributor

@phprus phprus commented May 31, 2021

If static fmt library is used in shared library.
Error in commit 13e6529.

@vitaut
Copy link
Contributor

vitaut commented Jun 1, 2021

I wonder why it doesn't happen in CI. Could you provide repro details?

@phprus
Copy link
Contributor Author

phprus commented Jun 1, 2021

Pseudocmake:

add_library(mylibrary SHARED lib.cpp)
target_link_libraries(mylibrary PRIVATE libfmt.a) # link with static fmt

add_executable(exe main.cpp)
target_link_libraries(exe PRIVATE mylibrary) # Error: undefined reference to `fmt::v7::detail::basic_data<void>::digits'

Because extern template struct basic_data<void>; and template struct detail::basic_data<void>; with visibility attribute is add reference to fmt::v7::detail::basic_data<void>::digits into mylibrary.so.

If link exe with libfmt.a, there will be no error.

src/format.cc Outdated Show resolved Hide resolved
@sergiud
Copy link
Contributor

sergiud commented Jun 1, 2021

Adding the combination to the test suite would certainly help catch problems in the future.

@sergiud
Copy link
Contributor

sergiud commented Jun 1, 2021

I believe, you are also linking to fmtlib incorrectly because the error occurs not in mylibrary but according to your description first in the exe target.

Note that libfmt is not linked transitively to exe because you specify the PRIVATE modifier:

add_library(mylibrary SHARED lib.cpp)
target_link_libraries(mylibrary PRIVATE libfmt.a) # link with static fmt

while you seem to be using libfmt in your exe target as well:

add_executable(exe main.cpp)
target_link_libraries(exe PRIVATE mylibrary) # Error: undefined reference to `fmt::v7::detail::basic_data<void>::digits'

I think, a clear (non-pseudo) example is necessary to understand the scenario.

@phprus
Copy link
Contributor Author

phprus commented Jun 1, 2021

@sergiud, No.
Now, mylibrary has are reference to fmt::v7::detail::basic_data<void>::digits always. Any use of mylibrary will result in an error.

The exe target does not use fmt functions anywhere and cannot depend on it.

@sergiud
Copy link
Contributor

sergiud commented Jun 1, 2021

I cannot reproduce the problem at ad97258. Here's my MWE:

CMakeLists.txt

cmake_minimum_required (VERSION 3.0)
project (fmt-link)

set (CMAKE_CXX_VISIBILITY_PRESET hidden)

find_package (fmt REQUIRED)

add_library (mylib SHARED mylib.cpp)
target_link_libraries (mylib PRIVATE fmt::fmt)

add_executable (myexe myexe.cpp)
target_link_libraries (myexe PRIVATE mylib)

mylib.cpp:

#include <fmt/format.h>

__attribute__((visibility("default")))
std::string foo()
{
    return fmt::format("foo bar {}", 42);
}

myexe.cpp:

#include <string>
#include <iostream>

extern std::string foo();

int main()
{
    std::cout << foo() << std::endl;
}

@phprus
Copy link
Contributor Author

phprus commented Jun 1, 2021

CMakeLists.txt:

cmake_minimum_required (VERSION 3.9)
project (fmt-link)

set(CMAKE_CXX_STANDARD                  17)
set(CMAKE_VISIBILITY_INLINES_HIDDEN     TRUE)
set(CMAKE_CXX_VISIBILITY_PRESET         "hidden")

include(CheckIPOSupported)
check_ipo_supported()
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION  TRUE)

set(BUILD_SHARED_LIBS           OFF)
add_subdirectory(fmt-master)
#set_property(TARGET fmt PROPERTY POSITION_INDEPENDENT_CODE ON)

add_library (mylib SHARED mylib.cpp)
target_link_libraries (mylib PRIVATE fmt::fmt)

add_executable (myexe myexe.cpp)
target_link_libraries (myexe PRIVATE mylib)

mylib.cpp:

#include <fmt/compile.h>

__attribute__((visibility("default")))
std::string foo()
{
    return fmt::format(FMT_COMPILE("foo bar {}"), 4242);
}

Build:

cmake -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_BUILD_TYPE=Release . && make

Error:

/usr/lib64/gcc/x86_64-suse-linux/7/../../../../x86_64-suse-linux/bin/ld: libmylib.so: неопределённая ссылка на «fmt::v7::detail::basic_data<void>::digits»

@phprus
Copy link
Contributor Author

phprus commented Jun 1, 2021

openSUSE Leap 15.2
gcc version 7.5.0 (SUSE Linux) ... and gcc-8 & 9

@sergiud
Copy link
Contributor

sergiud commented Jun 1, 2021

Still no linker error. I copy pasted your modifications and used a local clone of fmtlib included via add_subdirectory instead of using find_package

I'm on ArchLinux with gcc version 11.1.0.

Edit: I missed the foo function modification. Now I'm getting the linker error as well.

@sergiud
Copy link
Contributor

sergiud commented Jun 1, 2021

The culprit seems to be the combination libfmt.a + mylib.so + -flto. The sole modification to fix the issue that works for me is

diff --git a/include/fmt/format.h b/include/fmt/format.h
index 3f6ab357..71df6ced 100644
--- a/include/fmt/format.h
+++ b/include/fmt/format.h
@@ -902,7 +902,7 @@ template <typename T = void> struct basic_data {
   FMT_API static constexpr const char right_padding_shifts[] = {0, 31, 0, 1, 0};
 };
 
-#ifndef FMT_HEADER_ONLY
+#if !defined(FMT_HEADER_ONLY) && defined(FMT_EXPORT)
 // Required for -flto, -fivisibility=hidden and -shared to work
 extern template struct basic_data<void>;
 #endif

@phprus Can you verify this? FMT_API does not need to be changed.

@phprus
Copy link
Contributor Author

phprus commented Jun 2, 2021

I will check this change a little later today.

But anyway static libraries don't need visibility attribute. Therefore, I think it would be better to change the FMT_API as well.

@phprus
Copy link
Contributor Author

phprus commented Jun 2, 2021

!defined(FMT_HEADER_ONLY) && - this condition is not needed, because FMT_EXPORT is defined only on shared fmt.

FMT_EXPORT - is are private cmake definition
FMT_SHARED - is are interface cmake definition

In this case condition defined (FMT_EXPORT) is not needed and has been removed.

@@ -253,9 +253,10 @@
# endif
#else
# define FMT_CLASS_API
# if defined(__GNUC__) || defined(__clang__)
# define FMT_API __attribute__((visibility("default")))
# define FMT_EXTERN_TEMPLATE_API FMT_API
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove FMT_EXTERN_TEMPLATE_API definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FMT_EXTERN_TEMPLATE_API was added in the partial incorrect commit 13e6529 and is not used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, "partial incorrect commit" 👎

FMT_EXTERN_TEMPLATE_API was used before. But you're correct to note that it is not necessary anymore.

@sergiud
Copy link
Contributor

sergiud commented Jun 2, 2021

Could you also please add the combination to the test suite? This case is currently not being tested.

@phprus phprus force-pushed the export branch 2 times, most recently from 5729075 to ac89e52 Compare June 2, 2021 13:27
@phprus
Copy link
Contributor Author

phprus commented Jun 2, 2021

Test added.

@sergiud
Copy link
Contributor

sergiud commented Jun 2, 2021

Great, thanks! LGTM

@phprus
Copy link
Contributor Author

phprus commented Jun 2, 2021

Don't merge now.
I am making a change to the test.

@alexezeder
Copy link
Contributor

Note that you can transform this PR to Draft until you are ready.

@phprus
Copy link
Contributor Author

phprus commented Jun 2, 2021

Changes applied.
@vitaut review my PR please.

@vitaut vitaut merged commit f286139 into fmtlib:master Jun 2, 2021
@vitaut
Copy link
Contributor

vitaut commented Jun 2, 2021

Merged, thank you both!

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.

4 participants