[SERVER-45445] Upgrade of libfmt to 7.1.3 Created: 09/Jan/20  Updated: 08/Jan/24  Resolved: 22/Apr/21

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 5.0.0-rc0

Type: Task Priority: Major - P3
Reporter: Spencer Jackson Assignee: Mark Benvenuto
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-54368 Failure to compile when including <fm... Backlog
Problem/Incident
Backwards Compatibility: Fully Compatible
Sprint: Dev Tools 2020-02-10, Dev Tools 2020-02-24, Security 2021-02-22, Security 2021-03-08, Security 2021-04-19, Security 2021-05-03
Participants:
Linked BF Score: 170

 Description   

A new release has been made: https://github.com/fmtlib/fmt/releases/tag/7.0.1
More information: https://github.com/fmtlib/fmt/blob/master/ChangeLog.rst#701---2020-07-07



 Comments   
Comment by Billy Donahue [ 23/Apr/21 ]

Good emergency patch.

It needs some follow-up action. It changes the source of the fmt library, so we need to track that change in https://github.com/mongodb-forks/fmt.
This replaces the old workflow of keeping a patches/ directory.

From there we could make a more principled change and upstream it perhaps. If __cplusplus>=201703L then we don't need FMT_HAS_INCLUDE at all. Assume that any C++17 implementation has <string_view>. We could rearrange the if condition slightly to bypass the FMT_HAS_INCLUDE check for C++17 .
That would be an upstreamable PR I think.

Comment by Githook User [ 23/Apr/21 ]

Author:

{'name': 'Mark Benvenuto', 'email': 'mark.benvenuto@mongodb.com', 'username': 'markbenvenuto'}

Message: SERVER-45445 Workaround Clang's buggy -frewrite-includes
Branch: master
https://github.com/mongodb/mongo/commit/70edddbf56bfca262558baecbc535256b189d976

Comment by Billy Donahue [ 21/Apr/21 ]

Woohoo!

What a slugfest.

Comment by Githook User [ 21/Apr/21 ]

Author:

{'name': 'Mark Benvenuto', 'email': 'mark.benvenuto@mongodb.com', 'username': 'markbenvenuto'}

Message: SERVER-45445 Upgrade third_party/fmt (6.1.1 -> 7.1.3)
Branch: master
https://github.com/mongodb/mongo/commit/2f0aae1ce89a3493fec0bedfbed4788f070105c2

Comment by Billy Donahue [ 22/Mar/21 ]

It might make sense to wait for libfmt-1.7.4 when these upstream issues will be fixed and integrated into a release.

We'd still have to deal with the decimal point elision problem somehow.

Comment by Billy Donahue [ 20/Mar/21 ]

I should write down the 4 fmtlib bugs I reported upstream while pursing this ticket.

https://github.com/fmtlib/fmt/issues/2178 (fixed)
https://github.com/fmtlib/fmt/issues/2181 (fixed)
https://github.com/fmtlib/fmt/issues/2180 (fixed)
https://github.com/fmtlib/fmt/issues/2185 ( w/ PR https://github.com/fmtlib/fmt/pull/2187 )

Comment by Billy Donahue [ 18/Mar/21 ]

Returning it to Security backlog.

The C++ side is proven out, but the unforeseen problems at the next layer of the onion have now arisen.

The problems now seems to be the fragile JSTests caring too much about round trip fidelity of decimal point placement even as an object goes from:

JavaScript JSON => wire BSON => C++ BSONObj => LogV2 JSON => JavaScript JSON

We can attack the jstest libraries and fix the logical problem.

Or alternatively figuring out how to choose backward-compatible formats for logging floating point quantities.
Or even extend libfmt to provide exactly the old decimalization behavior as an option.

Comment by Billy Donahue [ 18/Mar/21 ]

The revert was two causes.

Some compile failures. Could fix the compile failures with these tweaks:

https://github.com/mongodb/mongo/compare/9154322...be337da

More serious is default output format change for double. The meaning of format("{}"), 1.0) has changed. It used to produce "1.0" like python does. In 7.1.3, it produces "1" which is specified by C++20's std::format.

