Module linkage fixes for shared build by kamrann · Pull Request #4169 · fmtlib/fmt

kamrann · 2024-09-20T04:52:51Z

Following on from #4153:

In regards to constexpr, this was indeed a Clang bug, and anything already marked constexpr should be a non-issue; the problem has been fixed on Clang trunk with potential for it to be backported to the 19.x release.
A number of other linker errors specific to mixing #include with import were also being caused by a related Clang bug, also now fixed on trunk.
Despite this, a bunch of linker errors still remained when trying to consume the fmt module via a shared library build, due to the last bullet point in the linked issue.

This PR addresses those remaining issues by adding FMT_INLINE/FMT_CONSTEXPR to some member functions defined within the class definition. However, due to the nature of linker issues, problematic cases are only being picked up where a symbol is actually being used from an external dll/exe. Currently, the coverage for that is limited - fmt's own CI is not yet running modules builds I think (or anyway not with FMT_SHARED); the build2 fmt package CI also cannot stress test this as the very latest Clang is not available. Furthermore, I've so far only enabled a subset of the tests in the build2 package.

Fundamentally, I think this addition of inline or constexpr is essentially needed on every single in-class defined member function which is intended for public use (or alternatively FMT_API on the member/class), which is probably a lot more than what I've addressed in these commits and would be quite intrusive. So of course feel free to merge if you like, but it's possible some more thought is warranted in regards to

is every public member function in the headers actually intended to be usable directly by clients (this question of course also relates to what should be exported from the module, but for export, doing things in bulk is possible and less intrusive in terms of code changes)
is there a preference for exposing those that should be exposed as inline vs FMT_API

…texpr, to avoid missing external symbols when using fmt module with shared build due to modules not defaulting to implicit inline.

NOTE: Looks as if basic_format_args::get(string_view) could probably be made constexpr instead, but sticking with minimal change approach.

…+ std::string constexpr limitations.

kamrann · 2024-09-20T08:29:52Z

Linter seems to be complaining about formatting that I didn't change.

vitaut · 2024-09-21T14:47:12Z

Before we go into detail, could you link to where this assumption comes from?

inside a module TU, member functions defined within the class definition are not implicitly inline

This seems like a major incompatibility and I would be surprised if it was the case.

kamrann · 2024-09-22T03:40:52Z

Sure. The relevant revision to the standard was P1779. There's also some discussion regarding it in this SO post.

vitaut · 2024-09-22T15:28:44Z

include/fmt/base.h

+  FMT_CONSTEXPR explicit buffer_traits(size_t) {}
+  FMT_CONSTEXPR auto count() const -> size_t { return 0; }
+  FMT_CONSTEXPR auto limit(size_t size) -> size_t { return size; }


These can be constexpr. FMT_CONSTEXPR is only for cases when C++14 relaxed constexpr is needed.

Okay my bad, hadn't realized the distinction. I think all the changes are now constexpr where they can be.

vitaut · 2024-09-22T15:29:09Z

include/fmt/base.h

