diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.cpp index 945f9734ca..9c77ae6b35 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.cpp @@ -8,11 +8,6 @@ #include -#define MCPP_DLL_IMPORT 1 -#define MCPP_DONT_USE_SHORT_NAMES 1 -#include -#undef MCPP_DLL_IMPORT - #include #include @@ -31,8 +26,6 @@ #include -#include - namespace AZ { namespace ShaderBuilder @@ -83,124 +76,125 @@ namespace AZ } } - //! Binder helper to Matsui C-Pre-Processor library - class McppBinder + /////////////////////////////////////////////////////////////////////// + // McppBinder starts + bool McppBinder::StartPreprocessWithCommandLine(int argc, const char* argv[]) { - public: - McppBinder(PreprocessorData& out, bool plugERR) - : m_outputData(out), - m_plugERR(plugERR) + int errorCode = mcpp_lib_main(argc, argv); + // convert from std::ostringstring to AZStd::string + m_outputData.code = m_outStream.str().c_str(); + m_outputData.diagnostics = m_errStream.str().c_str(); + return errorCode == 0; + } + + int McppBinder::Putc_StaticHinge(int c, MCPP_OUTDEST od) + { + char asString[2] = { aznumeric_cast(c), 0 }; + return Fputs_StaticHinge(asString, od); + } + + int McppBinder::Fputs_StaticHinge(const char* s, MCPP_OUTDEST od) + { + if (!OkToLog(od)) { - // single live instance - s_mcppExclusiveProtection.lock(); - s_currentInstance = this; - SetupMcppCallbacks(); + return 0; } - ~McppBinder() + // chose the proper stream + auto& selectedStream = od == MCPP_OUT ? s_currentInstance->m_outStream : s_currentInstance->m_errStream; + auto tellBefore = selectedStream.tellp(); + // append that message to it + selectedStream << s; + return aznumeric_cast(selectedStream.tellp() - tellBefore); + } + + int McppBinder::Fprintf_StaticHinge(MCPP_OUTDEST od, const char* format, ...) + { + if (!OkToLog(od)) { - s_currentInstance = nullptr; - s_mcppExclusiveProtection.unlock(); + return 0; } + // run the formatting on stack memory first, in case it's enough + char localBuffer[DefaultFprintfBufferSize]; - bool StartPreprocessWithCommandLine(int argc, const char* argv[]) + va_list args; + + va_start(args, format); + int count = azvsnprintf(localBuffer, DefaultFprintfBufferSize, format, args); + va_end(args); + + char* result = localBuffer; + + // @result will be bound to @biggerData in case @localBuffer is not big enough. + std::unique_ptr biggerData; + // ">=" is the right comparison because in case count == bufferSize + // We will need an extra byte to accomodate the '\0' ending character. + if (count >= DefaultFprintfBufferSize) { - int errorCode = mcpp_lib_main(argc, argv); - // convert from std::ostringstring to AZStd::string - m_outputData.code = m_outStream.str().c_str(); - m_outputData.diagnostics = m_errStream.str().c_str(); - return errorCode == 0; - } - - private: - - // ====== C-API compatible "Static Hinges" (plain free functions) ====== - // : capturing-lambdas, function-objects, bind-expression; can't be decayed to function pointers, - // because they hold runtime-dynamic type-erased states. So we need intermediates - - // entry point from mcpp. hijacking its output - static int Putc_StaticHinge(int c, MCPP_OUTDEST od) - { - char asString[2] = { aznumeric_cast(c), 0 }; - return Fputs_StaticHinge(asString, od); - } - - // entry point from mcpp. hijacking its output - static int Fputs_StaticHinge(const char* s, MCPP_OUTDEST od) - { - if (!OkToLog(od)) - { - return 0; - } - // chose the proper stream - auto& selectedStream = od == MCPP_OUT ? s_currentInstance->m_outStream : s_currentInstance->m_errStream; - auto tellBefore = selectedStream.tellp(); - // append that message to it - selectedStream << s; - return aznumeric_cast(selectedStream.tellp() - tellBefore); - } - - // entry point from mcpp. hijacking its output - static int Fprintf_StaticHinge(MCPP_OUTDEST od, const char* format, ...) - { - if (!OkToLog(od)) - { - return 0; - } - // run the formatting on stack memory first, in case it's enough - constexpr int bufferSize = 256; - char localBuffer[bufferSize]; - va_list args; + // There wasn't enough space in the local store. + count++; // vsnprintf returns a size that doesn't include the null character. + biggerData.reset(new char[count]); + result = &biggerData[0]; + + // Remark: for MacOS & Linux it is important to call va_start again before + // each call to azvsnprintf. Not required for Windows. va_start(args, format); - int count = azvsnprintf(localBuffer, 256, format, args); - AZStd::unique_ptr biggerData; // will be bound to a bigger array if necessary. - char* result = localBuffer; - if (count > bufferSize) - { // there wasn't enough space in the local store. - biggerData.reset(new char[count]); - result = &biggerData[0]; // change `result`'s pointee - count = azvsnprintf(result, count, format, args); - } - AZ_Error("Preprocessor", count >= 0, "String formatting of pre-precessor output failed"); + count = azvsnprintf(result, count, format, args); va_end(args); - return Fputs_StaticHinge(result, od); } - - static void IncludeReport_StaticHinge(FILE*, const char*, const char*, const char* path) + else if (count == -1) { - s_currentInstance->m_outputData.includedPaths.insert(path); + // In Windows azvsnprintf will always return -1 if @localBuffer is not big enough, + // But it will write in @localBuffer what it could. + // See: + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/vsnprintf-vsnprintf-vsnprintf-l-vsnwprintf-vsnwprintf-l?view=msvc-160 + // In particular: "If the number of characters to write is greater than count, + // these functions return -1 indicating that output has been truncated." + + // There wasn't enough space in the local store. + // Remark: for MacOS & Linux it is important to call va_start again before + // each call to azvsnprintf. Not required for Windows. + va_start(args, format); + count = azvscprintf(format, args) + 1; // vscprintf returns a size that doesn't include the null character. + va_end(args); + + biggerData.reset(new char[count]); + result = &biggerData[0]; + + va_start(args, format); + count = azvsnprintf(result, count, format, args); + va_end(args); } - // ====== utility methods ===== + AZ_Error("Preprocessor", count >= 0, "String formatting of pre-precessor output failed"); + return Fputs_StaticHinge(result, od); + } - static bool OkToLog(MCPP_OUTDEST od) - { - bool isErrButOk = od == MCPP_ERR && s_currentInstance->m_plugERR; - return od == MCPP_OUT || isErrButOk; - } + void McppBinder::IncludeReport_StaticHinge(FILE*, const char*, const char*, const char* path) + { + s_currentInstance->m_outputData.includedPaths.insert(path); + } - static void SetupMcppCallbacks() - { - // callback for header included notification - mcpp_set_report_include_callback(IncludeReport_StaticHinge); - // callback for output redirection - mcpp_set_out_func(Putc_StaticHinge, Fputs_StaticHinge, Fprintf_StaticHinge); - } + bool McppBinder::OkToLog(MCPP_OUTDEST od) + { + bool isErrButOk = od == MCPP_ERR && s_currentInstance->m_plugERR; + return od == MCPP_OUT || isErrButOk; + } - // ====== instance data ====== - PreprocessorData& m_outputData; - std::ostringstream m_outStream, m_errStream; - bool m_plugERR; - - // ====== shared data ====== - // MCPP is a library with tons of non TLS global states, it can only be accessed by one client at a time. - static AZStd::mutex s_mcppExclusiveProtection; - static McppBinder* s_currentInstance; - }; + void McppBinder::SetupMcppCallbacks() + { + // callback for header included notification + mcpp_set_report_include_callback(IncludeReport_StaticHinge); + // callback for output redirection + mcpp_set_out_func(Putc_StaticHinge, Fputs_StaticHinge, Fprintf_StaticHinge); + } // definitions for the linker AZStd::mutex McppBinder::s_mcppExclusiveProtection; McppBinder* McppBinder::s_currentInstance = nullptr; + // McppBinder ends + /////////////////////////////////////////////////////////////////////// + bool PreprocessFile(const AZStd::string& fullPath, PreprocessorData& outputData, const PreprocessorOptions& options , bool collectDiagnostics, bool preprocessIncludedFiles) { diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.h b/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.h index bb4388999f..9f3c770e78 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.h +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/CommonFiles/Preprocessor.h @@ -14,6 +14,18 @@ #include #include +#define MCPP_DLL_IMPORT 1 +#define MCPP_DONT_USE_SHORT_NAMES 1 +#include +#undef MCPP_DLL_IMPORT + +#include + +namespace UnitTest +{ + class McppBinderTests; +} + namespace AZ { namespace ShaderBuilder @@ -93,5 +105,64 @@ namespace AZ AZStd::string& sourceCode, AZStd::string newFileOrigin); + //! Binder helper to Matsui C-Pre-Processor library + class McppBinder + { + public: + McppBinder(PreprocessorData& out, bool plugERR) + : m_outputData(out) + , m_plugERR(plugERR) + { + // single live instance + s_mcppExclusiveProtection.lock(); + s_currentInstance = this; + SetupMcppCallbacks(); + } + ~McppBinder() + { + s_currentInstance = nullptr; + s_mcppExclusiveProtection.unlock(); + } + + // This constant is in the header so McppBinderTests can see it. + static constexpr int DefaultFprintfBufferSize = 256; + + bool StartPreprocessWithCommandLine(int argc, const char* argv[]); + + private: + friend class ::UnitTest::McppBinderTests; + + // ====== C-API compatible "Static Hinges" (plain free functions) ====== + // : capturing-lambdas, function-objects, bind-expression; can't be decayed to function pointers, + // because they hold runtime-dynamic type-erased states. So we need intermediates + + // entry point from mcpp. hijacking its output + static int Putc_StaticHinge(int c, MCPP_OUTDEST od); + + // entry point from mcpp. hijacking its output + static int Fputs_StaticHinge(const char* s, MCPP_OUTDEST od); + + // entry point from mcpp. hijacking its output + static int Fprintf_StaticHinge(MCPP_OUTDEST od, const char* format, ...); + + static void IncludeReport_StaticHinge(FILE*, const char*, const char*, const char* path); + + // ====== utility methods ===== + + static bool OkToLog(MCPP_OUTDEST od); + + static void SetupMcppCallbacks(); + + // ====== instance data ====== + PreprocessorData& m_outputData; + std::ostringstream m_outStream, m_errStream; + bool m_plugERR; + + // ====== shared data ====== + // MCPP is a library with tons of non TLS global states, it can only be accessed by one client at a time. + static AZStd::mutex s_mcppExclusiveProtection; + static McppBinder* s_currentInstance; + }; + } // ShaderBuilder } // AZ diff --git a/Gems/Atom/Asset/Shader/Code/Tests/McppBinderTests.cpp b/Gems/Atom/Asset/Shader/Code/Tests/McppBinderTests.cpp new file mode 100644 index 0000000000..926f9b0a76 --- /dev/null +++ b/Gems/Atom/Asset/Shader/Code/Tests/McppBinderTests.cpp @@ -0,0 +1,92 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include +#include + +#include "Common/ShaderBuilderTestFixture.h" + +#include + +namespace UnitTest +{ + using namespace AZ; + + // The main purpose of this class is to test ShaderBuilder::McppBinder::Fprintf_StaticHinge() + // Which has three common scenarios to validate. + // 1- The formatted string is expected to yield less bytes than McppBinder::DefaultFprintfBufferSize. + // 2- The formatted string is expected to yield exactly McppBinder::DefaultFprintfBufferSize number of bytes. + // 3- The formatted string is expectedc to yield more bytes than McppBinder::DefaultFprintfBufferSize. + class McppBinderTests : public ShaderBuilderTestFixture + { + public: + + // Fills @buffer with 'a' to 'z' for up to @bufferSize number of bytes. + // This function will null('\0') char terminate @buffer. + void FillBufferWithAlphabet(char* buffer, int bufferSize) + { + for (int bufferPos = 0, rollback = 0; bufferPos < (bufferSize - 1); ++bufferPos) + { + const char value = 'a' + rollback++; + buffer[bufferPos] = value; + if (value == 'z') + { + rollback = 0; + } + } + buffer[bufferSize - 1] = '\0'; + } + + // Pushes the null terminated string, @inputString, into McppBinder capture stream + // using McppBinder::Fprintf_StaticHinge(). + // Returns the content of the McppBinder capture stream as a string. + AZStd::string PrintStringThroughStaticHinge(const char* inputString) + { + ShaderBuilder::PreprocessorData preprocessorData; + ShaderBuilder::McppBinder mcppBinder(preprocessorData, false); + ShaderBuilder::McppBinder::Fprintf_StaticHinge(MCPP_OUTDEST::MCPP_OUT, "%s", inputString); + // convert from std::ostringstring to AZStd::string + return AZStd::string(mcppBinder.m_outStream.str().c_str()); + } + }; // class McppBinderTests + + + TEST_F(McppBinderTests, ShouldPrintLessBytesThanDefaultSize) + { + constexpr int bufferSize = (ShaderBuilder::McppBinder::DefaultFprintfBufferSize / 2) + 1; + EXPECT_TRUE(bufferSize > 0); + char buffer[bufferSize] = ""; + FillBufferWithAlphabet(buffer, bufferSize); + auto printedString = PrintStringThroughStaticHinge(buffer); + EXPECT_EQ(AZStd::string(buffer), printedString); + } + + TEST_F(McppBinderTests, ShouldPrintSameBytesAsDefaultSize) + { + constexpr int bufferSize = ShaderBuilder::McppBinder::DefaultFprintfBufferSize + 1; + EXPECT_TRUE(bufferSize > 0); + char buffer[bufferSize] = ""; + FillBufferWithAlphabet(buffer, bufferSize); + auto printedString = PrintStringThroughStaticHinge(buffer); + EXPECT_EQ(AZStd::string(buffer), printedString); + } + + TEST_F(McppBinderTests, ShouldPrintMoreBytesThanDefaultSize) + { + constexpr int bufferSize = (ShaderBuilder::McppBinder::DefaultFprintfBufferSize * 2) + 1; + EXPECT_TRUE(bufferSize > 0); + char buffer[bufferSize] = ""; + FillBufferWithAlphabet(buffer, bufferSize); + auto printedString = PrintStringThroughStaticHinge(buffer); + EXPECT_EQ(AZStd::string(buffer), printedString); + } + +} //namespace UnitTest + +//AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV); + diff --git a/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake b/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake index bfddb5ce8e..033b399478 100644 --- a/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake +++ b/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake @@ -10,4 +10,5 @@ set(FILES Tests/Common/ShaderBuilderTestFixture.h Tests/Common/ShaderBuilderTestFixture.cpp Tests/SupervariantCmdArgumentTests.cpp + Tests/McppBinderTests.cpp )