🐛 Allow 64-bit values in MIPI arg encoding

Problem:
- All runtime arguments to logs are assumed by the mipi encoder to be
  `std::uint32_t`.
- In reality, the mipi spec supports 64-bit values as well. (See MIPI-SyS-T spec
  table 16.) In particular, logging a time value (from a
  `std::chrono::duration`) needs to account for 64 bits.

Solution:
- Pack integral values properly according to the spec. Anything 32 bits or less
  is packed as 32 bits. If it's 64-bits, it's packed that way.
- The XML output strings use the appropriate (printf-like) specifiers.
- The JSON output retains the C++ types, as does the generated C++.

Notes:
- Table 16 allows 64-bit integral values even when the catalog message subtype
  is *_P32.
- Floating point and pointer values are supported by MIPI but not by this code.
  Maybe in future, if there is a use case.

Author: elbeno

cmake/string_catalog.cmake

@@ -25,7 +25,7 @@ function(gen_str_catalog)
         if(${lib_type} STREQUAL OBJECT_LIBRARY)
             add_custom_command(
                 OUTPUT ${UNDEF}
-                DEPENDS ${LIB}
+                DEPENDS $<TARGET_OBJECTS:${LIB}>
                 COMMAND ${CMAKE_NM} -uC "$<TARGET_OBJECTS:${LIB}>" > "${UNDEF}"
                 COMMAND_EXPAND_LISTS)
         else()
@@ -82,6 +82,7 @@ function(gen_str_catalog)
             ${GUID_MASK_ARG} ${MODULE_ID_MAX_ARG}
         DEPENDS ${UNDEFS} ${INPUT_JSON} ${SC_GEN_STR_CATALOG} ${STABLE_JSON}
         COMMAND_EXPAND_LISTS)
+
     if(SC_OUTPUT_LIB)
         add_library(${SC_OUTPUT_LIB} STATIC ${SC_OUTPUT_CPP})
         target_link_libraries(${SC_OUTPUT_LIB} PUBLIC cib)

include/log/catalog/mipi_builder.hpp

@@ -4,6 +4,7 @@
 #include <log/catalog/mipi_messages.hpp>
 
 #include <stdx/compiler.hpp>
+#include <stdx/type_traits.hpp>
 #include <stdx/utility.hpp>
 
 #include <array>
