Skip to content

Commit 45052f1

Browse files
lukevalentylukevalenty
authored
Merge pull request #698 from elbeno/fix-mipi-arg-encoding
🐛 Allow 64-bit values in MIPI arg encoding
2 parents a6181e4 + 68b18cd commit 45052f1

File tree

10 files changed

+178
-64
lines changed

10 files changed

+178
-64
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/catalog.hpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,8 @@ using module_id = std::uint32_t;
1515

1616
template <typename> extern auto catalog() -> string_id;
1717
template <typename> extern auto module() -> module_id;
18+
19+
struct encode_32;
20+
struct encode_64;
21+
struct encode_u32;
22+
struct encode_u64;

‎include/log/catalog/mipi_builder.hpp‎

Lines changed: 68 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,111 @@
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::int32_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 encoding;
33+
34+
template <signed_packable T> struct encoding<T> {
35+
using encode_t = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
36+
encode_32, encode_64>;
37+
using pack_t = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
38+
std::int32_t, std::int64_t>;
39+
};
40+
41+
template <unsigned_packable T> struct encoding<T> {
42+
using encode_t = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
43+
encode_u32, encode_u64>;
44+
using pack_t = stdx::conditional_t<sizeof(T) <= sizeof(std::uint32_t),
45+
std::uint32_t, std::uint64_t>;
46+
};
47+
48+
template <enum_packable T>
49+
struct encoding<T> : encoding<stdx::underlying_type_t<T>> {};
50+
51+
template <packable T> using pack_as_t = typename encoding<T>::pack_t;
52+
template <packable T> using encode_as_t = typename encoding<T>::encode_t;
53+
1654
template <typename> struct builder;
1755

1856
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...) {
57+
template <auto Level> static auto build(string_id id, module_id) {
2158
using namespace msg;
2259
return owning<defn::short32_msg_t>{"payload"_field = id};
2360
}
2461
};
2562

2663
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) {
64+
template <auto Level, packable... Ts>
65+
static auto build(string_id id, module_id m, Ts... args) {
2966
using namespace msg;
3067
defn::catalog_msg_t::owner_t<Storage> message{"severity"_field = Level,
3168
"module_id"_field = m};
3269

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-
};
70+
using V = typename Storage::value_type;
71+
constexpr auto header_size = defn::catalog_msg_t::size<V>::value;
4672

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};
73+
auto const pack_arg = []<typename T>(V *p, T arg) -> V * {
74+
auto const packed = stdx::to_le(stdx::as_unsigned(
75+
static_cast<pack_as_t<T>>(stdx::to_underlying(arg))));
76+
std::memcpy(p, &packed, sizeof(packed));
77+
return p + stdx::sized8{sizeof(packed)}.in<V>();
78+
};
5579

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

6284
return message;
6385
}
6486
};
6587

6688
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) {
89+
template <auto Level, typename... Ts>
90+
static auto build(string_id id, module_id m, Ts... args) {
6991
using namespace msg;
70-
if constexpr (sizeof...(Ts) <= 2u) {
92+
if constexpr ((0 + ... + sizeof(Ts)) <= sizeof(std::uint32_t) * 2) {
7193
constexpr auto header_size =
7294
defn::catalog_msg_t::size<std::uint32_t>::value;
73-
constexpr auto payload_len = 1 + sizeof...(Ts);
95+
constexpr auto payload_size =
96+
stdx::sized8{(sizeof(id) + ... + sizeof(pack_as_t<Ts>))}
97+
.in<std::uint32_t>();
7498
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...);
99+
std::array<std::uint32_t, header_size + payload_size>;
100+
return catalog_builder<storage_t>{}.template build<Level>(id, m,
101+
args...);
78102
} else {
79103
constexpr auto header_size =
80104
defn::catalog_msg_t::size<std::uint8_t>::value;
81-
constexpr auto payload_len = (sizeof(id) + ... + sizeof(Ts));
105+
constexpr auto payload_size = (sizeof(id) + ... + sizeof(Ts));
82106
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...);
107+
std::array<std::uint8_t, header_size + payload_size>;
108+
return catalog_builder<storage_t>{}.template build<Level>(id, m,
109+
args...);
86110
}
87111
}
88112
};
89113

90114
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) {
115+
template <auto Level, packable... Ts>
116+
static auto build(string_id id, module_id m, Ts... args) {
93117
if constexpr (sizeof...(Ts) == 0u) {
94-
return builder<defn::short32_msg_t>{}.template build<Level>(
95-
id, m, msg_data...);
118+
return builder<defn::short32_msg_t>{}.template build<Level>(id, m);
96119
} else {
97120
return builder<defn::catalog_msg_t>{}.template build<Level>(
98-
id, m, msg_data...);
121+
id, m, args...);
99122
}
100123
}
101124
};

‎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<encode_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: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ 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;
28+
auto log_one_formatted_rt_arg() -> void;
2729
auto log_with_non_default_module_id() -> void;
2830
auto log_with_fixed_module_id() -> void;
2931

@@ -38,9 +40,22 @@ auto log_one_ct_arg() -> void {
3840
format("B string with {} placeholder"_sc, "one"_sc));
3941
}
4042

