Skip to content

Commit 378b488

Browse files
elbenoelbeno
committed
🐛 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.
1 parent e896707 commit 378b488

File tree

9 files changed

+135
-65
lines changed

9 files changed

+135
-65
lines changed

‎cmake/string_catalog.cmake‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ function(gen_str_catalog)
2525
if(${lib_type} STREQUAL OBJECT_LIBRARY)
2626
add_custom_command(
2727
OUTPUT ${UNDEF}
28-
DEPENDS ${LIB}
28+
DEPENDS $<TARGET_OBJECTS:${LIB}>
2929
COMMAND ${CMAKE_NM} -uC "$<TARGET_OBJECTS:${LIB}>" > "${UNDEF}"
3030
COMMAND_EXPAND_LISTS)
3131
else()
@@ -82,6 +82,7 @@ function(gen_str_catalog)
8282
${GUID_MASK_ARG} ${MODULE_ID_MAX_ARG}
8383
DEPENDS ${UNDEFS} ${INPUT_JSON} ${SC_GEN_STR_CATALOG} ${STABLE_JSON}
8484
COMMAND_EXPAND_LISTS)
85+
8586
if(SC_OUTPUT_LIB)
8687
add_library(${SC_OUTPUT_LIB} STATIC ${SC_OUTPUT_CPP})
8788
target_link_libraries(${SC_OUTPUT_LIB} PUBLIC cib)

‎include/log/catalog/mipi_builder.hpp‎

Lines changed: 66 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include <log/catalog/mipi_messages.hpp>
55

66
#include <stdx/compiler.hpp>
7+
#include <stdx/type_traits.hpp>
78
#include <stdx/utility.hpp>
89

910
#include <array>
@@ -13,89 +14,109 @@
1314
#include <utility>
1415

1516
namespace logging::mipi {
17+
template <typename T>
18+
concept signed_packable =
19+
std::signed_integral<T> and sizeof(T) <= sizeof(std::int64_t);
20+
21+
template <typename T>
22+
concept unsigned_packable =
23+
std::unsigned_integral<T> and sizeof(T) <= sizeof(std::int64_t);
24+
25+
template <typename T>
26+
concept enum_packable = std::is_enum_v<T> and sizeof(T) <= sizeof(std::int64_t);
27+
28+
template <typename T>
29+
concept packable =
30+
signed_packable<T> or unsigned_packable<T> or enum_packable<T>;
31+
32+
template <typename T> struct pack_as;
33+
34+
template <signed_packable T> struct pack_as<T> {
35+
using type =
36+
// NOLINTNEXTLINE(google-runtime-int)
37+
stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t), int, long long>;
38+
};
39+
40+
template <unsigned_packable T> struct pack_as<T> {
41+
using type = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
42+
// NOLINTNEXTLINE(google-runtime-int)
43+
unsigned int, unsigned long long>;
44+
};
45+
46+
template <enum_packable T>
47+
requires(sizeof(T) <= sizeof(std::int32_t))
48+
struct pack_as<T> : pack_as<stdx::underlying_type_t<T>> {};
49+
50+
template <packable T> using pack_as_t = typename pack_as<T>::type;
51+
1652
template <typename> struct builder;
1753

