From ffd52a0d8e43aba34d8d87def77b073c9fd90a07 Mon Sep 17 00:00:00 2001 From: Robert O'Callahan Date: Thu, 31 Jul 2025 10:19:44 +0000 Subject: [PATCH] Making `stringf()` use the format conversion specs as-is without widening them. And make sure our fast-path for `%d` and `%u` narrows to `int` correctly. Resolves #5260 --- kernel/io.cc | 34 ++---------------- kernel/io.h | 2 +- tests/unit/kernel/ioTest.cc | 72 +++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 32 deletions(-) create mode 100644 tests/unit/kernel/ioTest.cc diff --git a/kernel/io.cc b/kernel/io.cc index 3deb54d86..9811919ba 100644 --- a/kernel/io.cc +++ b/kernel/io.cc @@ -405,37 +405,9 @@ std::string unescape_format_string(std::string_view fmt) static std::string string_view_stringf(std::string_view spec, ...) { - std::string fmt(spec); - char format_specifier = fmt[fmt.size() - 1]; - switch (format_specifier) { - case 'd': - case 'i': - case 'o': - case 'u': - case 'x': - case 'X': { - // Strip any length modifier off `fmt` - std::string long_fmt; - for (size_t i = 0; i + 1 < fmt.size(); ++i) { - char ch = fmt[i]; - if ((ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z')) { - break; - } - long_fmt.push_back(ch); - } - // Add `lld` or whatever - long_fmt += "ll"; - long_fmt.push_back(format_specifier); - fmt = long_fmt; - break; - } - default: - break; - } - va_list ap; va_start(ap, spec); - std::string result = vstringf(fmt.c_str(), ap); + std::string result = vstringf(std::string(spec).c_str(), ap); va_end(ap); return result; } @@ -464,7 +436,7 @@ void format_emit_long_long(std::string &result, std::string_view spec, int *dyna { if (spec == "%d") { // Format checking will have guaranteed num_dynamic_ints == 0. - result += std::to_string(arg); + result += std::to_string(static_cast(arg)); return; } format_emit_stringf(result, spec, dynamic_ints, num_dynamic_ints, arg); @@ -475,7 +447,7 @@ void format_emit_unsigned_long_long(std::string &result, std::string_view spec, { if (spec == "%u") { // Format checking will have guaranteed num_dynamic_ints == 0. - result += std::to_string(arg); + result += std::to_string(static_cast(arg)); return; } if (spec == "%c") { diff --git a/kernel/io.h b/kernel/io.h index ea2499e43..dafef8bfa 100644 --- a/kernel/io.h +++ b/kernel/io.h @@ -62,7 +62,7 @@ enum ConversionSpecifier : uint8_t CONVSPEC_UNSIGNED_INT, // Consumes a "double" CONVSPEC_DOUBLE, - // Consumes a "const char*" + // Consumes a "const char*" or other string type CONVSPEC_CHAR_PTR, // Consumes a "void*" CONVSPEC_VOID_PTR, diff --git a/tests/unit/kernel/ioTest.cc b/tests/unit/kernel/ioTest.cc new file mode 100644 index 000000000..7cb8498cb --- /dev/null +++ b/tests/unit/kernel/ioTest.cc @@ -0,0 +1,72 @@ +#include + +#include "kernel/io.h" + +YOSYS_NAMESPACE_BEGIN + +TEST(KernelStringfTest, integerTruncation) +{ + EXPECT_EQ(stringf("%d", 1LL << 32), "0"); + EXPECT_EQ(stringf("%u", 1LL << 32), "0"); + EXPECT_EQ(stringf("%x", 0xff12345678LL), "12345678"); + EXPECT_EQ(stringf("%hu", 0xff12345678LL), "22136"); +} + +TEST(KernelStringfTest, charFormat) +{ + EXPECT_EQ(stringf("%c", 256), std::string_view("\0", 1)); + EXPECT_EQ(stringf("%c", -1), "\377"); +} + +TEST(KernelStringfTest, floatFormat) +{ + EXPECT_EQ(stringf("%g", 1.0), "1"); +} + +TEST(KernelStringfTest, intToFloat) +{ + EXPECT_EQ(stringf("%g", 1), "1"); +} + +TEST(KernelStringfTest, floatToInt) +{ + EXPECT_EQ(stringf("%d", 1.0), "1"); + EXPECT_EQ(stringf("%d", -1.6), "-1"); +} + +TEST(KernelStringfTest, stringParam) +{ + EXPECT_EQ(stringf("%s", std::string("hello")), "hello"); +} + +TEST(KernelStringfTest, stringViewParam) +{ + EXPECT_EQ(stringf("%s", std::string_view("hello")), "hello"); +} + +TEST(KernelStringfTest, escapePercent) +{ + EXPECT_EQ(stringf("%%"), "%"); +} + +TEST(KernelStringfTest, trailingPercent) +{ + EXPECT_EQ(stringf("%"), "%"); +} + +TEST(KernelStringfTest, dynamicWidth) +{ + EXPECT_EQ(stringf("%*s", 8, "hello"), " hello"); +} + +TEST(KernelStringfTest, dynamicPrecision) +{ + EXPECT_EQ(stringf("%.*f", 4, 1.0), "1.0000"); +} + +TEST(KernelStringfTest, dynamicWidthAndPrecision) +{ + EXPECT_EQ(stringf("%*.*f", 8, 4, 1.0), " 1.0000"); +} + +YOSYS_NAMESPACE_END