41-
auto log_one_rt_arg() -> void {
43+
auto log_one_32bit_rt_arg() -> void {
4244
auto cfg = logging::mipi::config{test_log_args_destination{}};
43-
cfg.logger.log_msg<log_env1>(format("C string with {} placeholder"_sc, 1));
45+
cfg.logger.log_msg<log_env1>(
46+
format("C1 string with {} placeholder"_sc, std::int32_t{1}));
47+
}
48+
49+
auto log_one_64bit_rt_arg() -> void {
50+
auto cfg = logging::mipi::config{test_log_args_destination{}};
51+
cfg.logger.log_msg<log_env1>(
52+
format("C2 string with {} placeholder"_sc, std::int64_t{1}));
53+
}
54+
55+
auto log_one_formatted_rt_arg() -> void {
56+
auto cfg = logging::mipi::config{test_log_args_destination{}};
57+
cfg.logger.log_msg<log_env1>(
58+
format("C3 string with {:08x} placeholder"_sc, std::int32_t{1}));
4459
}
4560

4661
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: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ 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;
17+
extern auto log_one_formatted_rt_arg() -> void;
1618
extern auto log_two_rt_args() -> void;
1719
extern auto log_rt_enum_arg() -> void;
1820
extern auto log_with_non_default_module_id() -> void;
@@ -38,10 +40,26 @@ TEST_CASE("log one compile-time argument", "[catalog]") {
3840
CHECK(last_header >> 4u != 0);
3941
}
4042

41-
TEST_CASE("log one runtime argument", "[catalog]") {
43+
TEST_CASE("log one 32-bit runtime argument", "[catalog]") {
4244
log_calls = 0;
4345
test_critical_section::count = 0;
44-
log_one_rt_arg();
46+
log_one_32bit_rt_arg();
47+
CHECK(test_critical_section::count == 2);
48+
CHECK(log_calls == 1);
49+
}
50+
51+
TEST_CASE("log one 64-bit runtime argument", "[catalog]") {
52+
log_calls = 0;
53+
test_critical_section::count = 0;
54+
log_one_64bit_rt_arg();
55+
CHECK(test_critical_section::count == 2);
56+
CHECK(log_calls == 1);
57+
}
58+
59+
TEST_CASE("log one formatted runtime argument", "[catalog]") {
60+
log_calls = 0;
61+
test_critical_section::count = 0;
62+
log_one_formatted_rt_arg();
4563
CHECK(test_critical_section::count == 2);
4664
CHECK(log_calls == 1);
4765
}
@@ -65,7 +83,7 @@ TEST_CASE("log runtime enum argument", "[catalog]") {
6583
TEST_CASE("log module ids change", "[catalog]") {
6684
// subtype 1, severity 7, type 3
6785
std::uint32_t expected_static = (1u << 24u) | (7u << 4u) | 3u;
68-
log_one_rt_arg();
86+
log_one_32bit_rt_arg();
6987
CHECK((last_header & expected_static) == expected_static);
7088

7189
auto default_header = last_header;

‎test/log/mipi_encoder.cpp‎

Lines changed: 41 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,34 @@ 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(
135+
std::same_as<logging::mipi::pack_as_t<std::int32_t>, std::int32_t>);
136+
static_assert(
137+
std::same_as<logging::mipi::pack_as_t<std::uint32_t>, std::uint32_t>);
138+
static_assert(
139+
std::same_as<logging::mipi::pack_as_t<std::int64_t>, std::int64_t>);
140+
static_assert(
141+
std::same_as<logging::mipi::pack_as_t<std::uint64_t>, std::uint64_t>);
142+
static_assert(std::same_as<logging::mipi::pack_as_t<char>, std::int32_t>);
143+
static_assert(
144+
std::same_as<logging::mipi::pack_as_t<unsigned char>, std::uint32_t>);
145+
}
146+
147+
TEST_CASE("argument encoding", "[mipi]") {
148+
static_assert(
149+
std::same_as<logging::mipi::encode_as_t<std::int32_t>, encode_32>);
150+
static_assert(
151+
std::same_as<logging::mipi::encode_as_t<std::uint32_t>, encode_u32>);
152+
static_assert(
153+
std::same_as<logging::mipi::encode_as_t<std::int64_t>, encode_64>);
154+
static_assert(
155+
std::same_as<logging::mipi::encode_as_t<std::uint64_t>, encode_u64>);
156+
static_assert(std::same_as<logging::mipi::encode_as_t<char>, encode_32>);
157+
static_assert(
158+
std::same_as<logging::mipi::encode_as_t<unsigned char>, encode_u32>);
159+
}
160+
132161
TEST_CASE("log zero arguments", "[mipi]") {
133162
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
134163
test_critical_section::count = 0;
@@ -138,7 +167,7 @@ TEST_CASE("log zero arguments", "[mipi]") {
138167
CHECK(test_critical_section::count == 2);
139168
}
140169

141-
TEST_CASE("log one argument", "[mipi]") {
170+
TEST_CASE("log one 32-bit argument", "[mipi]") {
142171
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
143172
test_critical_section::count = 0;
144173
auto cfg = logging::mipi::config{
@@ -147,6 +176,17 @@ TEST_CASE("log one argument", "[mipi]") {
147176
CHECK(test_critical_section::count == 2);
148177
}
149178

179+
TEST_CASE("log one 64-bit argument", "[mipi]") {
180+
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
181+
test_critical_section::count = 0;
182+
auto x = std::uint64_t{0x1234'5678'90ab'cdefull};
183+
auto cfg = logging::mipi::config{
184+
test_log_args_destination<logging::level::TRACE, 42u, 0x90ab'cdefu,
185+
0x1234'5678u>{}};
186+
cfg.logger.log_msg<log_env>(format("{}"_sc, x));
187+
CHECK(test_critical_section::count == 2);
188+
}
189+
150190
TEST_CASE("log two arguments", "[mipi]") {
151191
CIB_LOG_ENV(logging::get_level, logging::level::TRACE);
152192
test_critical_section::count = 0;

0 commit comments

Comments
 (0)