If you want the old behavior you can use a hash mark for "alternate format", but that {:#} isn't even true. That forces a decimal point to appear but doesn't force digits to appear after it.

Anyway we have jstests that are making a fragile STRING comparison of json objects obtained from the log, and basically that's what's failing. JavaScript serializes an object like the "command" of message 51803 (Slow query). And then it expects that exact substring to appear in the fetched log, which is rather messed up, because it's insisting that JavaScript and C++ use the same representation for JSON numerics, and we never had that guarantee. JSON has only one numeric type, and only the values really matter.

So we could try to get into the JavaScript log matching library and make it use value comparisons instead of string comparisons.

The difference is in apparent below in the formatting of command._id and command.batchSize, but not nShards. Lobster and jq hide these differences, btw. You have to look at raw output.

// BEFORE (SUCCESS):
 
{"t":{"$date":"2021-03-18T03:13:41.605+00:00"},"s":"I",  "c":"COMMAND",  "id":51803,   "ctx":"conn12","msg":"Slow query","attr":{
    "type":"command",
    "ns":"allow_partial_results_nshards.test",
    "appName":"MongoDB Shell",
    "command":{
        "find":"test",
        "filter":{},
        "sort":{"_id":1.0},
        "allowPartialResults":true,
        "comment":"allow_partial_results_find_nshards_2",
        "batchSize":2.0,
        "$db":"allow_partial_results_nshards"},
    "nShards":2,
    "cursorid":1143586564051078280,
    "numYields":0,
    "nreturned":2,
    "reslen":290,
    "remote":"127.0.0.1:42356",
    "protocol":"op_msg",
    "durationMillis":2}}
 
// AFTER (FAILURE):
 
{"t":{"$date":"2021-03-15T16:25:35.764+00:00"},"s":"I",  "c":"COMMAND",  "id":51803,   "ctx":"conn11","msg":"Slow query","attr":{
    "type":"command",
    "ns":"allow_partial_results_nshards.test",
    "appName":"MongoDB Shell",
    "command":{
        "find":"test",
        "filter":{},
        "sort":{"_id":1},
        "allowPartialResults":true,
        "comment":"allow_partial_results_find_nshards_2",
        "batchSize":2,
        "$db":"allow_partial_results_nshards"
    },
    "nShards":2,
    "cursorid":5030931449254192477,
    "numYields":0,
    "nreturned":2,
    "reslen":290,
    "remote":"127.0.0.1:38862",
    "protocol":"op_msg",
    "durationMillis":2}}

https://logkeeper.mongodb.org/lobster/build/313e35b3f5c4ca6cb5e439779a0935b7/test/6051f4019041304fb3bd5363#bookmarks=0%2C5600%2C5605%2C6786

Comment by Githook User [ 17/Mar/21 ]

Author:

{'name': 'XueruiFa', 'email': 'xuerui.fa@mongodb.com', 'username': 'XueruiFa'}

Message: Revert "SERVER-45445 Upgrade third_party/fmt (6.1.1 -> 7.1.3)"

This reverts commit dc68f885b80e70805eab8c5686ee0941bbdd806b.
Branch: master
https://github.com/mongodb/mongo/commit/191e5b8f0c1b946e0ee785c908da82a14270fa22

Comment by Githook User [ 17/Mar/21 ]

Author:

{'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}

Message: SERVER-45445 Upgrade third_party/fmt (6.1.1 -> 7.1.3)
Branch: master
https://github.com/mongodb/mongo/commit/dc68f885b80e70805eab8c5686ee0941bbdd806b

Comment by Billy Donahue [ 17/Mar/21 ]

The notes from the CR, now that it's finalized.

 
Server changes needed for new libfmt:
 
`FMT_COMPILE`:
 
    - Replace all static `fmt::compile` objects with inlined FMT_COMPILE macro.
      The static compiled objects are deprecated and don't work the same way.
 
    - Compiled formats will not accept enums as integers, or types
      having a to_string_view function.
      ( http://github.com/fmtlib/fmt/issues/2181 ).
 
    - Compiled formats cannot directly write to a fmt::memory_buffer.
      This was always an almost-documented feature of non-compiled formats.
      So you have to `fmt::format_to` a `std::back_inserter(buffer)`.
      ( https://github.com/fmtlib/fmt/issues/1303 ).
 
Hacky use of `fmt::internal::*` names:
 
    Replace fmt::internal types with equivalent mongo-defined types.
 
    - fmt::internal::udl_arg<char> -> logv2::detail::AttrUdl.
 
    - fmt::internal::named_arg<T,char> -> logv2::detail::NamedArg<T>
 
    - Use `decltype( public api call )` to avoid maintaining internal typenames.
      We only do this in one place.
 
""_attr names:
 
    - Change from `StringData` to `const char*`, as fmt::arg needs NUL termination.
 
 
`fmt::dynamic_format_arg_store`:
 
    - Replace `fmt::internal::make_arg` with `fmt::arg`
 
    - In our formatters and extractors, replace
      handrolled containers of `fmt::basic_format_arg` with
      `fmt::dynamic_format_arg_store`.
 
    - dynamic_format_argument_store copies argument values, but supports
      wrapping arguments with std::cref to opt-out of this. Then peformance
      is comparable to before. It makes string copies of attribute names, but
      we can avoid even that cost by pushing only a reference_wrapper to the named_arg,
      which avoid the name copying.
 
    - Remove to_string_view(StringData) as it aggravates libfmt issue #2180
      (dynamic_format_arg_store) https://github.com/fmtlib/fmt/issues/2180 ,
      and issue #2181 (FMT_COMPILE) https://github.com/fmtlib/fmt/issues/2181 .
      We don't need it and can define a fmt::formatter<StringData> instead.
      A side-effect is that being implicitly convertible to StringData doesn't
      make a type formattable anymore. This was an issue with e.g. ShardId.

Comment by Billy Donahue [ 14/Mar/21 ]

I picked this back up and finished it.
Lots of notes about the roadblocks and solutions are in the CR desc.
CR https://mongodbcr.appspot.com/739640001/

Comment by Billy Donahue [ 08/Feb/21 ]

sara.golemon sgolemon, I happened to do a fair amount of work on this ticket yesterday just for Sunday Shiggles.
I can share my notes with you if you want to chat about it.
The short story is that it's way harder than I thought, mostly because of logv2 using the fmt API "in a manner inconsistent with its labeling".

Comment by Billy Donahue [ 13/Jul/20 ]

There's a fmtlib v7.0.1 as of 2020-07-07.
https://github.com/fmtlib/fmt/blob/master/ChangeLog.rst#701---2020-07-07

It will be a little more work to import it but looks very worthwhile.

Comment by Billy Donahue [ 21/Apr/20 ]

While writing that up, I thought it might be easier to just try it than to write up my guesses about what it would be like to try it. It's all my own import scripts so there's no learning curve for me to do it.

PQ: https://evergreen.mongodb.com/version/5e9f5a5b32f4170a47d26b36

I built the "core" scons metatarget and ran the logv2_test successfully.
We're probably fine to just take the new version, at least on master.

CR: https://mongodbcr.appspot.com/579040002/

Here's a deliberate attempt to fmt::format an unformattable type:

src/mongo/logv2/logv2_test.cpp:1628:24: required from here
src/third_party/fmt/dist/include/fmt/core.h:1016:9: error: static assertion failed: Cannot format argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#formatting-user-defined-types
 formattable,
 ^~~~~~~~~~~
ninja: build stopped: subcommand failed.

Which is is a big improvement.

Comment by Billy Donahue [ 21/Apr/20 ]

For reference, we're currently on libfmt 6.1.1.

They've moved on to 6.2.0 by now.
https://github.com/fmtlib/fmt/blob/master/ChangeLog.rst

There's some good stuff in there. No emergencies.

6.2.0 -  Added experimental dynamic argument storage (#1170, #1584): This could be interesting for us. I think we're doing this using "fmt::internal" names and it could be an upgrade to do what we need to do in assembling format args without breaking the namespace barrier.

Build failure diagnostics are improved. This could be pretty significant. If you've ever looked at a failed format call, it's very messy. Devs are confronted with these error messes when they write broken LOGV2 statements. The upgrade could help us out there.

There are some subtle numeric rounding fixes as well.

As a very recent import, upgrading fmt is easier than most of third_party has been.
You have to tweak the FMT_GIT_REV variable in src/third_party/fmt/scripts/import.sh and run it.

Comment by Mira Carey [ 21/Apr/20 ]

This is just a ticket to look at release notes and sum up if anything interesting came up

Generated at Thu Feb 08 05:08:49 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.