+  FMT_CONSTEXPR explicit fixed_buffer_traits(size_t limit) : limit_(limit) {}
+  FMT_CONSTEXPR auto count() const -> size_t { return count_; }
+  FMT_CONSTEXPR auto limit(size_t size) -> size_t {


Same here and in a few more places below.

vitaut · 2024-09-22T15:31:30Z

include/fmt/base.h

  constexpr locale_ref() : locale_(nullptr) {}
  template <typename Locale> explicit locale_ref(const Locale& loc);
-  explicit operator bool() const noexcept { return locale_ != nullptr; }
+  FMT_INLINE explicit operator bool() const noexcept { return locale_ != nullptr; }


Here and elsewhere, please use inline instead of FMT_INLINE since the latter expands to always inline in optimized mode.

vitaut · 2024-09-22T15:32:58Z

include/fmt/chrono.h

+    FMT_INLINE dispatcher(std::time_t t) : time_(t) {}

-    auto run() -> bool {
+    FMT_INLINE auto run() -> bool {
      using namespace fmt::detail;
      return handle(localtime_r(&time_, &tm_));
    }

-    auto handle(std::tm* tm) -> bool { return tm != nullptr; }
+    FMT_INLINE auto handle(std::tm* tm) -> bool { return tm != nullptr; }

-    auto handle(detail::null<>) -> bool {
+    FMT_INLINE auto handle(detail::null<>) -> bool {
      using namespace fmt::detail;
      return fallback(localtime_s(&tm_, &time_));
    }

-    auto fallback(int res) -> bool { return res == 0; }
+    FMT_INLINE auto fallback(int res) -> bool { return res == 0; }

 #if !FMT_MSC_VERSION
-    auto fallback(detail::null<>) -> bool {
+    FMT_INLINE auto fallback(detail::null<>) -> bool {


This is not needed since the struct where these functions are defined is inaccessible to module consumers.

Although I may have branched out a little when making the changes, the vast majority of them were made on functions that were generating linker errors. I've verified again, these are necessary for the linker to succeed.

Essentially, the issue being addressed here is one of dll symbol export, not module name export, and so the visibility of names (which is what module export/detail namespaces relate to) I think has no bearing. Since this struct is in an inline function which is exposed, a consumer TU that calls this function (directly or transitively) will either need to inline the member functions of the struct, or emit an external symbol reference for them. So the same choice still applies.

include/fmt/chrono.h

vitaut · 2024-09-22T15:35:00Z

include/fmt/chrono.h


 struct tm_format_checker : null_chrono_spec_handler<tm_format_checker> {
-  FMT_NORETURN void unsupported() { FMT_THROW(format_error("no format")); }
+  FMT_NORETURN FMT_INLINE void unsupported() { FMT_THROW(format_error("no format")); }


Here and in other places in the detail namespace - inline is not needed since it is not exported from the module.

As above, detail namespace functions will still generate linker errors if neither inline nor dll-exported.

vitaut · 2024-09-25T15:21:52Z

OK, considering that the number of changes is manageable, we can merge it in but please apply clang-format first.

vitaut · 2024-09-26T14:54:01Z

Merged, thanks!

kamrann added 5 commits September 20, 2024 11:20

Mark some in-class defined member functions as explicitly inline/cons…

e1789cf

…texpr, to avoid missing external symbols when using fmt module with shared build due to modules not defaulting to implicit inline.

Merge branch 'master' into pr/module-linkage-fixes

27f9966

Switch constexpr to inline for context::arg(string_view).

9ca41fa

NOTE: Looks as if basic_format_args::get(string_view) could probably be made constexpr instead, but sticking with minimal change approach.

Work around apparent non-conformance of older MSVC compilers.

a67e251

Switch format_int::str() from constexpr to inline to satisfy libstdc+…

f9e99e5

…+ std::string constexpr limitations.

vitaut requested changes Sep 22, 2024

View reviewed changes

kamrann added 4 commits September 23, 2024 14:45

Replace usages of macros for constexpr/inline with keywords.

afb5f21

Merge branch 'master' into pr/module-linkage-fixes

8765197

Fix for locations requiring C++14 constexpr.

076afe3

Further minor constexpr tweaks.

7226c0e

kamrann force-pushed the pr/module-linkage-fixes branch from e30fadd to 7226c0e Compare September 23, 2024 06:28

Apply clang format

d6c7200

vitaut approved these changes Sep 26, 2024

View reviewed changes

vitaut merged commit 96dca56 into fmtlib:master Sep 26, 2024

kamrann deleted the pr/module-linkage-fixes branch September 27, 2024 07:30

vitaut mentioned this pull request May 12, 2025

Remove some redundant consts and inlines #4445

Merged

Uh oh!

Conversation

kamrann commented Sep 20, 2024

Uh oh!

kamrann commented Sep 20, 2024

Uh oh!

vitaut commented Sep 21, 2024

Uh oh!

kamrann commented Sep 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitaut commented Sep 25, 2024

Uh oh!

vitaut commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants