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

current spdlog v1.x branch (b6051273) fails to build with fmt (b79ed41) or later #2485

Closed
tycho opened this issue Sep 10, 2022 · 13 comments
Closed

Comments

@tycho
Copy link

tycho commented Sep 10, 2022

Originally submitted to fmt via fmtlib/fmt#3090 but they think it's an spdlog problem. Including the text of the bug report here for convenience:


I just did a rebase of my project, including latest upstream fmt and spdlog. It appears that fmt broke compatibility with spdlog somehow, but my C++ template-fu is weak and I'm not sure what the error message is trying to tell me exactly:

In file included from code/app.cpp:1:
In file included from code/lib/universal_include.h:450:
In file included from /Users/steven/Development/game/contrib/spdlog/include/spdlog/spdlog.h:14:
/Users/steven/Development/game/contrib/spdlog/include/spdlog/logger.h:375:13: error: no matching function for call to 'vformat_to'
            fmt::detail::vformat_to(buf, fmt, fmt::make_format_args(std::forward<Args>(args)...));
            ^~~~~~~~~~~~~~~~~~~~~~~
/Users/steven/Development/game/contrib/spdlog/include/spdlog/logger.h:90:9: note: in instantiation of function template specialization 'spdlog::logger::log_<const char *>' requested here
        log_(loc, lvl, fmt, std::forward<Args>(args)...);
        ^
code/app.cpp:114:2: note: in instantiation of function template specialization 'spdlog::logger::log<const char *>' requested here
        SPDLOG_LOGGER_INFO(s_logApp, "Version: {}", Game::Version::AnnotatedVersion());
        ^
/Users/steven/Development/game/contrib/spdlog/include/spdlog/spdlog.h:318:45: note: expanded from macro 'SPDLOG_LOGGER_INFO'
#    define SPDLOG_LOGGER_INFO(logger, ...) SPDLOG_LOGGER_CALL(logger, spdlog::level::info, __VA_ARGS__)
                                            ^
/Users/steven/Development/game/contrib/spdlog/include/spdlog/spdlog.h:296:58: note: expanded from macro 'SPDLOG_LOGGER_CALL'
#define SPDLOG_LOGGER_CALL(logger, level, ...) (logger)->log(spdlog::source_loc{__FILE__, __LINE__, SPDLOG_FUNCTION}, level, __VA_ARGS__)
                                                         ^
/Users/steven/Development/game/build/game.x86_64-clang-macos-x86_64-debug/contrib/include/fmt/format.h:4120:6: note: candidate template ignored: could not match 'basic_format_args' against 'format_arg_store'
void vformat_to(buffer<Char>& buf, basic_string_view<Char> fmt,
     ^

git bisection blames fmt revision fmtlib/fmt@b79ed41:

commit b79ed4105ab2ee268cfdf056aed5b7fb4c09dcb8
Author: Victor Zverovich <[email protected]>
Date:   Thu Sep 1 16:14:53 2022 -0700

    Remove unnecessary type_identity

Here's the bisection log:

$ git bisect log
# bad: [ecffca672656d6e4a2d39314237289f5740e8562] Don't parse '}' as fill
# good: [541cd218381493728241fcc690e8c362434a37ea] Fix locale name (thanks Mikhail Paulyshka)
git bisect start 'ecffca672656d6e4a2d39314237289f5740e8562' '541cd218381493728241fcc690e8c362434a37ea'
# good: [a07411c2b9c440ce0879deeb05774b19bdc48dd2] Disable compile-time checks for dynamic width/precision test for LCC and compiler without std::is_constant_evaluated()
git bisect good a07411c2b9c440ce0879deeb05774b19bdc48dd2
# bad: [f187274d3637403e1391deb69233b34b5e345d37] Add loc_value
git bisect bad f187274d3637403e1391deb69233b34b5e345d37
# bad: [48327a82e335fda96eb184602d84c562ddb372c2] Make format.h compile faster
git bisect bad 48327a82e335fda96eb184602d84c562ddb372c2
# good: [0b0f7cfbfcebd021c910078003d413354bd843e2] hip workaround
git bisect good 0b0f7cfbfcebd021c910078003d413354bd843e2
# bad: [b79ed4105ab2ee268cfdf056aed5b7fb4c09dcb8] Remove unnecessary type_identity
git bisect bad b79ed4105ab2ee268cfdf056aed5b7fb4c09dcb8
# good: [64e29893cfd3a603b79e38d75384e12336e8b980] Improve locale support
git bisect good 64e29893cfd3a603b79e38d75384e12336e8b980
# first bad commit: [b79ed4105ab2ee268cfdf056aed5b7fb4c09dcb8] Remove unnecessary type_identity
@gabime
Copy link
Owner

gabime commented Sep 10, 2022

It is being used here because my benchmarks showed it is about 15-20ns faster than fmt::vformat_to(std::back_inserter(buf),..).

@vitaut Is there a way to keep using it ? This is in the hot path of spdlog where performance is critical.

@vitaut
Copy link

vitaut commented Sep 10, 2022

You could use fmt::vformat_to which is a trivial wrapper around fmt::detail::vformat_to:
https://github.com/fmtlib/fmt/blob/21c2137e77a41a0b815c3735ce971e7af7cb0c21/include/fmt/core.h#L3224-L3231

If there is still perf difference, I'd be happy to look at the benchmark and see how it can be resolved. There is no reason for such difference to exist.

@gabime
Copy link
Owner

gabime commented Sep 10, 2022

I think this is the function I was referring to which was slower. Could be because I needed on each call to wrap my buffer with back_inserter just to get unwrapped again in fmt.

@vitaut
Copy link

vitaut commented Sep 12, 2022

Using fmt::appender instead of std::back_inserter should resolve perf difference without the need for internal API: https://godbolt.org/z/KGWj4GMe3. I'll add the same optimization for std::back_insert_iterator.

@gabime
Copy link
Owner

gabime commented Sep 12, 2022

That’s cool. Will try that.

@PengyiPan
Copy link

Have the same issue here. Buidling fails when using latest fmtlib with SPDLOG_FMT_EXTERNAL set.

@vitaut
Copy link

vitaut commented Sep 13, 2022

BTW handling of back_insert_iterator<buffer> has been optimized in fmtlib/fmt@192859c so using internal APIs shouldn't be needed anymore.

@h-vetinari
Copy link

This breakage puts us in a bit of a bind in the conda-forge ecosystem. We did not immediately unvendor format from spdlog (mistakes happen...), which now means many builds depending on spdlog are broken (because the ecosystem has moved on to fmt 9 in the meantime), and unvendoring causes an ABI break for packages built against the old world.

What would help us immensely would be to have a new version tag (whether 1.10.1 or 1.11.0), then we'd have a clear boundary (that's well-enforcable by the rest of our machinery) at which we could unvendor fmt, and get unstuck again.

