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

C++20 modules build issues #4153

Closed
kamrann opened this issue Sep 4, 2024 · 5 comments
Closed

C++20 modules build issues #4153

kamrann opened this issue Sep 4, 2024 · 5 comments

Comments

@kamrann
Copy link
Contributor

kamrann commented Sep 4, 2024

I've hit a number of issues trying to package fmt for build2 with modules support.

  • A couple of I think recent regressions, addressed in this PR.
  • fmt.cc with import std; disabled will #include <expected> without any of the guards used for including it inside fmt/std.h, which raises a warning on MSVC. I haven't addressed this in the PR as FMT_HAS_INCLUDE isn't available at that point in the source file, and I'm also unsure why the C++ version and above macro are used in some places but feature test macros in others.
  • I guess the fmt modules support is not being verified by the CI? I'm also a bit unclear on what the status is of the tests wrt modules. I can see there is a module-test.cc, but before that gets added in the cmake file this check will bail out.

There appear to also be issues with the combination of modules and shared builds.

  • I've hit issues with the symbol export macros, however there are some unanswered questions here about what build2 should be doing, so it's not clear to me yet if anything will need to change on the fmt side for this particular problem.
  • Even if I work around the above, I end up with lots of linker errors about undefined symbols when building the tests against a shared modules build of the library (example output attached). I believe these derive from the fact that inside a module TU, member functions defined within the class definition are not implicitly inline as they are in traditional TUs. As such there are lots of functions that have not been marked as FMT_API (themselves or via their class) as they were implicitly inline; but now in the modules case they no longer are. There would appear to be two options: mark more classes/functions as FMT_API, or mark the member functions as explicitly inline. I wonder if the former can potentially cause problems for a non-modules build, but I'm generally unsure of the trade-offs here.

clang-libcpp-fedora-modules-link-errors.txt

@vitaut
Copy link
Contributor

vitaut commented Sep 5, 2024

Thanks for the fixes!

The warning about <expected> should be suppressed in a2c290b. Tests need more work to make them compatible with modules which is why they are currently disabled.

I haven't tried using modules with shared libraries. As usual PRs to make {fmt} more compatible with this configuration would be welcome.

@vitaut
Copy link
Contributor

vitaut commented Sep 5, 2024

Closing for now as I don't see anything actionable from {fmt} but feel free to reopen if new information comes up.

@vitaut vitaut closed this as completed Sep 5, 2024
@kamrann
Copy link
Contributor Author

kamrann commented Sep 6, 2024

No worries.

I'd have to experiment a bit and not sure how soon I can, but if I were to make the shared library changes with the second approach mentioned (essentially changing a lot of member functions from FMT_CONSTEXPR to FMT_CONSTEXPR FMT_INLINE), would you be ok with that? As I understand it, it means that the member function bodies become part of the module ABI, so it's somewhat less encapsulated than taking the FMT_API route, but I think less risky and more in-sync with the non-modules build.

@vitaut
Copy link
Contributor

vitaut commented Sep 6, 2024

constexpr is implicitly inline so we could probably change FMT_CONSTEXPR to include inline if that is beneficial for the modular build.

@kamrann
Copy link
Contributor Author

kamrann commented Sep 7, 2024

Right, I was wondering about this since in the module ABI sense also, it doesn't really seem to make sense to have a constexpr member function without exporting the body. Had a quick look at the standard and I'm now suspecting this should be correct as is and is perhaps a clang bug.

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

2 participants