@@ -13,89 +14,110 @@
 #include <utility>
 
 namespace logging::mipi {
+template <typename T>
+concept signed_packable =
+    std::signed_integral<T> and sizeof(T) <= sizeof(std::int64_t);
+
+template <typename T>
+concept unsigned_packable =
+    std::unsigned_integral<T> and sizeof(T) <= sizeof(std::int64_t);
+
+template <typename T>
+concept enum_packable = std::is_enum_v<T> and sizeof(T) <= sizeof(std::int64_t);
+
+template <typename T>
+concept packable =
+    signed_packable<T> or unsigned_packable<T> or enum_packable<T>;
+
+template <typename T> struct pack_as;
+
+template <signed_packable T> struct pack_as<T> {
+    using type =
+        // NOLINTNEXTLINE(google-runtime-int)
+        stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t), int, long long>;
+};
+
+template <unsigned_packable T> struct pack_as<T> {
+    using type = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
+                                     // NOLINTNEXTLINE(google-runtime-int)
+                                     unsigned int, unsigned long long>;
+};
+
+template <enum_packable T>
+    requires(sizeof(T) <= sizeof(std::int32_t))
+struct pack_as<T> : pack_as<stdx::underlying_type_t<T>> {};
+
+template <packable T> using pack_as_t = typename pack_as<T>::type;
+
 template <typename> struct builder;
 
 template <> struct builder<defn::short32_msg_t> {
-    template <auto Level, std::same_as<std::uint32_t>... Ts>
-    static auto build(string_id id, module_id, Ts...) {
+    template <auto Level> static auto build(string_id id, module_id) {
         using namespace msg;
         return owning<defn::short32_msg_t>{"payload"_field = id};
     }
 };
 
 template <typename Storage> struct catalog_builder {
-    template <auto Level, std::same_as<std::uint32_t>... Ts>
-    static auto build(string_id id, module_id m, Ts... msg_data) {
+    template <auto Level, packable... Ts>
+    static auto build(string_id id, module_id m, Ts... args) {
         using namespace msg;
         defn::catalog_msg_t::owner_t<Storage> message{"severity"_field = Level,
                                                       "module_id"_field = m};
 
         constexpr auto header_size =
             defn::catalog_msg_t::size<typename Storage::value_type>::value;
-        constexpr auto copy_arg = [](std::uint32_t arg, auto &dest) {
-            std::memcpy(dest, &arg, sizeof(std::uint32_t));
-            dest += sizeof(std::uint32_t);
-        };
-        auto dest = &message.data()[header_size];
-        copy_arg(stdx::to_le(id), dest);
-        (copy_arg(stdx::to_le(msg_data), dest), ...);
-
-        return message;
-    }
-};
 
-template <typename Storage>
-    requires std::same_as<typename Storage::value_type, std::uint32_t>
-struct catalog_builder<Storage> {
-    template <auto Level, std::same_as<std::uint32_t>... Ts>
-    static auto build(string_id id, module_id m, Ts... msg_data) {
-        using namespace msg;
-        defn::catalog_msg_t::owner_t<Storage> message{"severity"_field = Level,
-                                                      "module_id"_field = m};
+        using V = Storage::value_type;
+        auto const pack_arg = []<typename T>(V *p, T arg) -> V * {
+            auto const packed = stdx::to_le(stdx::as_unsigned(
+                static_cast<pack_as_t<T>>(stdx::to_underlying(arg))));
+            std::memcpy(p, &packed, sizeof(packed));
+            return p + stdx::sized8{sizeof(packed)}.in<V>();
+        };
 
-        constexpr auto header_size =
-            defn::catalog_msg_t::size<std::uint32_t>::value;
         auto dest = &message.data()[header_size];
-        *dest++ = stdx::to_le(id);
-        ((*dest++ = stdx::to_le(msg_data)), ...);
+        dest = pack_arg(dest, stdx::to_le(id));
+        ((dest = pack_arg(dest, args)), ...);
 
         return message;
     }
 };
 
 template <> struct builder<defn::catalog_msg_t> {
-    template <auto Level, std::same_as<std::uint32_t>... Ts>
-    static auto build(string_id id, module_id m, Ts... msg_data) {
+    template <auto Level, typename... Ts>
+    static auto build(string_id id, module_id m, Ts... args) {
         using namespace msg;
-        if constexpr (sizeof...(Ts) <= 2u) {
+        if constexpr ((0 + ... + sizeof(Ts)) <= sizeof(std::uint32_t) * 2) {
             constexpr auto header_size =
                 defn::catalog_msg_t::size<std::uint32_t>::value;
-            constexpr auto payload_len = 1 + sizeof...(Ts);
+            constexpr auto payload_size =
+                stdx::sized8{(sizeof(id) + ... + sizeof(pack_as_t<Ts>))}
+                    .in<std::uint32_t>();
             using storage_t =
-                std::array<std::uint32_t, header_size + payload_len>;
-            return catalog_builder<storage_t>{}.template build<Level>(
-                id, m, msg_data...);
+                std::array<std::uint32_t, header_size + payload_size>;
+            return catalog_builder<storage_t>{}.template build<Level>(id, m,
+                                                                      args...);
         } else {
             constexpr auto header_size =
                 defn::catalog_msg_t::size<std::uint8_t>::value;
-            constexpr auto payload_len = (sizeof(id) + ... + sizeof(Ts));
+            constexpr auto payload_size = (sizeof(id) + ... + sizeof(Ts));
             using storage_t =
-                std::array<std::uint8_t, header_size + payload_len>;
-            return catalog_builder<storage_t>{}.template build<Level>(
-                id, m, msg_data...);
+                std::array<std::uint8_t, header_size + payload_size>;
+            return catalog_builder<storage_t>{}.template build<Level>(id, m,
+                                                                      args...);
         }
     }
 };
 
 struct default_builder {
-    template <auto Level, std::same_as<std::uint32_t>... Ts>
-    static auto build(string_id id, module_id m, Ts... msg_data) {
+    template <auto Level, packable... Ts>
+    static auto build(string_id id, module_id m, Ts... args) {
         if constexpr (sizeof...(Ts) == 0u) {
-            return builder<defn::short32_msg_t>{}.template build<Level>(
-                id, m, msg_data...);
+            return builder<defn::short32_msg_t>{}.template build<Level>(id, m);
         } else {
             return builder<defn::catalog_msg_t>{}.template build<Level>(
-                id, m, msg_data...);
+                id, m, args...);
         }
     }
 };

include/log/catalog/mipi_encoder.hpp

@@ -26,7 +26,7 @@ template <typename S, typename... Args> constexpr auto to_message() {
     using char_t = typename std::remove_cv_t<decltype(s)>::value_type;
     return [&]<std::size_t... Is>(std::integer_sequence<std::size_t, Is...>) {
         return sc::message<
-            sc::undefined<sc::args<Args...>, char_t, s[Is]...>>{};
+            sc::undefined<sc::args<pack_as_t<Args>...>, char_t, s[Is]...>>{};
     }(std::make_integer_sequence<std::size_t, std::size(s)>{});
 }
 
@@ -54,9 +54,8 @@ template <typename TDestinations> struct log_handler {
             using Module =
                 decltype(detail::to_module<get_module(Env{}).value>());
             auto builder = get_builder(Env{}).value;
-            write(
-                builder.template build<L>(catalog<Message>(), module<Module>(),
-                                          static_cast<std::uint32_t>(args)...));
+            write(builder.template build<L>(catalog<Message>(),
+                                            module<Module>(), args...));
         });
     }
 

test/log/catalog1_lib.cpp

@@ -23,7 +23,8 @@ using log_env1 = logging::make_env_t<logging::get_level, logging::level::TRACE>;
 
 auto log_zero_args() -> void;
 auto log_one_ct_arg() -> void;
-auto log_one_rt_arg() -> void;
+auto log_one_32bit_rt_arg() -> void;
+auto log_one_64bit_rt_arg() -> void;
 auto log_with_non_default_module_id() -> void;
 auto log_with_fixed_module_id() -> void;
 
@@ -38,9 +39,16 @@ auto log_one_ct_arg() -> void {
         format("B string with {} placeholder"_sc, "one"_sc));
 }
 
-auto log_one_rt_arg() -> void {
+auto log_one_32bit_rt_arg() -> void {
     auto cfg = logging::mipi::config{test_log_args_destination{}};
-    cfg.logger.log_msg<log_env1>(format("C string with {} placeholder"_sc, 1));
+    cfg.logger.log_msg<log_env1>(
+        format("C1 string with {} placeholder"_sc, std::int32_t{1}));
+}
+
+auto log_one_64bit_rt_arg() -> void {
+    auto cfg = logging::mipi::config{test_log_args_destination{}};
+    cfg.logger.log_msg<log_env1>(
+        format("C2 string with {} placeholder"_sc, std::int64_t{1}));
 }
 
 auto log_with_non_default_module_id() -> void {

test/log/catalog2a_lib.cpp

@@ -13,6 +13,10 @@ extern int log_calls;
 namespace {
 struct test_log_args_destination {
     auto log_by_args(std::uint32_t, auto...) -> void { ++log_calls; }
+    template <std::size_t N>
+    auto log_by_buf(stdx::span<std::uint8_t const, N>) const {
+        ++log_calls;
+    }
 };
 
 using log_env2a =
@@ -24,5 +28,6 @@ auto log_two_rt_args() -> void;
 auto log_two_rt_args() -> void {
     auto cfg = logging::mipi::config{test_log_args_destination{}};
     cfg.logger.log_msg<log_env2a>(
-        format("D string with {} and {} placeholder"_sc, 1, 2));
+        format("D string with {} and {} placeholder"_sc, std::uint32_t{1},
+               std::int64_t{2}));
 }

test/log/catalog_app.cpp

@@ -12,7 +12,8 @@ extern int log_calls;
 extern std::uint32_t last_header;
 extern auto log_zero_args() -> void;
 extern auto log_one_ct_arg() -> void;
-extern auto log_one_rt_arg() -> void;
+extern auto log_one_32bit_rt_arg() -> void;
+extern auto log_one_64bit_rt_arg() -> void;
 extern auto log_two_rt_args() -> void;
 extern auto log_rt_enum_arg() -> void;
 extern auto log_with_non_default_module_id() -> void;
@@ -38,10 +39,18 @@ TEST_CASE("log one compile-time argument", "[catalog]") {
     CHECK(last_header >> 4u != 0);
 }
 
-TEST_CASE("log one runtime argument", "[catalog]") {
+TEST_CASE("log one 32-bit runtime argument", "[catalog]") {
     log_calls = 0;
     test_critical_section::count = 0;
-    log_one_rt_arg();
+    log_one_32bit_rt_arg();
+    CHECK(test_critical_section::count == 2);
+    CHECK(log_calls == 1);
+}
+
+TEST_CASE("log one 64-bit runtime argument", "[catalog]") {
+    log_calls = 0;
+    test_critical_section::count = 0;
+    log_one_64bit_rt_arg();
     CHECK(test_critical_section::count == 2);
     CHECK(log_calls == 1);
 }
@@ -65,7 +74,7 @@ TEST_CASE("log runtime enum argument", "[catalog]") {
 TEST_CASE("log module ids change", "[catalog]") {
     // subtype 1, severity 7, type 3
     std::uint32_t expected_static = (1u << 24u) | (7u << 4u) | 3u;
-    log_one_rt_arg();
+    log_one_32bit_rt_arg();
     CHECK((last_header & expected_static) == expected_static);
 
     auto default_header = last_header;

test/log/mipi_encoder.cpp

@@ -66,6 +66,7 @@ struct test_conc_policy {
 int num_log_args_calls{};
 
 constexpr auto check = [](auto value, auto expected) {
+    // static_assert(std::is_same_v<decltype(value), decltype(expected)>);
     CHECK(value == expected);
 };
 
@@ -129,6 +130,19 @@ using log_env = logging::make_env_t<logging::get_level, logging::level::TRACE>;
 
 template <> inline auto conc::injected_policy<> = test_conc_policy{};
 
+TEST_CASE("argument packing", "[mipi]") {
+    static_assert(std::same_as<logging::mipi::pack_as_t<std::int32_t>, int>);
+    static_assert(
+        std::same_as<logging::mipi::pack_as_t<std::uint32_t>, unsigned int>);
+    static_assert(
+        std::same_as<logging::mipi::pack_as_t<std::int64_t>, long long>);
+    static_assert(std::same_as<logging::mipi::pack_as_t<std::uint64_t>,
+                               unsigned long long>);
+    static_assert(std::same_as<logging::mipi::pack_as_t<char>, int>);
+    static_assert(
+        std::same_as<logging::mipi::pack_as_t<unsigned char>, unsigned int>);
+}
+
 TEST_CASE("log zero arguments", "[mipi]") {
     CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
     test_critical_section::count = 0;
@@ -138,7 +152,7 @@ TEST_CASE("log zero arguments", "[mipi]") {
     CHECK(test_critical_section::count == 2);
 }
 
-TEST_CASE("log one argument", "[mipi]") {
+TEST_CASE("log one 32-bit argument", "[mipi]") {
     CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
     test_critical_section::count = 0;
     auto cfg = logging::mipi::config{
@@ -147,6 +161,17 @@ TEST_CASE("log one argument", "[mipi]") {
     CHECK(test_critical_section::count == 2);
 }
 
+TEST_CASE("log one 64-bit argument", "[mipi]") {
+    CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
+    test_critical_section::count = 0;
+    auto x = std::uint64_t{0x1234'5678'90ab'cdefull};
+    auto cfg = logging::mipi::config{
+        test_log_args_destination<logging::level::TRACE, 42u, 0x90ab'cdefu,
+                                  0x1234'5678u>{}};
+    cfg.logger.log_msg<log_env>(format("{}"_sc, x));
+    CHECK(test_critical_section::count == 2);
+}
+
 TEST_CASE("log two arguments", "[mipi]") {
     CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
     test_critical_section::count = 0;

test/log/stable_strings.json

@@ -1,15 +1,13 @@
 {
     "messages": [
         {
-            "level": "TRACE",
             "msg": "An old string that can't be reused",
             "type": "msg",
             "arg_types": [],
             "arg_count": 0,
             "id": 0
         },
         {
-            "level": "TRACE",
             "msg": "A string with no placeholders",
             "type": "msg",
             "arg_types": [],

tools/gen_str_catalog.py

@@ -255,6 +255,10 @@ def serialize_modules(client_node: et.Element, modules):
         syst_module.text = f"<![CDATA[{m['string']}]]>"
 
 
+def arg_printf_spec(arg: str):
+    printf_dict = {"long long": "%lld", "unsigned long long": "%llu"}
+    return printf_dict.get(arg, "%u" if "unsigned" in arg else "%d")
+        
 def serialize_messages(short_node: et.Element, catalog_node: et.Element, messages):
     for msg in messages.values():
         syst_format = et.SubElement(
@@ -266,10 +270,10 @@ def serialize_messages(short_node: et.Element, catalog_node: et.Element, message
             syst_format.set(
                 "EnumLookup", f'{msg["enum_lookup"][0]}:{msg["enum_lookup"][1]}'
             )
-
-        fmt_string = re.sub(r"{}", r"%d", msg["msg"])
-        if msg["arg_count"] != 0:
-            fmt_string = re.sub(r"{:(.*?)}", r"%\1", fmt_string)
+            
+        fmt_string = msg["msg"]
+        for arg in msg["arg_types"]:
+            fmt_string = re.sub(r"{.*?}", arg_printf_spec(arg), fmt_string, count = 1)
         syst_format.text = f"<![CDATA[{fmt_string}]]>"