1854
template <> struct builder<defn::short32_msg_t> {
19-
template <auto Level, std::same_as<std::uint32_t>... Ts>
20-
static auto build(string_id id, module_id, Ts...) {
55+
template <auto Level> static auto build(string_id id, module_id) {
2156
using namespace msg;
2257
return owning<defn::short32_msg_t>{"payload"_field = id};
2358
}
2459
};
2560

2661
template <typename Storage> struct catalog_builder {
27-
template <auto Level, std::same_as<std::uint32_t>... Ts>
28-
static auto build(string_id id, module_id m, Ts... msg_data) {
62+
template <auto Level, packable... Ts>
63+
static auto build(string_id id, module_id m, Ts... args) {
2964
using namespace msg;
3065
defn::catalog_msg_t::owner_t<Storage> message{"severity"_field = Level,
3166
"module_id"_field = m};
3267

33-
constexpr auto header_size =
34-
defn::catalog_msg_t::size<typename Storage::value_type>::value;
35-
constexpr auto copy_arg = [](std::uint32_t arg, auto &dest) {
36-
std::memcpy(dest, &arg, sizeof(std::uint32_t));
37-
dest += sizeof(std::uint32_t);
38-
};
39-
auto dest = &message.data()[header_size];
40-
copy_arg(stdx::to_le(id), dest);
41-
(copy_arg(stdx::to_le(msg_data), dest), ...);
42-
43-
return message;
44-
}
45-
};
68+
using V = typename Storage::value_type;
69+
constexpr auto header_size = defn::catalog_msg_t::size<V>::value;
4670

47-
template <typename Storage>
48-
requires std::same_as<typename Storage::value_type, std::uint32_t>
49-
struct catalog_builder<Storage> {
50-
template <auto Level, std::same_as<std::uint32_t>... Ts>
51-
static auto build(string_id id, module_id m, Ts... msg_data) {
52-
using namespace msg;
53-
defn::catalog_msg_t::owner_t<Storage> message{"severity"_field = Level,
54-
"module_id"_field = m};
71+
auto const pack_arg = []<typename T>(V *p, T arg) -> V * {
72+
auto const packed = stdx::to_le(stdx::as_unsigned(
73+
static_cast<pack_as_t<T>>(stdx::to_underlying(arg))));
74+
std::memcpy(p, &packed, sizeof(packed));
75+
return p + stdx::sized8{sizeof(packed)}.in<V>();
76+
};
5577

56-
constexpr auto header_size =
57-
defn::catalog_msg_t::size<std::uint32_t>::value;
5878
auto dest = &message.data()[header_size];
59-
*dest++ = stdx::to_le(id);
60-
((*dest++ = stdx::to_le(msg_data)), ...);
79+
dest = pack_arg(dest, stdx::to_le(id));
80+
((dest = pack_arg(dest, args)), ...);
6181

6282
return message;
6383
}
6484
};
6585

6686
template <> struct builder<defn::catalog_msg_t> {
67-
template <auto Level, std::same_as<std::uint32_t>... Ts>
68-
static auto build(string_id id, module_id m, Ts... msg_data) {
87+
template <auto Level, typename... Ts>
88+
static auto build(string_id id, module_id m, Ts... args) {
6989
using namespace msg;
70-
if constexpr (sizeof...(Ts) <= 2u) {
90+
if constexpr ((0 + ... + sizeof(Ts)) <= sizeof(std::uint32_t) * 2) {
7191
constexpr auto header_size =
7292
defn::catalog_msg_t::size<std::uint32_t>::value;
73-
constexpr auto payload_len = 1 + sizeof...(Ts);
93+
constexpr auto payload_size =
94+
stdx::sized8{(sizeof(id) + ... + sizeof(pack_as_t<Ts>))}
95+
.in<std::uint32_t>();
7496
using storage_t =
75-
std::array<std::uint32_t, header_size + payload_len>;
76-
return catalog_builder<storage_t>{}.template build<Level>(
77-
id, m, msg_data...);
97+
std::array<std::uint32_t, header_size + payload_size>;
98+
return catalog_builder<storage_t>{}.template build<Level>(id, m,
99+
args...);
78100
} else {
79101
constexpr auto header_size =
80102
defn::catalog_msg_t::size<std::uint8_t>::value;
81-
constexpr auto payload_len = (sizeof(id) + ... + sizeof(Ts));
103+
constexpr auto payload_size = (sizeof(id) + ... + sizeof(Ts));
82104
using storage_t =
83-
std::array<std::uint8_t, header_size + payload_len>;
84-
return catalog_builder<storage_t>{}.template build<Level>(
85-
id, m, msg_data...);
105+
std::array<std::uint8_t, header_size + payload_size>;
106+
return catalog_builder<storage_t>{}.template build<Level>(id, m,
107+
args...);
86108
}
87109
}
88110
};
89111

90112
struct default_builder {
91-
template <auto Level, std::same_as<std::uint32_t>... Ts>
92-
static auto build(string_id id, module_id m, Ts... msg_data) {
113+
template <auto Level, packable... Ts>
114+
static auto build(string_id id, module_id m, Ts... args) {
93115
if constexpr (sizeof...(Ts) == 0u) {
94-
return builder<defn::short32_msg_t>{}.template build<Level>(
95-
id, m, msg_data...);
116+
return builder<defn::short32_msg_t>{}.template build<Level>(id, m);
96117
} else {
97118
return builder<defn::catalog_msg_t>{}.template build<Level>(
98-
id, m, msg_data...);
119+
id, m, args...);
99120
}
100121
}
101122
};

‎include/log/catalog/mipi_encoder.hpp‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ template <typename S, typename... Args> constexpr auto to_message() {
2626
using char_t = typename std::remove_cv_t<decltype(s)>::value_type;
2727
return [&]<std::size_t... Is>(std::integer_sequence<std::size_t, Is...>) {
2828
return sc::message<
29-
sc::undefined<sc::args<Args...>, char_t, s[Is]...>>{};
29+
sc::undefined<sc::args<pack_as_t<Args>...>, char_t, s[Is]...>>{};
3030
}(std::make_integer_sequence<std::size_t, std::size(s)>{});
3131
}
3232

@@ -54,9 +54,8 @@ template <typename TDestinations> struct log_handler {
5454
using Module =
5555
decltype(detail::to_module<get_module(Env{}).value>());
5656
auto builder = get_builder(Env{}).value;
57-
write(
58-
builder.template build<L>(catalog<Message>(), module<Module>(),
59-
static_cast<std::uint32_t>(args)...));
57+
write(builder.template build<L>(catalog<Message>(),
58+
module<Module>(), args...));
6059
});
6160
}
6261

‎test/log/catalog1_lib.cpp‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ using log_env1 = logging::make_env_t<logging::get_level, logging::level::TRACE>;
2323

2424
auto log_zero_args() -> void;
2525
auto log_one_ct_arg() -> void;
26-
auto log_one_rt_arg() -> void;
26+
auto log_one_32bit_rt_arg() -> void;
27+
auto log_one_64bit_rt_arg() -> void;
2728
auto log_with_non_default_module_id() -> void;
2829
auto log_with_fixed_module_id() -> void;
2930

@@ -38,9 +39,16 @@ auto log_one_ct_arg() -> void {
3839
format("B string with {} placeholder"_sc, "one"_sc));
3940
}
4041

41-
auto log_one_rt_arg() -> void {
42+
auto log_one_32bit_rt_arg() -> void {
4243
auto cfg = logging::mipi::config{test_log_args_destination{}};
43-
cfg.logger.log_msg<log_env1>(format("C string with {} placeholder"_sc, 1));
44+
cfg.logger.log_msg<log_env1>(
45+
format("C1 string with {} placeholder"_sc, std::int32_t{1}));
46+
}
47+
48+
auto log_one_64bit_rt_arg() -> void {
49+
auto cfg = logging::mipi::config{test_log_args_destination{}};
50+
cfg.logger.log_msg<log_env1>(
51+
format("C2 string with {} placeholder"_sc, std::int64_t{1}));
4452
}
4553

4654
auto log_with_non_default_module_id() -> void {

‎test/log/catalog2a_lib.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ extern int log_calls;
1313
namespace {
1414
struct test_log_args_destination {
1515
auto log_by_args(std::uint32_t, auto...) -> void { ++log_calls; }
16+
template <std::size_t N>
17+
auto log_by_buf(stdx::span<std::uint8_t const, N>) const {
18+
++log_calls;
19+
}
1620
};
1721

1822
using log_env2a =
@@ -24,5 +28,6 @@ auto log_two_rt_args() -> void;
2428
auto log_two_rt_args() -> void {
2529
auto cfg = logging::mipi::config{test_log_args_destination{}};
2630
cfg.logger.log_msg<log_env2a>(
27-
format("D string with {} and {} placeholder"_sc, 1, 2));
31+
format("D string with {} and {} placeholder"_sc, std::uint32_t{1},
32+
std::int64_t{2}));
2833
}

‎test/log/catalog_app.cpp‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ extern int log_calls;
1212
extern std::uint32_t last_header;
1313
extern auto log_zero_args() -> void;
1414
extern auto log_one_ct_arg() -> void;
15-
extern auto log_one_rt_arg() -> void;
15+
extern auto log_one_32bit_rt_arg() -> void;
16+
extern auto log_one_64bit_rt_arg() -> void;
1617
extern auto log_two_rt_args() -> void;
1718
extern auto log_rt_enum_arg() -> void;
1819
extern auto log_with_non_default_module_id() -> void;
@@ -38,10 +39,18 @@ TEST_CASE("log one compile-time argument", "[catalog]") {
3839
CHECK(last_header >> 4u != 0);
3940
}
4041

41-
TEST_CASE("log one runtime argument", "[catalog]") {
42+
TEST_CASE("log one 32-bit runtime argument", "[catalog]") {
4243
log_calls = 0;
4344
test_critical_section::count = 0;
44-
log_one_rt_arg();
45+
log_one_32bit_rt_arg();
46+
CHECK(test_critical_section::count == 2);
47+
CHECK(log_calls == 1);
48+
}
49+
50+
TEST_CASE("log one 64-bit runtime argument", "[catalog]") {
51+
log_calls = 0;
52+
test_critical_section::count = 0;
53+
log_one_64bit_rt_arg();
4554
CHECK(test_critical_section::count == 2);
4655
CHECK(log_calls == 1);
4756
}
@@ -65,7 +74,7 @@ TEST_CASE("log runtime enum argument", "[catalog]") {
6574
TEST_CASE("log module ids change", "[catalog]") {
6675
// subtype 1, severity 7, type 3
6776
std::uint32_t expected_static = (1u << 24u) | (7u << 4u) | 3u;
68-
log_one_rt_arg();
77+
log_one_32bit_rt_arg();
6978
CHECK((last_header & expected_static) == expected_static);
7079

7180
auto default_header = last_header;

‎test/log/mipi_encoder.cpp‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ struct test_conc_policy {
6666
int num_log_args_calls{};
6767

6868
constexpr auto check = [](auto value, auto expected) {
69+
// static_assert(std::is_same_v<decltype(value), decltype(expected)>);
6970
CHECK(value == expected);
7071
};
7172

@@ -129,6 +130,19 @@ using log_env = logging::make_env_t<logging::get_level, logging::level::TRACE>;
129130

130131
template <> inline auto conc::injected_policy<> = test_conc_policy{};
131132

133+
TEST_CASE("argument packing", "[mipi]") {
134+
static_assert(std::same_as<logging::mipi::pack_as_t<std::int32_t>, int>);
135+
static_assert(
136+
std::same_as<logging::mipi::pack_as_t<std::uint32_t>, unsigned int>);
137+
static_assert(
138+
std::same_as<logging::mipi::pack_as_t<std::int64_t>, long long>);
139+
static_assert(std::same_as<logging::mipi::pack_as_t<std::uint64_t>,
140+
unsigned long long>);
141+
static_assert(std::same_as<logging::mipi::pack_as_t<char>, int>);
142+
static_assert(
143+
std::same_as<logging::mipi::pack_as_t<unsigned char>, unsigned int>);
144+
}
145+
132146
TEST_CASE("log zero arguments", "[mipi]") {
133147
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
134148
test_critical_section::count = 0;
@@ -138,7 +152,7 @@ TEST_CASE("log zero arguments", "[mipi]") {
138152
CHECK(test_critical_section::count == 2);
139153
}
140154

141-
TEST_CASE("log one argument", "[mipi]") {
155+
TEST_CASE("log one 32-bit argument", "[mipi]") {
142156
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
143157
test_critical_section::count = 0;
144158
auto cfg = logging::mipi::config{
@@ -147,6 +161,17 @@ TEST_CASE("log one argument", "[mipi]") {
147161
CHECK(test_critical_section::count == 2);
148162
}
149163

164+
TEST_CASE("log one 64-bit argument", "[mipi]") {
165+
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
166+
test_critical_section::count = 0;
167+
auto x = std::uint64_t{0x1234'5678'90ab'cdefull};
168+
auto cfg = logging::mipi::config{
169+
test_log_args_destination<logging::level::TRACE, 42u, 0x90ab'cdefu,
170+
0x1234'5678u>{}};
171+
cfg.logger.log_msg<log_env>(format("{}"_sc, x));
172+
CHECK(test_critical_section::count == 2);
173+
}
174+
150175
TEST_CASE("log two arguments", "[mipi]") {
151176
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
152177
test_critical_section::count = 0;

‎test/log/stable_strings.json‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,13 @@
11
{
22
"messages": [
33
{
4-
"level": "TRACE",
54
"msg": "An old string that can't be reused",
65
"type": "msg",
76
"arg_types": [],
87
"arg_count": 0,
98
"id": 0
109
},
1110
{
12-
"level": "TRACE",
1311
"msg": "A string with no placeholders",
1412
"type": "msg",
1513
"arg_types": [],

‎tools/gen_str_catalog.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,10 @@ def serialize_modules(client_node: et.Element, modules):
255255
syst_module.text = f"<![CDATA[{m['string']}]]>"
256256

257257

258+
def arg_printf_spec(arg: str):
259+
printf_dict = {"long long": "%lld", "unsigned long long": "%llu"}
260+
return printf_dict.get(arg, "%u" if "unsigned" in arg else "%d")
261+
258262
def serialize_messages(short_node: et.Element, catalog_node: et.Element, messages):
259263
for msg in messages.values():
260264
syst_format = et.SubElement(
@@ -266,10 +270,10 @@ def serialize_messages(short_node: et.Element, catalog_node: et.Element, message
266270
syst_format.set(
267271
"EnumLookup", f'{msg["enum_lookup"][0]}:{msg["enum_lookup"][1]}'
268272
)
269-
270-
fmt_string = re.sub(r"{}", r"%d", msg["msg"])
271-
if msg["arg_count"] != 0:
272-
fmt_string = re.sub(r"{:(.*?)}", r"%\1", fmt_string)
273+
274+
fmt_string = msg["msg"]
275+
for arg in msg["arg_types"]:
276+
fmt_string = re.sub(r"{.*?}", arg_printf_spec(arg), fmt_string, count = 1)
273277
syst_format.text = f"<![CDATA[{fmt_string}]]>"
274278

275279

0 commit comments

Comments
 (0)