From d18cd9ac5279d44b04fe1056a44561f341332ffe Mon Sep 17 00:00:00 2001 From: Jacob Ledbetter Date: Wed, 7 Oct 2026 11:17:02 -0600 Subject: [PATCH 1/7] fix(ini): Preserve legacy parsing for oversized integers --- Core/GameEngine/Source/Common/INI/INI.cpp | 15 ++++- Tests/Google/Core/CMakeLists.txt | 1 + .../Google/Core/GameEngine/Common/INITest.cpp | 60 +++++++++++++++++++ 3 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 Tests/Google/Core/GameEngine/Common/INITest.cpp diff --git a/Core/GameEngine/Source/Common/INI/INI.cpp b/Core/GameEngine/Source/Common/INI/INI.cpp index a30d5b03cfe..f873d0f8f51 100644 --- a/Core/GameEngine/Source/Common/INI/INI.cpp +++ b/Core/GameEngine/Source/Common/INI/INI.cpp @@ -1611,8 +1611,9 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList #if USE_STD_FROM_CHARS_PARSING template -Type scanType(std::string_view token) +Type scanType(const char* tokenString) { + std::string_view token(tokenString); DEBUG_ASSERTCRASH(!token.empty(), ("token is not expected to be empty")); // Unlike sscanf, std::from_chars cannot parse "+". @@ -1626,6 +1627,18 @@ Type scanType(std::string_view token) std::conditional_t, Int64, Type> result{}; const auto [ptr, ec] = std::from_chars(token.data(), token.data() + token.size(), result); + if constexpr (std::is_integral_v) + { + // TheSuperHackers @bugfix Preserve legacy INI parsing for integers outside the Int64 range. + if (ec == std::errc::result_out_of_range) + { + Type value; + if (sscanf(tokenString, std::is_signed_v ? "%d" : "%u", &value) != 1) + throw INI_INVALID_DATA; + return value; + } + } + if (ec != std::errc{}) { throw INI_INVALID_DATA; diff --git a/Tests/Google/Core/CMakeLists.txt b/Tests/Google/Core/CMakeLists.txt index f4a850622cf..45353c8a5c3 100644 --- a/Tests/Google/Core/CMakeLists.txt +++ b/Tests/Google/Core/CMakeLists.txt @@ -2,6 +2,7 @@ set(COREI_GOOGLETEST_SRC GoogleTestMain.cpp GameEngine/Common/AsciiStringTest.cpp + GameEngine/Common/INITest.cpp Libraries/Compression/CompressionManagerTest.cpp ) diff --git a/Tests/Google/Core/GameEngine/Common/INITest.cpp b/Tests/Google/Core/GameEngine/Common/INITest.cpp new file mode 100644 index 00000000000..2b833be9fb6 --- /dev/null +++ b/Tests/Google/Core/GameEngine/Common/INITest.cpp @@ -0,0 +1,60 @@ +/* +** Command & Conquer Generals Zero Hour(tm) +** Copyright 2026 TheSuperHackers +** +** This program is free software: you can redistribute it and/or modify +** it under the terms of the GNU General Public License as published by +** the Free Software Foundation, either version 3 of the License, or +** (at your option) any later version. +** +** This program is distributed in the hope that it will be useful, +** but WITHOUT ANY WARRANTY; without even the implied warranty of +** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +** GNU General Public License for more details. +** +** You should have received a copy of the GNU General Public License +** along with this program. If not, see . +*/ + +#include +#include + +#include "Common/INI.h" + +TEST(INI, OversizedIntegersMatchLegacyParser) +{ + const char* tokens[] = { + "9999999999999999999", "+9999999999999999999", + "-9999999999999999999", "9999999999999999999suffix", + "9223372036854775807", "-9223372036854775808", + "9223372036854775808", "-9223372036854775809", + "18446744073709551616", "-18446744073709551616", + "99999999999999999999999999999999999999999999999999" + }; + for (const char* token : tokens) + { + SCOPED_TRACE(token); + // Preserve the old parser's behavior on this runtime, not a portable overflow policy. + Int signedExpected = 0; + UnsignedInt unsignedExpected = 0; + ASSERT_EQ(std::sscanf(token, "%d", &signedExpected), 1); + ASSERT_EQ(std::sscanf(token, "%u", &unsignedExpected), 1); + EXPECT_EQ(INI::scanInt(token), signedExpected); + EXPECT_EQ(INI::scanUnsignedInt(token), unsignedExpected); + } +} + +TEST(INI, InvalidIntegersAreRejected) +{ + for (const char* token : {"abc", "+", "-", "+-9999999999999999999"}) + { + SCOPED_TRACE(token); + EXPECT_THROW(INI::scanInt(token), decltype(INI_INVALID_DATA)); + EXPECT_THROW(INI::scanUnsignedInt(token), decltype(INI_INVALID_DATA)); + } +} + +TEST(INI, RealOverflowIsRejected) +{ + EXPECT_THROW(INI::scanReal("1e100"), decltype(INI_INVALID_DATA)); +} From 12f56bdbba036c7b5f671a80ccca77b8a4e3722f Mon Sep 17 00:00:00 2001 From: Jacob Ledbetter Date: Wed, 7 Oct 2026 21:30:27 -0600 Subject: [PATCH 2/7] fix(ini): undo undefined behavior --- Core/GameEngine/Source/Common/INI/INI.cpp | 10 ++++- .../Google/Core/GameEngine/Common/INITest.cpp | 38 ++++++++++--------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/Core/GameEngine/Source/Common/INI/INI.cpp b/Core/GameEngine/Source/Common/INI/INI.cpp index f873d0f8f51..bdd483ce36c 100644 --- a/Core/GameEngine/Source/Common/INI/INI.cpp +++ b/Core/GameEngine/Source/Common/INI/INI.cpp @@ -67,6 +67,7 @@ #if USE_STD_FROM_CHARS_PARSING #include +#include #include #include #endif @@ -1629,11 +1630,16 @@ Type scanType(const char* tokenString) if constexpr (std::is_integral_v) { - // TheSuperHackers @bugfix Preserve legacy INI parsing for integers outside the Int64 range. + // TheSuperHackers @bugfix Match Windows legacy parsing with defined 64-bit saturation before narrowing. if (ec == std::errc::result_out_of_range) { + char* end; Type value; - if (sscanf(tokenString, std::is_signed_v ? "%d" : "%u", &value) != 1) + if constexpr (std::is_signed_v) + value = static_cast(std::strtoll(tokenString, &end, 10)); + else + value = static_cast(std::strtoull(tokenString, &end, 10)); + if (end == tokenString) throw INI_INVALID_DATA; return value; } diff --git a/Tests/Google/Core/GameEngine/Common/INITest.cpp b/Tests/Google/Core/GameEngine/Common/INITest.cpp index 2b833be9fb6..adea66f7062 100644 --- a/Tests/Google/Core/GameEngine/Common/INITest.cpp +++ b/Tests/Google/Core/GameEngine/Common/INITest.cpp @@ -16,31 +16,35 @@ ** along with this program. If not, see . */ -#include #include #include "Common/INI.h" TEST(INI, OversizedIntegersMatchLegacyParser) { - const char* tokens[] = { - "9999999999999999999", "+9999999999999999999", - "-9999999999999999999", "9999999999999999999suffix", - "9223372036854775807", "-9223372036854775808", - "9223372036854775808", "-9223372036854775809", - "18446744073709551616", "-18446744073709551616", - "99999999999999999999999999999999999999999999999999" + const struct { + const char* token; + Int signedExpected; + UnsignedInt unsignedExpected; + } cases[] = { + {"9999999999999999999", -1, 2313682943u}, + {"+9999999999999999999", -1, 2313682943u}, + {"-9999999999999999999", 0, 1981284353u}, + {"9999999999999999999suffix", -1, 2313682943u}, + {"9223372036854775807", -1, 4294967295u}, + {"-9223372036854775808", 0, 0u}, + {"9223372036854775808", -1, 0u}, + {"-9223372036854775809", 0, 4294967295u}, + {"18446744073709551616", -1, 4294967295u}, + {"-18446744073709551616", 0, 4294967295u}, + {"99999999999999999999999999999999999999999999999999", -1, 4294967295u} }; - for (const char* token : tokens) + // Windows legacy parsing saturates at 64 bits, then keeps the low 32 bits. + for (const auto& test : cases) { - SCOPED_TRACE(token); - // Preserve the old parser's behavior on this runtime, not a portable overflow policy. - Int signedExpected = 0; - UnsignedInt unsignedExpected = 0; - ASSERT_EQ(std::sscanf(token, "%d", &signedExpected), 1); - ASSERT_EQ(std::sscanf(token, "%u", &unsignedExpected), 1); - EXPECT_EQ(INI::scanInt(token), signedExpected); - EXPECT_EQ(INI::scanUnsignedInt(token), unsignedExpected); + SCOPED_TRACE(test.token); + EXPECT_EQ(INI::scanInt(test.token), test.signedExpected); + EXPECT_EQ(INI::scanUnsignedInt(test.token), test.unsignedExpected); } } From 60ad4db41872ba7b6697a1add7a23ff11f69e5a3 Mon Sep 17 00:00:00 2001 From: Jacob Ledbetter Date: Thu, 8 Oct 2026 18:55:24 -0600 Subject: [PATCH 3/7] style(ini): Address parser review readability comments --- Core/GameEngine/Source/Common/INI/INI.cpp | 3 +++ .../Google/Core/GameEngine/Common/INITest.cpp | 20 +++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/Core/GameEngine/Source/Common/INI/INI.cpp b/Core/GameEngine/Source/Common/INI/INI.cpp index bdd483ce36c..177e983aaa9 100644 --- a/Core/GameEngine/Source/Common/INI/INI.cpp +++ b/Core/GameEngine/Source/Common/INI/INI.cpp @@ -1614,6 +1614,7 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList template Type scanType(const char* tokenString) { + // strto* needs the original null-terminated token, including any leading '+'. std::string_view token(tokenString); DEBUG_ASSERTCRASH(!token.empty(), ("token is not expected to be empty")); @@ -1639,8 +1640,10 @@ Type scanType(const char* tokenString) value = static_cast(std::strtoll(tokenString, &end, 10)); else value = static_cast(std::strtoull(tokenString, &end, 10)); + if (end == tokenString) throw INI_INVALID_DATA; + return value; } } diff --git a/Tests/Google/Core/GameEngine/Common/INITest.cpp b/Tests/Google/Core/GameEngine/Common/INITest.cpp index adea66f7062..72806c3717e 100644 --- a/Tests/Google/Core/GameEngine/Common/INITest.cpp +++ b/Tests/Google/Core/GameEngine/Common/INITest.cpp @@ -27,16 +27,16 @@ TEST(INI, OversizedIntegersMatchLegacyParser) Int signedExpected; UnsignedInt unsignedExpected; } cases[] = { - {"9999999999999999999", -1, 2313682943u}, - {"+9999999999999999999", -1, 2313682943u}, - {"-9999999999999999999", 0, 1981284353u}, - {"9999999999999999999suffix", -1, 2313682943u}, - {"9223372036854775807", -1, 4294967295u}, - {"-9223372036854775808", 0, 0u}, - {"9223372036854775808", -1, 0u}, - {"-9223372036854775809", 0, 4294967295u}, - {"18446744073709551616", -1, 4294967295u}, - {"-18446744073709551616", 0, 4294967295u}, + {"9999999999999999999", -1, 2313682943u}, + {"+9999999999999999999", -1, 2313682943u}, + {"-9999999999999999999", 0, 1981284353u}, + {"9999999999999999999suffix", -1, 2313682943u}, + {"9223372036854775807", -1, 4294967295u}, + {"-9223372036854775808", 0, 0u}, + {"9223372036854775808", -1, 0u}, + {"-9223372036854775809", 0, 4294967295u}, + {"18446744073709551616", -1, 4294967295u}, + {"-18446744073709551616", 0, 4294967295u}, {"99999999999999999999999999999999999999999999999999", -1, 4294967295u} }; // Windows legacy parsing saturates at 64 bits, then keeps the low 32 bits. From 2d5b2fb3600e56a281e5a48b21282389b4c932c5 Mon Sep 17 00:00:00 2001 From: Jacob Ledbetter Date: Fri, 9 Oct 2026 18:16:28 -0600 Subject: [PATCH 4/7] fix(ini): Preserve legacy floating-point range handling --- Core/GameEngine/Source/Common/INI/INI.cpp | 45 +++++++-------- .../Google/Core/GameEngine/Common/INITest.cpp | 56 ++++++++++++++++++- 2 files changed, 74 insertions(+), 27 deletions(-) diff --git a/Core/GameEngine/Source/Common/INI/INI.cpp b/Core/GameEngine/Source/Common/INI/INI.cpp index 177e983aaa9..fe4199df425 100644 --- a/Core/GameEngine/Source/Common/INI/INI.cpp +++ b/Core/GameEngine/Source/Common/INI/INI.cpp @@ -1612,40 +1612,35 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList #if USE_STD_FROM_CHARS_PARSING template -Type scanType(const char* tokenString) +Type scanType(std::string_view token) { - // strto* needs the original null-terminated token, including any leading '+'. - std::string_view token(tokenString); DEBUG_ASSERTCRASH(!token.empty(), ("token is not expected to be empty")); // Unlike sscanf, std::from_chars cannot parse "+". - // Consume the plus symbol to accommodate custom ini files that have numbers prefixed with a plus. - if (token[0] == '+') - { - token.remove_prefix(1); - } + // Skip it for from_chars while preserving the original token for the fallback. + const size_t tokenOffset = (token[0] == '+'); // Unlike sscanf, std::from_chars cannot parse "-" as unsigned integer. std::conditional_t, Int64, Type> result{}; - const auto [ptr, ec] = std::from_chars(token.data(), token.data() + token.size(), result); - - if constexpr (std::is_integral_v) - { - // TheSuperHackers @bugfix Match Windows legacy parsing with defined 64-bit saturation before narrowing. - if (ec == std::errc::result_out_of_range) - { - char* end; - Type value; - if constexpr (std::is_signed_v) - value = static_cast(std::strtoll(tokenString, &end, 10)); - else - value = static_cast(std::strtoull(tokenString, &end, 10)); + const auto [ptr, ec] = std::from_chars(token.data() + tokenOffset, token.data() + token.size(), result); + + // TheSuperHackers @bugfix Preserve legacy range handling with defined strto* conversions. + if (ec == std::errc::result_out_of_range) + { + // All callers pass null-terminated INI tokens. + char* end; + Type value; + if constexpr (std::is_floating_point_v) + value = std::strtof(token.data(), &end); + else if constexpr (std::is_signed_v) + value = static_cast(std::strtoll(token.data(), &end, 10)); + else + value = static_cast(std::strtoull(token.data(), &end, 10)); - if (end == tokenString) - throw INI_INVALID_DATA; + if (end == token.data()) + throw INI_INVALID_DATA; - return value; - } + return value; } if (ec != std::errc{}) diff --git a/Tests/Google/Core/GameEngine/Common/INITest.cpp b/Tests/Google/Core/GameEngine/Common/INITest.cpp index 72806c3717e..56d58cab6c1 100644 --- a/Tests/Google/Core/GameEngine/Common/INITest.cpp +++ b/Tests/Google/Core/GameEngine/Common/INITest.cpp @@ -16,6 +16,9 @@ ** along with this program. If not, see . */ +#include +#include +#include #include #include "Common/INI.h" @@ -45,6 +48,15 @@ TEST(INI, OversizedIntegersMatchLegacyParser) SCOPED_TRACE(test.token); EXPECT_EQ(INI::scanInt(test.token), test.signedExpected); EXPECT_EQ(INI::scanUnsignedInt(test.token), test.unsignedExpected); +#ifdef _WIN32 + // Characterize the Windows CRT; out-of-range scanf conversions are not portable. + Int legacySigned; + UnsignedInt legacyUnsigned; + ASSERT_EQ(std::sscanf(test.token, "%d", &legacySigned), 1); + ASSERT_EQ(std::sscanf(test.token, "%u", &legacyUnsigned), 1); + EXPECT_EQ(legacySigned, test.signedExpected); + EXPECT_EQ(legacyUnsigned, test.unsignedExpected); +#endif } } @@ -58,7 +70,47 @@ TEST(INI, InvalidIntegersAreRejected) } } -TEST(INI, RealOverflowIsRejected) +TEST(INI, RealRangeErrorsMatchLegacyParser) { - EXPECT_THROW(INI::scanReal("1e100"), decltype(INI_INVALID_DATA)); + const Real infinity = std::numeric_limits::infinity(); + const Real subnormal = std::numeric_limits::denorm_min(); + const struct { + const char* token; + Real expected; + } cases[] = { + {"1e100", infinity}, + {"+1e100", infinity}, + {"-1e100", -infinity}, + {"3.5e38", infinity}, + {"1e100suffix", infinity}, + {"1e-100", 0.0f}, + {"+1e-100", 0.0f}, + {"-1e-100", -0.0f}, + {"1e-45", subnormal}, + {"-1e-45", -subnormal}, + {"+1.25", 1.25f} + }; + for (const auto& test : cases) + { + SCOPED_TRACE(test.token); + const Real actual = INI::scanReal(test.token); + EXPECT_EQ(actual, test.expected); + EXPECT_EQ(std::signbit(actual), std::signbit(test.expected)); +#ifdef _WIN32 + // As above, scanf is a Windows compatibility check, not a portable overflow oracle. + Real legacy; + ASSERT_EQ(std::sscanf(test.token, "%f", &legacy), 1); + EXPECT_EQ(legacy, test.expected); + EXPECT_EQ(std::signbit(legacy), std::signbit(test.expected)); +#endif + } +} + +TEST(INI, InvalidRealsAreRejected) +{ + for (const char* token : {"abc", "+", "-", "+-1e100", "+-1e-100"}) + { + SCOPED_TRACE(token); + EXPECT_THROW(INI::scanReal(token), decltype(INI_INVALID_DATA)); + } } From 4f68d8fa20932c7036b86797876a1442d6fe670a Mon Sep 17 00:00:00 2001 From: Jacob Ledbetter Date: Sat, 10 Oct 2026 08:14:37 -0600 Subject: [PATCH 5/7] fix(ini): Reject double signs before numeric parsing --- Core/GameEngine/Source/Common/INI/INI.cpp | 16 +++++++++------- Tests/Google/Core/GameEngine/Common/INITest.cpp | 10 ++++++++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/Core/GameEngine/Source/Common/INI/INI.cpp b/Core/GameEngine/Source/Common/INI/INI.cpp index fe4199df425..4a7b68b86b3 100644 --- a/Core/GameEngine/Source/Common/INI/INI.cpp +++ b/Core/GameEngine/Source/Common/INI/INI.cpp @@ -1612,32 +1612,34 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList #if USE_STD_FROM_CHARS_PARSING template -Type scanType(std::string_view token) +Type scanType(const char* tokenString) { + std::string_view token(tokenString); DEBUG_ASSERTCRASH(!token.empty(), ("token is not expected to be empty")); // Unlike sscanf, std::from_chars cannot parse "+". // Skip it for from_chars while preserving the original token for the fallback. const size_t tokenOffset = (token[0] == '+'); + if (tokenOffset && token.size() > 1 && token[1] == '-') + throw INI_INVALID_DATA; // Unlike sscanf, std::from_chars cannot parse "-" as unsigned integer. std::conditional_t, Int64, Type> result{}; const auto [ptr, ec] = std::from_chars(token.data() + tokenOffset, token.data() + token.size(), result); - // TheSuperHackers @bugfix Preserve legacy range handling with defined strto* conversions. + // Preserve legacy range handling with defined strto* conversions. if (ec == std::errc::result_out_of_range) { - // All callers pass null-terminated INI tokens. char* end; Type value; if constexpr (std::is_floating_point_v) - value = std::strtof(token.data(), &end); + value = std::strtof(tokenString, &end); else if constexpr (std::is_signed_v) - value = static_cast(std::strtoll(token.data(), &end, 10)); + value = static_cast(std::strtoll(tokenString, &end, 10)); else - value = static_cast(std::strtoull(token.data(), &end, 10)); + value = static_cast(std::strtoull(tokenString, &end, 10)); - if (end == token.data()) + if (end == tokenString) throw INI_INVALID_DATA; return value; diff --git a/Tests/Google/Core/GameEngine/Common/INITest.cpp b/Tests/Google/Core/GameEngine/Common/INITest.cpp index 56d58cab6c1..2c507e33d07 100644 --- a/Tests/Google/Core/GameEngine/Common/INITest.cpp +++ b/Tests/Google/Core/GameEngine/Common/INITest.cpp @@ -62,11 +62,15 @@ TEST(INI, OversizedIntegersMatchLegacyParser) TEST(INI, InvalidIntegersAreRejected) { - for (const char* token : {"abc", "+", "-", "+-9999999999999999999"}) + for (const char* token : {"abc", "+", "-", "++1", "+-99999999", "+-9999999999999999999"}) { SCOPED_TRACE(token); EXPECT_THROW(INI::scanInt(token), decltype(INI_INVALID_DATA)); EXPECT_THROW(INI::scanUnsignedInt(token), decltype(INI_INVALID_DATA)); + Int legacySigned; + UnsignedInt legacyUnsigned; + EXPECT_NE(std::sscanf(token, "%d", &legacySigned), 1); + EXPECT_NE(std::sscanf(token, "%u", &legacyUnsigned), 1); } } @@ -108,9 +112,11 @@ TEST(INI, RealRangeErrorsMatchLegacyParser) TEST(INI, InvalidRealsAreRejected) { - for (const char* token : {"abc", "+", "-", "+-1e100", "+-1e-100"}) + for (const char* token : {"abc", "+", "-", "++1.25", "+-1.25", "+-1e100", "+-1e-100"}) { SCOPED_TRACE(token); EXPECT_THROW(INI::scanReal(token), decltype(INI_INVALID_DATA)); + Real legacy; + EXPECT_NE(std::sscanf(token, "%f", &legacy), 1); } } From d322999119c761948925636b6df6a527697f468c Mon Sep 17 00:00:00 2001 From: CryoTheRenegade Date: Mon, 12 Oct 2026 16:27:06 -0600 Subject: [PATCH 6/7] perf(ini): Add numeric parser benchmarks against sscanf --- Benchmarks/Google/Core/CMakeLists.txt | 1 + .../Core/GameEngine/Common/INIBenchmark.cpp | 87 +++++++++++++++++++ TESTING.md | 10 +++ 3 files changed, 98 insertions(+) create mode 100644 Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp diff --git a/Benchmarks/Google/Core/CMakeLists.txt b/Benchmarks/Google/Core/CMakeLists.txt index d18c2ea05fa..329026c30ed 100644 --- a/Benchmarks/Google/Core/CMakeLists.txt +++ b/Benchmarks/Google/Core/CMakeLists.txt @@ -2,6 +2,7 @@ set(COREI_GOOGLEBENCHMARK_SRC GoogleBenchmarkMain.cpp GameEngine/Common/AsciiStringBenchmark.cpp + GameEngine/Common/INIBenchmark.cpp Libraries/Compression/CompressionManagerBenchmark.cpp ) diff --git a/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp b/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp new file mode 100644 index 00000000000..dc920268d2e --- /dev/null +++ b/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp @@ -0,0 +1,87 @@ +/* +** Command & Conquer Generals Zero Hour(tm) +** Copyright 2026 TheSuperHackers +** +** This program is free software: you can redistribute it and/or modify +** it under the terms of the GNU General Public License as published by +** the Free Software Foundation, either version 3 of the License, or +** (at your option) any later version. +** +** This program is distributed in the hope that it will be useful, +** but WITHOUT ANY WARRANTY; without even the implied warranty of +** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +** GNU General Public License for more details. +** +** You should have received a copy of the GNU General Public License +** along with this program. If not, see . +*/ + +#include +#include +#include + +#include "Common/INI.h" +#include "Precompiled/CppMacros.h" + +// Keep the original conversion and failure check, with a call boundary like the production scanners. +// Out-of-range sscanf results characterize the local CRT, not portable C behavior. +template +NOINLINE static Type scanLegacy(const char* token) +{ + Type value; + const char* format = std::is_same_v ? "%f" : std::is_signed_v ? "%d" : "%u"; + if (std::sscanf(token, format, &value) != 1) + throw INI_INVALID_DATA; + return value; +} + +// Each iteration parses four fixed tokens. These are microbenchmarks, not a distribution of game INI inputs. +// Input 0 uses the fast path, 1 overflows, and 2 underflows (Real only). +template +static void BM_INIScan(benchmark::State &state) +{ + const char* const integerTokens[][4] = { + {"0", "+42", "-1000", "2147483647"}, + {"9999999999999999999", "+9999999999999999999", "-9999999999999999999", "18446744073709551616"} + }; + const char* const realTokens[][4] = { + {"0.0", "+1.25", "-1000.5", "3.14159265"}, + {"1e100", "+1e100", "-1e100", "3.5e38"}, + {"1e-100", "+1e-100", "-1e-100", "1e-46"} + }; + const auto& tokens = std::is_same_v ? realTokens[Input] : integerTokens[Input]; + + for (auto _ : state) + { + for (const char* token : tokens) + { + benchmark::DoNotOptimize(token); + Type value; + if constexpr (Legacy) + value = scanLegacy(token); + else if constexpr (std::is_same_v) + value = INI::scanReal(token); + else if constexpr (std::is_signed_v) + value = INI::scanInt(token); + else + value = INI::scanUnsignedInt(token); + benchmark::DoNotOptimize(value); + } + } + state.SetItemsProcessed(state.iterations() * 4); +} + +BENCHMARK_TEMPLATE(BM_INIScan, Int, false, 0)->Name("INI/Int/Ordinary/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, Int, true, 0)->Name("INI/Int/Ordinary/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, Int, false, 1)->Name("INI/Int/Overflow/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, Int, true, 1)->Name("INI/Int/Overflow/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, false, 0)->Name("INI/UnsignedInt/Ordinary/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, true, 0)->Name("INI/UnsignedInt/Ordinary/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, false, 1)->Name("INI/UnsignedInt/Overflow/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, true, 1)->Name("INI/UnsignedInt/Overflow/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, Real, false, 0)->Name("INI/Real/Ordinary/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, Real, true, 0)->Name("INI/Real/Ordinary/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, Real, false, 1)->Name("INI/Real/Overflow/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, Real, true, 1)->Name("INI/Real/Overflow/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, Real, false, 2)->Name("INI/Real/Underflow/Current"); +BENCHMARK_TEMPLATE(BM_INIScan, Real, true, 2)->Name("INI/Real/Underflow/Legacy"); diff --git a/TESTING.md b/TESTING.md index 8389081dbb9..fdb60e9ffe7 100644 --- a/TESTING.md +++ b/TESTING.md @@ -45,3 +45,13 @@ The test executables use the regular user data folder of the game, so a `RELEASE Benchmarks use [Google Benchmark](https://github.com/TheSuperHackers/google-benchmark). Enable them with `RTS_BUILD_OPTION_BENCHMARKS`. They are organized like the unit tests under Benchmarks/Google, with the executables g_googlebenchmark and z_googlebenchmark. CI builds them, but does not run them. Run benchmarks from a Release build. Select benchmarks with `--benchmark_filter=`, and use `--benchmark_out=.json` to compare the results before and after a change. + +The `INI/` benchmarks compare the production numeric scanners with the original `sscanf` conversion and failure check. Ordinary inputs exercise `from_chars`; overflow and underflow inputs exercise the range fallback. Each iteration parses four fixed tokens, so divide the reported CPU time by four for ns/token. `items_per_second` already counts individual tokens. Range-error `sscanf` results characterize the local CRT and are not portable C behavior. These measurements do not represent game startup time or the frequency of range errors in game data. + +For example, from an x86 developer shell: +``` +cmake --preset win32 -DRTS_BUILD_OPTION_BENCHMARKS=ON +cmake --build --preset win32 --target z_googlebenchmark g_googlebenchmark +build/win32/Benchmarks/Google/Release/z_googlebenchmark.exe --benchmark_filter=^INI/ --benchmark_min_time=0.2s --benchmark_repetitions=5 --benchmark_enable_random_interleaving=true --benchmark_out=ini-benchmark.json --benchmark_out_format=json +``` +Use the median CPU time for each case when comparing repeated runs. The same filter also works with `g_googlebenchmark.exe`. Relative output paths are resolved from the executable's directory; use an absolute `--benchmark_out` path to save elsewhere. From abd35951bc0f24f397b73ccedacb64be6645057a Mon Sep 17 00:00:00 2001 From: CryoTheRenegade Date: Tue, 13 Oct 2026 12:11:41 -0600 Subject: [PATCH 7/7] perf(ini): Address benchmark review comments Co-authored-by: Cursor --- .../Core/GameEngine/Common/INIBenchmark.cpp | 48 ++++++++++++------- TESTING.md | 4 +- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp b/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp index dc920268d2e..45f51fe072c 100644 --- a/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp +++ b/Benchmarks/Google/Core/GameEngine/Common/INIBenchmark.cpp @@ -21,7 +21,20 @@ #include #include "Common/INI.h" -#include "Precompiled/CppMacros.h" + +enum ScanParser +{ + ScanParser_Current, + ScanParser_Legacy, +}; + +// Ordinary inputs use the fast path. Overflow and underflow (Real only) use the range fallback. +enum ScanInput +{ + ScanInput_Ordinary, + ScanInput_Overflow, + ScanInput_Underflow, +}; // Keep the original conversion and failure check, with a call boundary like the production scanners. // Out-of-range sscanf results characterize the local CRT, not portable C behavior. @@ -36,8 +49,7 @@ NOINLINE static Type scanLegacy(const char* token) } // Each iteration parses four fixed tokens. These are microbenchmarks, not a distribution of game INI inputs. -// Input 0 uses the fast path, 1 overflows, and 2 underflows (Real only). -template +template static void BM_INIScan(benchmark::State &state) { const char* const integerTokens[][4] = { @@ -57,7 +69,7 @@ static void BM_INIScan(benchmark::State &state) { benchmark::DoNotOptimize(token); Type value; - if constexpr (Legacy) + if constexpr (Parser == ScanParser_Legacy) value = scanLegacy(token); else if constexpr (std::is_same_v) value = INI::scanReal(token); @@ -71,17 +83,17 @@ static void BM_INIScan(benchmark::State &state) state.SetItemsProcessed(state.iterations() * 4); } -BENCHMARK_TEMPLATE(BM_INIScan, Int, false, 0)->Name("INI/Int/Ordinary/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, Int, true, 0)->Name("INI/Int/Ordinary/Legacy"); -BENCHMARK_TEMPLATE(BM_INIScan, Int, false, 1)->Name("INI/Int/Overflow/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, Int, true, 1)->Name("INI/Int/Overflow/Legacy"); -BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, false, 0)->Name("INI/UnsignedInt/Ordinary/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, true, 0)->Name("INI/UnsignedInt/Ordinary/Legacy"); -BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, false, 1)->Name("INI/UnsignedInt/Overflow/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, true, 1)->Name("INI/UnsignedInt/Overflow/Legacy"); -BENCHMARK_TEMPLATE(BM_INIScan, Real, false, 0)->Name("INI/Real/Ordinary/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, Real, true, 0)->Name("INI/Real/Ordinary/Legacy"); -BENCHMARK_TEMPLATE(BM_INIScan, Real, false, 1)->Name("INI/Real/Overflow/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, Real, true, 1)->Name("INI/Real/Overflow/Legacy"); -BENCHMARK_TEMPLATE(BM_INIScan, Real, false, 2)->Name("INI/Real/Underflow/Current"); -BENCHMARK_TEMPLATE(BM_INIScan, Real, true, 2)->Name("INI/Real/Underflow/Legacy"); +BENCHMARK_TEMPLATE(BM_INIScan, Int, ScanParser_Current, ScanInput_Ordinary); +BENCHMARK_TEMPLATE(BM_INIScan, Int, ScanParser_Legacy, ScanInput_Ordinary); +BENCHMARK_TEMPLATE(BM_INIScan, Int, ScanParser_Current, ScanInput_Overflow); +BENCHMARK_TEMPLATE(BM_INIScan, Int, ScanParser_Legacy, ScanInput_Overflow); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, ScanParser_Current, ScanInput_Ordinary); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, ScanParser_Legacy, ScanInput_Ordinary); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, ScanParser_Current, ScanInput_Overflow); +BENCHMARK_TEMPLATE(BM_INIScan, UnsignedInt, ScanParser_Legacy, ScanInput_Overflow); +BENCHMARK_TEMPLATE(BM_INIScan, Real, ScanParser_Current, ScanInput_Ordinary); +BENCHMARK_TEMPLATE(BM_INIScan, Real, ScanParser_Legacy, ScanInput_Ordinary); +BENCHMARK_TEMPLATE(BM_INIScan, Real, ScanParser_Current, ScanInput_Overflow); +BENCHMARK_TEMPLATE(BM_INIScan, Real, ScanParser_Legacy, ScanInput_Overflow); +BENCHMARK_TEMPLATE(BM_INIScan, Real, ScanParser_Current, ScanInput_Underflow); +BENCHMARK_TEMPLATE(BM_INIScan, Real, ScanParser_Legacy, ScanInput_Underflow); diff --git a/TESTING.md b/TESTING.md index fdb60e9ffe7..bb58d2591c2 100644 --- a/TESTING.md +++ b/TESTING.md @@ -46,12 +46,12 @@ Benchmarks use [Google Benchmark](https://github.com/TheSuperHackers/google-benc Run benchmarks from a Release build. Select benchmarks with `--benchmark_filter=`, and use `--benchmark_out=.json` to compare the results before and after a change. -The `INI/` benchmarks compare the production numeric scanners with the original `sscanf` conversion and failure check. Ordinary inputs exercise `from_chars`; overflow and underflow inputs exercise the range fallback. Each iteration parses four fixed tokens, so divide the reported CPU time by four for ns/token. `items_per_second` already counts individual tokens. Range-error `sscanf` results characterize the local CRT and are not portable C behavior. These measurements do not represent game startup time or the frequency of range errors in game data. +The `BM_INIScan` benchmarks compare the production numeric scanners with the original `sscanf` conversion and failure check. Ordinary inputs exercise `from_chars`; overflow and underflow inputs exercise the range fallback. Each iteration parses four fixed tokens, so divide the reported CPU time by four for ns/token. `items_per_second` already counts individual tokens. Range-error `sscanf` results characterize the local CRT and are not portable C behavior. These measurements do not represent game startup time or the frequency of range errors in game data. For example, from an x86 developer shell: ``` cmake --preset win32 -DRTS_BUILD_OPTION_BENCHMARKS=ON cmake --build --preset win32 --target z_googlebenchmark g_googlebenchmark -build/win32/Benchmarks/Google/Release/z_googlebenchmark.exe --benchmark_filter=^INI/ --benchmark_min_time=0.2s --benchmark_repetitions=5 --benchmark_enable_random_interleaving=true --benchmark_out=ini-benchmark.json --benchmark_out_format=json +build/win32/Benchmarks/Google/Release/z_googlebenchmark.exe --benchmark_filter=^BM_INIScan --benchmark_min_time=0.2s --benchmark_repetitions=5 --benchmark_enable_random_interleaving=true --benchmark_out=ini-benchmark.json --benchmark_out_format=json ``` Use the median CPU time for each case when comparing repeated runs. The same filter also works with `g_googlebenchmark.exe`. Relative output paths are resolved from the executable's directory; use an absolute `--benchmark_out` path to save elsewhere.