@gabime
Copy link
Owner

gabime commented Oct 31, 2022

Updated bundled fmt to 9.1.0 and replaced fmt::detail::vformat_to: with fmt::vformat_to(fmt::appender(buf),...

@gabime gabime closed this as completed Oct 31, 2022
@h-vetinari
Copy link

@gabime: Updated bundled fmt to 9.1.0 and replaced fmt::detail::vformat_to: with fmt::vformat_to(fmt::appender(buf),...

Thanks a lot! 🙏🙃

What would help us immensely would be to have a new version tag (whether 1.10.1 or 1.11.0)

With the flurry of changes today, may I ask if a new release is somewhere on the horizon?

@gabime
Copy link
Owner

gabime commented Nov 1, 2022

Yes, should be a new release in few days.

@h-vetinari
Copy link

That's amazing, thank you very much! If you don't have a very strong preference about how to increment the version, 1.11.0 would make our lives quite a bit easier than 1.10.1. 🙃

@gabime
Copy link
Owner

gabime commented Nov 2, 2022

Released https://github.com/gabime/spdlog/releases/tag/v1.11.0

wdconinc added a commit to wdconinc/spack that referenced this issue Dec 30, 2022
This adds a new version of spdlog,
https://github.com/gabime/spdlog/releases/tag/v1.11.0

While the release notes are ambiguous, I think that this PR,
gabime/spdlog#2485, indicates that
spdlog from that point on uses features of fmt@9:.
haampie pushed a commit to spack/spack that referenced this issue Jan 10, 2023
This adds a new version of spdlog,
https://github.com/gabime/spdlog/releases/tag/v1.11.0

While the release notes are ambiguous, I think that this PR,
gabime/spdlog#2485, indicates that
spdlog from that point on uses features of fmt@9:.
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this issue Jan 24, 2023
…k#34731)

This adds a new version of spdlog,
https://github.com/gabime/spdlog/releases/tag/v1.11.0

While the release notes are ambiguous, I think that this PR,
gabime/spdlog#2485, indicates that
spdlog from that point on uses features of fmt@9:.
amd-toolchain-support pushed a commit to amd-toolchain-support/spack that referenced this issue Feb 16, 2023
…k#34731)

This adds a new version of spdlog,
https://github.com/gabime/spdlog/releases/tag/v1.11.0

While the release notes are ambiguous, I think that this PR,
gabime/spdlog#2485, indicates that
spdlog from that point on uses features of fmt@9:.
techxdave pushed a commit to Tech-XCorp/spack that referenced this issue Feb 17, 2023
…k#34731)

This adds a new version of spdlog,
https://github.com/gabime/spdlog/releases/tag/v1.11.0

While the release notes are ambiguous, I think that this PR,
gabime/spdlog#2485, indicates that
spdlog from that point on uses features of fmt@9:.
jmcarcell pushed a commit to key4hep/spack that referenced this issue Apr 13, 2023
…k#34731)

This adds a new version of spdlog,
https://github.com/gabime/spdlog/releases/tag/v1.11.0

While the release notes are ambiguous, I think that this PR,
gabime/spdlog#2485, indicates that
spdlog from that point on uses features of fmt@9:.
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

5 participants