From 89efcaa10a29eb3008f0aa3218d3996388d6079d Mon Sep 17 00:00:00 2001 From: Matt Stern Date: Tue, 19 Nov 2024 14:48:27 -0500 Subject: [PATCH] [C++] Integrate std::span support for flyweight API The impetus was a bug that we ran into when writing a string-literal to a fixed-width char field: ```c++ flyweight.putFixedChar("hello"); ``` This is unsafe: - If the field size is less than 6, we overrun the buffer and corrupt it. - If the field size is more than 6, we don't zero pad the rest of it. Instead, we build on support for the std::string_view getters and setters, which do length checking. std::span generalizes this to fixed-width fields of all types. Notably, if the size of the std::span is knowable at compile time, we pay no runtime cost for the length checking, and we should get similar performance to the existing API which takes a raw pointer. Further, we add a sbetool option to disable accepting arrays by raw pointer, which should prevent memcpy operation without bounds checking. This is off by default to avoid a breaking change. --- .../sbe/generation/cpp/CppGenerator.java | 134 +++++++++++++++++- sbe-tool/src/test/cpp/CMakeLists.txt | 13 +- sbe-tool/src/test/cpp/CodeGenTest.cpp | 62 ++++++++ 3 files changed, 203 insertions(+), 6 deletions(-) diff --git a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/cpp/CppGenerator.java b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/cpp/CppGenerator.java index 8533a4df84..cdb6b798c8 100755 --- a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/cpp/CppGenerator.java +++ b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/cpp/CppGenerator.java @@ -49,6 +49,8 @@ public class CppGenerator implements CodeGenerator { private static final boolean DISABLE_IMPLICIT_COPYING = Boolean.parseBoolean( System.getProperty("sbe.cpp.disable.implicit.copying", "false")); + private static final boolean DISABLE_RAW_ARRAYS = Boolean.parseBoolean( + System.getProperty("sbe.cpp.disable.raw.arrays", "false")); private static final String BASE_INDENT = ""; private static final String INDENT = " "; private static final String TWO_INDENT = INDENT + INDENT; @@ -1612,6 +1614,23 @@ private static CharSequence generateArrayFieldNotPresentCondition(final int sinc sinceVersion); } + private static CharSequence generateSpanFieldNotPresentCondition( + final int sinceVersion, final String indent, final String cppTypeName) + { + if (0 == sinceVersion) + { + return ""; + } + + return String.format( + indent + " if (m_actingVersion < %1$d)\n" + + indent + " {\n" + + indent + " return std::span();\n" + + indent + " }\n\n", + sinceVersion, + cppTypeName); + } + private static CharSequence generateStringNotPresentCondition(final int sinceVersion, final String indent) { if (0 == sinceVersion) @@ -1681,10 +1700,16 @@ private CharSequence generateFileHeader( "#if __cplusplus >= 201703L\n" + "# include \n" + "# define SBE_NODISCARD [[nodiscard]]\n" + + "# define SBE_USE_STRING_VIEW 1\n" + "#else\n" + "# define SBE_NODISCARD\n" + "#endif\n\n" + + "#if __cplusplus >= 202002L\n" + + "# include \n" + + "# define SBE_USE_SPAN 1\n" + + "#endif\n\n" + + "#if !defined(__STDC_LIMIT_MACROS)\n" + "# define __STDC_LIMIT_MACROS 1\n" + "#endif\n\n" + @@ -2236,12 +2261,38 @@ private void generateArrayProperty( accessOrderListenerCall); new Formatter(sb).format("\n" + - indent + " %1$s &put%2$s(const char *const src)%7$s\n" + + indent + " #ifdef SBE_USE_SPAN\n" + + indent + " SBE_NODISCARD std::span get%1$sAsSpan() const%7$s\n" + + indent + " {\n" + + "%3$s" + + "%6$s" + + indent + " const char *buffer = m_buffer + m_offset + %4$d;\n" + + indent + " return std::span(reinterpret_cast(buffer), %2$d);\n" + + indent + " }\n" + + indent + " #endif\n", + toUpperFirstChar(propertyName), + arrayLength, + generateSpanFieldNotPresentCondition(propertyToken.version(), indent, cppTypeName), + offset, + cppTypeName, + accessOrderListenerCall, + noexceptDeclaration); + + new Formatter(sb).format("\n" + + indent + " #ifdef SBE_USE_SPAN\n" + + indent + " template \n" + + indent + " %1$s &put%2$s(std::span src)%7$s\n" + indent + " {\n" + + indent + " static_assert(N <= %5$d, \"array too large for put%2$s\");\n\n" + "%6$s" + - indent + " std::memcpy(m_buffer + m_offset + %3$d, src, sizeof(%4$s) * %5$d);\n" + + indent + " std::memcpy(m_buffer + m_offset + %3$d, src.data(), sizeof(%4$s) * N);\n" + + indent + " for (std::size_t start = N; start < %5$d; ++start)\n" + + indent + " {\n" + + indent + " m_buffer[m_offset + %3$d + start] = 0;\n" + + indent + " }\n\n" + indent + " return *this;\n" + - indent + " }\n", + indent + " }\n" + + indent + " #endif\n", containingClassName, toUpperFirstChar(propertyName), offset, @@ -2250,6 +2301,79 @@ private void generateArrayProperty( accessOrderListenerCall, noexceptDeclaration); + if (encodingToken.encoding().primitiveType() != PrimitiveType.CHAR) + { + new Formatter(sb).format("\n" + + indent + " #ifdef SBE_USE_SPAN\n" + + indent + " %1$s &put%2$s(std::span src)\n" + + indent + " {\n" + + indent + " const std::size_t srcLength = src.size();\n" + + indent + " if (srcLength > %5$d)\n" + + indent + " {\n" + + indent + " throw std::runtime_error(\"array too large for put%2$s [E106]\");\n" + + indent + " }\n\n" + + + "%6$s" + + indent + " std::memcpy(m_buffer + m_offset + %3$d, src.data(), sizeof(%4$s) * srcLength);\n" + + indent + " for (std::size_t start = srcLength; start < %5$d; ++start)\n" + + indent + " {\n" + + indent + " m_buffer[m_offset + %3$d + start] = 0;\n" + + indent + " }\n\n" + + indent + " return *this;\n" + + indent + " }\n" + + indent + " #endif\n", + containingClassName, + toUpperFirstChar(propertyName), + offset, + cppTypeName, + arrayLength, + accessOrderListenerCall); + } + + if (!DISABLE_RAW_ARRAYS) + { + new Formatter(sb).format("\n" + + indent + " #ifdef SBE_USE_SPAN\n" + + indent + " template \n" + + // If std::span is available, redirect string literals to the std::span-accepting overload, + // where we can do compile-time bounds checking. + indent + " %1$s &put%2$s(T&& src) %7$s requires\n" + + indent + " (std::is_pointer_v> &&\n" + + indent + " !std::is_array_v>)\n" + + indent + " #else\n" + + indent + " %1$s &put%2$s(const char *const src)%7$s\n" + + indent + " #endif\n" + + indent + " {\n" + + "%6$s" + + indent + " std::memcpy(m_buffer + m_offset + %3$d, src, sizeof(%4$s) * %5$d);\n" + + indent + " return *this;\n" + + indent + " }\n", + containingClassName, + toUpperFirstChar(propertyName), + offset, + cppTypeName, + arrayLength, + accessOrderListenerCall, + noexceptDeclaration); + + } + if (encodingToken.encoding().primitiveType() == PrimitiveType.CHAR) + { + // Resolve ambiguity of string literal arguments, which match both + // std::span and std::string_view overloads. + new Formatter(sb).format("\n" + + indent + " #ifdef SBE_USE_SPAN\n" + + indent + " template \n" + + indent + " %1$s &put%2$s(const char (&src)[N])%3$s\n" + + indent + " {\n" + + indent + " return put%2$s(std::span(src));\n" + + indent + " }\n" + + indent + " #endif\n", + containingClassName, + toUpperFirstChar(propertyName), + noexceptDeclaration); + } + if (arrayLength > 1 && arrayLength <= 4) { sb.append("\n").append(indent).append(" ") @@ -2310,7 +2434,7 @@ private void generateArrayProperty( generateJsonEscapedStringGetter(sb, encodingToken, indent, propertyName, accessOrderListenerCall); new Formatter(sb).format("\n" + - indent + " #if __cplusplus >= 201703L\n" + + indent + " #ifdef SBE_USE_STRING_VIEW\n" + indent + " SBE_NODISCARD std::string_view get%1$sAsStringView() const%6$s\n" + indent + " {\n" + "%4$s" + @@ -2332,7 +2456,7 @@ private void generateArrayProperty( noexceptDeclaration); new Formatter(sb).format("\n" + - indent + " #if __cplusplus >= 201703L\n" + + indent + " #ifdef SBE_USE_STRING_VIEW\n" + indent + " %1$s &put%2$s(const std::string_view str)\n" + indent + " {\n" + indent + " const std::size_t srcLength = str.length();\n" + diff --git a/sbe-tool/src/test/cpp/CMakeLists.txt b/sbe-tool/src/test/cpp/CMakeLists.txt index 1de7412623..b3271dade0 100644 --- a/sbe-tool/src/test/cpp/CMakeLists.txt +++ b/sbe-tool/src/test/cpp/CMakeLists.txt @@ -103,6 +103,11 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") sbe_test(DtoTest codecs) target_compile_features(DtoTest PRIVATE cxx_std_17) endif() + + # Check if the GCC version supports C++20 + if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "11.0") + target_compile_features(CodeGenTest PRIVATE cxx_std_20) + endif() endif() if (CMAKE_CXX_COMPILER_ID STREQUAL "CLang") @@ -111,12 +116,18 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "CLang") sbe_test(DtoTest codecs) target_compile_features(DtoTest PRIVATE cxx_std_17) endif() + + # Check if CLang version supports C++20 + if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "10.0") + target_compile_features(CodeGenTest PRIVATE cxx_std_20) + endif() endif() if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") - # Check if MSVC version supports C++17 + # Check if MSVC version supports C++17 / C++20 if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "19.14") sbe_test(DtoTest codecs) target_compile_options(DtoTest PRIVATE /std:c++17) + target_compile_options(CodeGenTest PRIVATE /std:c++20) endif() endif() diff --git a/sbe-tool/src/test/cpp/CodeGenTest.cpp b/sbe-tool/src/test/cpp/CodeGenTest.cpp index ea00b8120c..039620a6c1 100644 --- a/sbe-tool/src/test/cpp/CodeGenTest.cpp +++ b/sbe-tool/src/test/cpp/CodeGenTest.cpp @@ -1083,3 +1083,65 @@ TEST_F(CodeGenTest, shouldAllowForMultipleIterations) std::string passFive = walkCar(carDecoder); EXPECT_EQ(passOne, passFive); } + +#ifdef SBE_USE_STRING_VIEW +TEST_F(CodeGenTest, shouldBeAbleToUseStdStringViewMethods) +{ + std::string vehicleCode(VEHICLE_CODE, Car::vehicleCodeLength()); + + char buffer[BUFFER_LEN] = {}; + std::uint64_t baseOffset = MessageHeader::encodedLength(); + Car car; + car.wrapForEncode(buffer, baseOffset, sizeof(buffer)); + car.putVehicleCode(std::string_view(vehicleCode)); + + car.sbeRewind(); + EXPECT_EQ(car.getVehicleCodeAsStringView(), vehicleCode); +} +#endif + +#ifdef SBE_USE_SPAN +TEST_F(CodeGenTest, shouldBeAbleToUseStdSpanViewMethods) +{ + char buffer[BUFFER_LEN] = {}; + + std::uint64_t baseOffset = MessageHeader::encodedLength(); + Car car; + car.wrapForEncode(buffer, baseOffset, sizeof(buffer)); + + { + std::array numbers; + for (std::uint64_t i = 0; i < Car::someNumbersLength(); i++) + { + numbers[i] = static_cast(i); + } + car.putSomeNumbers(numbers); + } + + car.sbeRewind(); + + { + std::span numbers = car.getSomeNumbersAsSpan(); + ASSERT_EQ(numbers.size(), Car::someNumbersLength()); + for (std::uint64_t i = 0; i < numbers.size(); i++) + { + EXPECT_EQ(numbers[i], static_cast(i)); + } + } +} +#endif + +#ifdef SBE_USE_SPAN +TEST_F(CodeGenTest, shouldBeAbleToResolveStringLiterals) +{ + char buffer[BUFFER_LEN] = {}; + + std::uint64_t baseOffset = MessageHeader::encodedLength(); + Car car; + car.wrapForEncode(buffer, baseOffset, sizeof(buffer)); + car.putVehicleCode("ABCDE"); + + car.sbeRewind(); + EXPECT_EQ(car.getVehicleCodeAsStringView(), "ABCDE"); +} +#endif \ No newline at end of file