From f7da64a5183f61bee2a057ee4eb9b82c6c4e8630 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Mon, 12 Apr 2021 19:52:01 -0700 Subject: [PATCH 01/14] =?UTF-8?q?=EF=BB=BFAllowing=20to=20build=20with=20A?= =?UTF-8?q?San=20enabled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- cmake/Configurations.cmake | 127 +++++++++--------- .../Common/MSVC/Configurations_msvc.cmake | 23 +++- cmake/cmake_files.cmake | 1 + 3 files changed, 83 insertions(+), 68 deletions(-) diff --git a/cmake/Configurations.cmake b/cmake/Configurations.cmake index a580f1c572..693cc23c7d 100644 --- a/cmake/Configurations.cmake +++ b/cmake/Configurations.cmake @@ -20,24 +20,33 @@ include_guard(GLOBAL) # \arg:LINK_STATIC_${CONFIGURATION} # \arg:LINK_NON_STATIC # \arg:LINK_NON_STATIC_${CONFIGURATION} -# \arg:LINK_EXECUTABLE -# \arg:LINK_EXECUTABLE_${CONFIGURATION} +# \arg:LINK_EXE +# \arg:LINK_EXE_${CONFIGURATION} +# \arg:LINK_MODULE +# \arg:LINK_MODULE_${CONFIGURATION} +# \arg:LINK_SHARED +# \arg:LINK_SHARED_${CONFIGURATION} # function(ly_append_configurations_options) set(options) set(oneValueArgs) - set(multiValueArgs + set(multiArgs DEFINES COMPILATION LINK LINK_STATIC LINK_NON_STATIC - LINK_EXECUTABLE + LINK_EXE + LINK_MODULE + LINK_SHARED ) - foreach(conf IN LISTS CMAKE_CONFIGURATION_TYPES) - string(TOUPPER ${conf} UCONF) - set(multiValueArgs ${multiValueArgs} DEFINES_${UCONF} COMPILATION_${UCONF} LINK_${UCONF} LINK_STATIC_${UCONF} LINK_NON_STATIC_${UCONF} LINK_EXECUTABLE_${UCONF}) + foreach(arg IN LISTS multiArgs) + list(APPEND multiValueArgs ${arg}) + foreach(conf IN LISTS CMAKE_CONFIGURATION_TYPES) + string(TOUPPER ${conf} UCONF) + list(APPEND multiValueArgs ${arg}_${UCONF}) + endforeach() endforeach() cmake_parse_arguments(ly_append_configurations_options "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN}) @@ -45,48 +54,46 @@ function(ly_append_configurations_options) if(ly_append_configurations_options_DEFINES) add_compile_definitions(${ly_append_configurations_options_DEFINES}) endif() + if(ly_append_configurations_options_COMPILATION) string(REPLACE ";" " " COMPILATION_STR "${ly_append_configurations_options_COMPILATION}") - string(APPEND CMAKE_C_FLAGS " " ${COMPILATION_STR}) - string(APPEND CMAKE_CXX_FLAGS " " ${COMPILATION_STR}) - set(CMAKE_C_FLAGS ${CMAKE_C_FLAGS} PARENT_SCOPE) - set(CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS} PARENT_SCOPE) + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${COMPILATION_STR}" PARENT_SCOPE) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${COMPILATION_STR}" PARENT_SCOPE) endif() + if(ly_append_configurations_options_LINK) string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK}") - string(APPEND LINK_OPTIONS " " ${LINK_STR}) - set(LINK_OPTIONS ${LINK_OPTIONS} PARENT_SCOPE) - - # Not defining these issue warnings, TODO: investigate - set(CMAKE_STATIC_LINKER_FLAGS ${LINK_OPTIONS} PARENT_SCOPE) - set(CMAKE_MODULE_LINKER_FLAGS ${LINK_OPTIONS} PARENT_SCOPE) - set(CMAKE_SHARED_LINKER_FLAGS ${LINK_OPTIONS} PARENT_SCOPE) - set(CMAKE_EXE_LINKER_FLAGS ${LINK_OPTIONS} PARENT_SCOPE) + set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${LINK_OPTIONS}" PARENT_SCOPE) endif() - if(ly_append_configurations_options_LINK_STATIC) - string(REPLACE ";" " " LINK_STATIC_STR "${ly_append_configurations_options_LINK_STATIC}") - string(APPEND LINK_STATIC_OPTIONS " " ${LINK_STATIC_STR}) - set(LINK_STATIC_OPTIONS ${LINK_STATIC_OPTIONS} PARENT_SCOPE) - set(CMAKE_STATIC_LINKER_FLAGS ${LINK_STATIC_OPTIONS} PARENT_SCOPE) + if(ly_append_configurations_options_LINK_STATIC) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_STATIC}") + set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) endif() if(ly_append_configurations_options_LINK_NON_STATIC) - string(REPLACE ";" " " LINK_NON_STATIC_STR "${ly_append_configurations_options_LINK_NON_STATIC}") - string(APPEND LINK_NON_STATIC_OPTIONS " " ${LINK_NON_STATIC_STR}) - set(LINK_NON_STATIC_OPTIONS ${LINK_NON_STATIC_OPTIONS} PARENT_SCOPE) - - set(CMAKE_MODULE_LINKER_FLAGS ${LINK_NON_STATIC_OPTIONS} PARENT_SCOPE) - set(CMAKE_SHARED_LINKER_FLAGS ${LINK_NON_STATIC_OPTIONS} PARENT_SCOPE) - set(CMAKE_EXE_LINKER_FLAGS ${LINK_NON_STATIC_OPTIONS} PARENT_SCOPE) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_NON_STATIC}") + set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) endif() - if(ly_append_configurations_options_LINK_EXECUTABLE) - string(REPLACE ";" " " LINK_EXECUTABLE_STR "${ly_append_configurations_options_LINK_EXECUTABLE}") - string(APPEND LINK_EXECUTABLE_OPTIONS " " ${LINK_EXECUTABLE_STR}) - set(LINK_EXECUTABLE_OPTIONS ${LINK_EXECUTABLE_OPTIONS} PARENT_SCOPE) + if(ly_append_configurations_options_LINK_EXE) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_EXE}") + set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + endif() - set(CMAKE_EXE_LINKER_FLAGS ${LINK_EXECUTABLE_OPTIONS} PARENT_SCOPE) + if(ly_append_configurations_options_LINK_MODULE) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_MODULE}") + set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) + endif() + + if(ly_append_configurations_options_LINK_SHARED) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_SHARED}") + set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${LINK_STR}" PARENT_SCOPE) endif() foreach(conf IN LISTS CMAKE_CONFIGURATION_TYPES) @@ -100,43 +107,33 @@ function(ly_append_configurations_options) endif() if(ly_append_configurations_options_COMPILATION_${UCONF}) string(REPLACE ";" " " COMPILATION_STR "${ly_append_configurations_options_COMPILATION_${UCONF}}") - string(APPEND CMAKE_C_FLAGS_${UCONF} " " ${COMPILATION_STR}) - string(APPEND CMAKE_CXX_FLAGS_${UCONF} " " ${COMPILATION_STR}) - set(CMAKE_C_FLAGS_${UCONF} ${CMAKE_C_FLAGS_${UCONF}} PARENT_SCOPE) - set(CMAKE_CXX_FLAGS_${UCONF} ${CMAKE_CXX_FLAGS_${UCONF}} PARENT_SCOPE) + set(CMAKE_C_FLAGS_${UCONF} "${CMAKE_C_FLAGS_${UCONF}} ${COMPILATION_STR}" PARENT_SCOPE) + set(CMAKE_CXX_FLAGS_${UCONF} "${CMAKE_CXX_FLAGS_${UCONF}} ${COMPILATION_STR}" PARENT_SCOPE) endif() if(ly_append_configurations_options_LINK_${UCONF}) string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_${UCONF}}") - string(APPEND LINK_OPTIONS_${UCONF} " " ${LINK_STR}) - set(LINK_OPTIONS_${UCONF} ${LINK_OPTIONS_${UCONF}} PARENT_SCOPE) - - set(CMAKE_STATIC_LINKER_FLAGS_${UCONF} ${LINK_OPTIONS_${UCONF}} PARENT_SCOPE) - set(CMAKE_MODULE_LINKER_FLAGS_${UCONF} ${LINK_OPTIONS_${UCONF}} PARENT_SCOPE) - set(CMAKE_SHARED_LINKER_FLAGS_${UCONF} ${LINK_OPTIONS_${UCONF}} PARENT_SCOPE) - set(CMAKE_EXE_LINKER_FLAGS_${UCONF} ${LINK_OPTIONS_${UCONF}} PARENT_SCOPE) + set(CMAKE_STATIC_LINKER_FLAGS_${UCONF} "${CMAKE_STATIC_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_MODULE_LINKER_FLAGS_${UCONF} "${CMAKE_MODULE_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_SHARED_LINKER_FLAGS_${UCONF} "${CMAKE_SHARED_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_EXE_LINKER_FLAGS_${UCONF} "${CMAKE_EXE_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) endif() if(ly_append_configurations_options_LINK_STATIC_${UCONF}) - string(REPLACE ";" " " LINK_STATIC_STR "${ly_append_configurations_options_LINK_STATIC_${UCONF}}") - string(APPEND LINK_STATIC_OPTIONS_${UCONF} " " ${LINK_STATIC_STR}) - set(LINK_STATIC_OPTIONS_${UCONF} ${LINK_STATIC_OPTIONS_${UCONF}} PARENT_SCOPE) - - set(CMAKE_STATIC_LINKER_FLAGS_${UCONF} ${LINK_STATIC_OPTIONS_${UCONF}} PARENT_SCOPE) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_STATIC_${UCONF}}") + set(CMAKE_STATIC_LINKER_FLAGS_${UCONF} "${CMAKE_STATIC_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) endif() if(ly_append_configurations_options_LINK_NON_STATIC_${UCONF}) - string(REPLACE ";" " " LINK_NON_STATIC_STR "${ly_append_configurations_options_LINK_NON_STATIC_${UCONF}}") - string(APPEND LINK_NON_STATIC_OPTIONS_${UCONF} " " ${LINK_NON_STATIC_STR}) - set(LINK_NON_STATIC_OPTIONS_${UCONF} ${LINK_NON_STATIC_OPTIONS_${UCONF}} PARENT_SCOPE) - - set(CMAKE_MODULE_LINKER_FLAGS_${UCONF} ${LINK_NON_STATIC_OPTIONS_${UCONF}} PARENT_SCOPE) - set(CMAKE_SHARED_LINKER_FLAGS_${UCONF} ${LINK_NON_STATIC_OPTIONS_${UCONF}} PARENT_SCOPE) - set(CMAKE_EXE_LINKER_FLAGS_${UCONF} ${LINK_NON_STATIC_OPTIONS_${UCONF}} PARENT_SCOPE) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_NON_STATIC_${UCONF}}") + set(CMAKE_MODULE_LINKER_FLAGS_${UCONF} "${CMAKE_MODULE_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_SHARED_LINKER_FLAGS_${UCONF} "${CMAKE_SHARED_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) + set(CMAKE_EXE_LINKER_FLAGS_${UCONF} "${CMAKE_EXE_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) endif() - if(ly_append_configurations_options_LINK_EXECUTABLE_${UCONF}) - string(REPLACE ";" " " LINK_EXECUTABLE_STR "${ly_append_configurations_options_LINK_EXECUTABLE_${UCONF}}") - string(APPEND LINK_EXECUTABLE_OPTIONS_${UCONF} " " ${LINK_EXECUTABLE_STR}) - set(LINK_EXECUTABLE_OPTIONS_${UCONF} ${LINK_EXECUTABLE_OPTIONS_${UCONF}} PARENT_SCOPE) - - set(CMAKE_EXE_LINKER_FLAGS_${UCONF} ${LINK_EXECUTABLE_OPTIONS_${UCONF}} PARENT_SCOPE) + if(ly_append_configurations_options_LINK_EXE_${UCONF}) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_EXE_${UCONF}}") + set(CMAKE_EXE_LINKER_FLAGS_${UCONF} "${CMAKE_EXE_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) + endif() + if(ly_append_configurations_options_LINK_MODULE_${UCONF}) + string(REPLACE ";" " " LINK_STR "${ly_append_configurations_options_LINK_MODULE_${UCONF}}") + set(CMAKE_MODULE_LINKER_FLAGS_${UCONF} "${CMAKE_MODULE_LINKER_FLAGS_${UCONF}} ${LINK_STR}" PARENT_SCOPE) endif() endforeach() diff --git a/cmake/Platform/Common/MSVC/Configurations_msvc.cmake b/cmake/Platform/Common/MSVC/Configurations_msvc.cmake index 118a515e30..c0d0809b0f 100644 --- a/cmake/Platform/Common/MSVC/Configurations_msvc.cmake +++ b/cmake/Platform/Common/MSVC/Configurations_msvc.cmake @@ -64,9 +64,6 @@ ly_append_configurations_options( # It also causes the compiler to place the library name MSVCRTD.lib into the .obj file. /Ob0 # Disables inline expansions /Od # Disables optimization - /RTCsu # Run-Time Error Checks: c Reports when a value is assigned to a smaller data type and results in a data loss (Not supoported by the STL) - # s Enables stack frame run-time error checking - # u Reports when a variable is used without having been initialized COMPILATION_PROFILE /GF # Enable string pooling /Gy # Function level linking @@ -96,6 +93,26 @@ ly_append_configurations_options( /INCREMENTAL:NO ) +set(LY_BUILD_WITH_ADDRESS_SANITIZER FALSE CACHE BOOL "Builds using AddressSanitizer (ASan). Will disable Edit/Continue, Incremental building and Run-Time checks (default = FALSE)") +if(LY_BUILD_WITH_ADDRESS_SANITIZER) + set(LY_BUILD_WITH_INCREMENTAL_LINKING_DEBUG FALSE) + ly_append_configurations_options( + COMPILATION_DEBUG + /fsanitize=address + ) + get_filename_component(link_tools_dir ${CMAKE_LINKER} DIRECTORY) + file(COPY + ${link_tools_dir}/clang_rt.asan_dbg_dynamic-x86_64.dll + DESTINATION ${CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG}) +else() + ly_append_configurations_options( + COMPILATION_DEBUG + /RTCsu # Run-Time Error Checks: c Reports when a value is assigned to a smaller data type and results in a data loss (Not supoported by the STL) + # s Enables stack frame run-time error checking + # u Reports when a variable is used without having been initialized + ) +endif() + set(LY_BUILD_WITH_INCREMENTAL_LINKING_DEBUG FALSE CACHE BOOL "Indicates if incremental linking is used in debug configurations (default = FALSE)") if(LY_BUILD_WITH_INCREMENTAL_LINKING_DEBUG) ly_append_configurations_options( diff --git a/cmake/cmake_files.cmake b/cmake/cmake_files.cmake index aa275b634a..10676b384e 100644 --- a/cmake/cmake_files.cmake +++ b/cmake/cmake_files.cmake @@ -20,6 +20,7 @@ set(FILES Findo3de.cmake Gems.cmake GeneralSettings.cmake + Initialize.cmake Install.cmake LyAutoGen.cmake LYPackage_S3Downloader.cmake From b4f6dc5bff2640fbef603f11099250310f4bd757 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Thu, 12 Aug 2021 16:44:01 -0700 Subject: [PATCH 02/14] fixes after merge Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- cmake/Platform/Common/MSVC/Configurations_msvc.cmake | 2 +- cmake/cmake_files.cmake | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cmake/Platform/Common/MSVC/Configurations_msvc.cmake b/cmake/Platform/Common/MSVC/Configurations_msvc.cmake index c0d0809b0f..891d48ddb2 100644 --- a/cmake/Platform/Common/MSVC/Configurations_msvc.cmake +++ b/cmake/Platform/Common/MSVC/Configurations_msvc.cmake @@ -95,7 +95,7 @@ ly_append_configurations_options( set(LY_BUILD_WITH_ADDRESS_SANITIZER FALSE CACHE BOOL "Builds using AddressSanitizer (ASan). Will disable Edit/Continue, Incremental building and Run-Time checks (default = FALSE)") if(LY_BUILD_WITH_ADDRESS_SANITIZER) - set(LY_BUILD_WITH_INCREMENTAL_LINKING_DEBUG FALSE) + set(LY_BUILD_WITH_INCREMENTAL_LINKING_DEBUG FALSE) ly_append_configurations_options( COMPILATION_DEBUG /fsanitize=address diff --git a/cmake/cmake_files.cmake b/cmake/cmake_files.cmake index 10676b384e..aa275b634a 100644 --- a/cmake/cmake_files.cmake +++ b/cmake/cmake_files.cmake @@ -20,7 +20,6 @@ set(FILES Findo3de.cmake Gems.cmake GeneralSettings.cmake - Initialize.cmake Install.cmake LyAutoGen.cmake LYPackage_S3Downloader.cmake From a087fc06a9bc833aba1eaf42aa8a1eeb5295cc77 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Thu, 12 Aug 2021 16:44:25 -0700 Subject: [PATCH 03/14] fixes for ASAn Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/std/string/string_view.h | 5 ++++- Code/Framework/AzCore/Tests/AZStd/String.cpp | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/std/string/string_view.h b/Code/Framework/AzCore/AzCore/std/string/string_view.h index 9a98795554..4ded44644f 100644 --- a/Code/Framework/AzCore/AzCore/std/string/string_view.h +++ b/Code/Framework/AzCore/AzCore/std/string/string_view.h @@ -7,7 +7,6 @@ */ #pragma once -#include #include #include @@ -46,6 +45,10 @@ namespace AZStd return npos; } size_t foundIndex = searchIndex + charFindIndex; + if (foundIndex + count > size) + { + return npos; // the rest of the string doesnt fit in the remainder of the data buffer + } if (Traits::compare(&data[foundIndex], ptr, count) == 0) { return foundIndex; diff --git a/Code/Framework/AzCore/Tests/AZStd/String.cpp b/Code/Framework/AzCore/Tests/AZStd/String.cpp index 9dae4a3d3f..a88de48b9b 100644 --- a/Code/Framework/AzCore/Tests/AZStd/String.cpp +++ b/Code/Framework/AzCore/Tests/AZStd/String.cpp @@ -1458,17 +1458,17 @@ namespace UnitTest constexpr double v15 = 0; constexpr const char* v16 = "Hello"; constexpr const wchar_t* v17 = L"Hello"; - constexpr void* v18 = 0; + constexpr void* v18 = nullptr; // This shouldn't give a compile error AZStd::string::format( - "%i %c %uc %c %c %i %i %u %i %lu %li %llu %lli %f %f %s %ls %p", + "%i %c %uc %hc %lc %i %i %u %i %lu %li %llu %lli %f %f %hs %ls %p", v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18); // This shouldn't give a compile error AZStd::wstring::format( - L"%i %c %uc %c %lc %i %i %u %i %lu %li %llu %lli %f %f %s %ls %p", - v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18); + L"%i %c %uc %hc %lc %i %i %u %i %lu %li %llu %lli %f %f %hs %ls %p", + v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18); class WrappedInt { From 89e1b6db255fa4f09ccd2a306d76cd58a70985f4 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Thu, 12 Aug 2021 16:44:59 -0700 Subject: [PATCH 04/14] Making allocator use the MallocSchema so we can take full advantage of ASan Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/IO/Path/Path.inl | 1 + .../AzCore/AzCore/Memory/SystemAllocator.cpp | 66 ++++---- Code/Framework/AzCore/CMakeLists.txt | 9 + .../AzFramework/Physics/Common/PhysicsTypes.h | 1 + Code/Legacy/CryCommon/LegacyAllocator.cpp | 79 +++++++++ Code/Legacy/CryCommon/LegacyAllocator.h | 157 ++---------------- Code/Legacy/CryCommon/crycommon_files.cmake | 1 + 7 files changed, 138 insertions(+), 176 deletions(-) create mode 100644 Code/Legacy/CryCommon/LegacyAllocator.cpp diff --git a/Code/Framework/AzCore/AzCore/IO/Path/Path.inl b/Code/Framework/AzCore/AzCore/IO/Path/Path.inl index 4e8356b436..ad1d09bc01 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/Path.inl +++ b/Code/Framework/AzCore/AzCore/IO/Path/Path.inl @@ -10,6 +10,7 @@ #include #include +#include // extern instantiations of Path templates to prevent implicit instantiations namespace AZ::IO diff --git a/Code/Framework/AzCore/AzCore/Memory/SystemAllocator.cpp b/Code/Framework/AzCore/AzCore/Memory/SystemAllocator.cpp index ada6c8f330..07c919cf00 100644 --- a/Code/Framework/AzCore/AzCore/Memory/SystemAllocator.cpp +++ b/Code/Framework/AzCore/AzCore/Memory/SystemAllocator.cpp @@ -17,15 +17,23 @@ #include -#define AZCORE_SYS_ALLOCATOR_HPPA // If you disable this make sure you start building the heapschema.cpp -//#define AZCORE_SYS_ALLOCATOR_MALLOC +#define AZCORE_SYSTEM_ALLOCATOR_HPHA 1 +#define AZCORE_SYSTEM_ALLOCATOR_MALLOC 2 +#define AZCORE_SYSTEM_ALLOCATOR_HEAP 3 -#ifdef AZCORE_SYS_ALLOCATOR_HPPA -# include -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) -#include +#if !defined(AZCORE_SYSTEM_ALLOCATOR) + // define the default + #define AZCORE_SYSTEM_ALLOCATOR AZCORE_SYSTEM_ALLOCATOR_HPHA +#endif + +#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HPHA + #include +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC + #include +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HEAP + #include #else -# include + #error "Invalid allocator selected for SystemAllocator" #endif @@ -34,12 +42,12 @@ using namespace AZ; ////////////////////////////////////////////////////////////////////////// // Globals - we use global storage for the first memory schema, since we can't use dynamic memory! static bool g_isSystemSchemaUsed = false; -#ifdef AZCORE_SYS_ALLOCATOR_HPPA -static AZStd::aligned_storage::value>::type g_systemSchema; -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) -static AZStd::aligned_storage::value>::type g_systemSchema; -#else -static AZStd::aligned_storage::value>::type g_systemSchema; +#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HPHA + static AZStd::aligned_storage::value>::type g_systemSchema; +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC + static AZStd::aligned_storage::value>::type g_systemSchema; +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HEAP + static AZStd::aligned_storage::value>::type g_systemSchema; #endif ////////////////////////////////////////////////////////////////////////// @@ -97,9 +105,9 @@ SystemAllocator::Create(const Descriptor& desc) else { m_isCustom = false; -#ifdef AZCORE_SYS_ALLOCATOR_HPPA - HphaSchema::Descriptor heapDesc; - heapDesc.m_pageSize = desc.m_heap.m_pageSize; +#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HPHA + HphaSchema::Descriptor heapDesc; + heapDesc.m_pageSize = desc.m_heap.m_pageSize; heapDesc.m_poolPageSize = desc.m_heap.m_poolPageSize; AZ_Assert(desc.m_heap.m_numFixedMemoryBlocks <= 1, "We support max1 memory block at the moment!"); if (desc.m_heap.m_numFixedMemoryBlocks > 0) @@ -111,11 +119,10 @@ SystemAllocator::Create(const Descriptor& desc) heapDesc.m_isPoolAllocations = desc.m_heap.m_isPoolAllocations; // Fix SystemAllocator from growing in small chunks heapDesc.m_systemChunkSize = desc.m_heap.m_systemChunkSize; - -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC MallocSchema::Descriptor heapDesc; -#else - HeapSchema::Descriptor heapDesc; +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HEAP + HeapSchema::Descriptor heapDesc; memcpy(heapDesc.m_memoryBlocks, desc.m_heap.m_memoryBlocks, sizeof(heapDesc.m_memoryBlocks)); memcpy(heapDesc.m_memoryBlocksByteSize, desc.m_heap.m_memoryBlocksByteSize, sizeof(heapDesc.m_memoryBlocksByteSize)); heapDesc.m_numMemoryBlocks = desc.m_heap.m_numMemoryBlocks; @@ -124,11 +131,11 @@ SystemAllocator::Create(const Descriptor& desc) { AZ_Assert(!g_isSystemSchemaUsed, "AZ::SystemAllocator MUST be created first! It's the source of all allocations!"); -#ifdef AZCORE_SYS_ALLOCATOR_HPPA +#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HPHA m_allocator = new(&g_systemSchema)HphaSchema(heapDesc); -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC m_allocator = new(&g_systemSchema)MallocSchema(heapDesc); -#else +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HEAP m_allocator = new(&g_systemSchema)HeapSchema(heapDesc); #endif g_isSystemSchemaUsed = true; @@ -139,14 +146,13 @@ SystemAllocator::Create(const Descriptor& desc) // this class should be inheriting from SystemAllocator AZ_Assert(AllocatorInstance::IsReady(), "System allocator must be created before any other allocator! They allocate from it."); -#ifdef AZCORE_SYS_ALLOCATOR_HPPA +#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HPHA m_allocator = azcreate(HphaSchema, (heapDesc), SystemAllocator); -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC m_allocator = azcreate(MallocSchema, (heapDesc), SystemAllocator); -#else +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HEAP m_allocator = azcreate(HeapSchema, (heapDesc), SystemAllocator); #endif - if (m_allocator == NULL) { isReady = false; @@ -178,11 +184,11 @@ SystemAllocator::Destroy() { if ((void*)m_allocator == (void*)&g_systemSchema) { -#ifdef AZCORE_SYS_ALLOCATOR_HPPA +#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HPHA static_cast(m_allocator)->~HphaSchema(); -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC static_cast(m_allocator)->~MallocSchema(); -#else +#elif AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_HEAP static_cast(m_allocator)->~HeapSchema(); #endif g_isSystemSchemaUsed = false; diff --git a/Code/Framework/AzCore/CMakeLists.txt b/Code/Framework/AzCore/CMakeLists.txt index ea7cc27af5..e3898ec17e 100644 --- a/Code/Framework/AzCore/CMakeLists.txt +++ b/Code/Framework/AzCore/CMakeLists.txt @@ -54,6 +54,15 @@ ly_add_source_properties( VALUES ${LY_PAL_TOOLS_DEFINES} ) +if(LY_BUILD_WITH_ADDRESS_SANITIZER) + # Default to use Malloc schema so ASan works well + ly_add_source_properties( + SOURCES AzCore/Memory/SystemAllocator.cpp + PROPERTY COMPILE_DEFINITIONS + VALUES AZCORE_SYSTEM_ALLOCATOR=AZCORE_SYSTEM_ALLOCATOR_MALLOC + ) +endif() + ################################################################################ # Tests ################################################################################ diff --git a/Code/Framework/AzFramework/AzFramework/Physics/Common/PhysicsTypes.h b/Code/Framework/AzFramework/AzFramework/Physics/Common/PhysicsTypes.h index bb53d6d3dd..e224c0ab22 100644 --- a/Code/Framework/AzFramework/AzFramework/Physics/Common/PhysicsTypes.h +++ b/Code/Framework/AzFramework/AzFramework/Physics/Common/PhysicsTypes.h @@ -13,6 +13,7 @@ #include #include #include +#include namespace Physics { diff --git a/Code/Legacy/CryCommon/LegacyAllocator.cpp b/Code/Legacy/CryCommon/LegacyAllocator.cpp new file mode 100644 index 0000000000..9c4258530a --- /dev/null +++ b/Code/Legacy/CryCommon/LegacyAllocator.cpp @@ -0,0 +1,79 @@ +/* + * 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 + * + */ + +#pragma once + +#include + +namespace AZ +{ + LegacyAllocator::pointer_type LegacyAllocator::Allocate(size_type byteSize, size_type alignment, int flags, const char* name, const char* fileName, int lineNum, unsigned int suppressStackRecord) + { + if (alignment == 0) + { + // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement + // Take a look at _Allocate_manually_vector_aligned in xmemory0 + alignment = sizeof(void*) * 2; + } + + pointer_type ptr = m_schema->Allocate(byteSize, alignment, flags, name, fileName, lineNum, suppressStackRecord); + AZ_PROFILE_MEMORY_ALLOC_EX(AZ::Debug::ProfileCategory::MemoryReserved, fileName, lineNum, ptr, byteSize, name ? name : GetName()); + AZ_MEMORY_PROFILE(ProfileAllocation(ptr, byteSize, alignment, name, fileName, lineNum, suppressStackRecord)); + AZ_Assert(ptr || byteSize == 0, "OOM - Failed to allocate %zu bytes from LegacyAllocator", byteSize); + return ptr; + } + + // DeAllocate with file/line, to track when allocs were freed from Cry + void LegacyAllocator::DeAllocate(pointer_type ptr, [[maybe_unused]] const char* file, [[maybe_unused]] const int line, size_type byteSize, size_type alignment) + { + AZ_PROFILE_MEMORY_FREE_EX(AZ::Debug::ProfileCategory::MemoryReserved, file, line, ptr); + AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, byteSize, alignment, nullptr)); + m_schema->DeAllocate(ptr, byteSize, alignment); + } + + // Realloc with file/line, because Cry uses realloc(nullptr) and realloc(ptr, 0) to mimic malloc/free + LegacyAllocator::pointer_type LegacyAllocator::ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment, [[maybe_unused]] const char* file, [[maybe_unused]] const int line) + { + if (newAlignment == 0) + { + // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement + // Take a look at _Allocate_manually_vector_aligned in xmemory0 + newAlignment = sizeof(void*) * 2; + } + + AZ_MEMORY_PROFILE(ProfileReallocationBegin(ptr, newSize)); + AZ_PROFILE_MEMORY_FREE_EX(AZ::Debug::ProfileCategory::MemoryReserved, file, line, ptr); + pointer_type newPtr = m_schema->ReAllocate(ptr, newSize, newAlignment); + AZ_PROFILE_MEMORY_ALLOC_EX(AZ::Debug::ProfileCategory::MemoryReserved, file, line, newPtr, newSize, "LegacyAllocator Realloc"); + AZ_MEMORY_PROFILE(ProfileReallocationEnd(ptr, newPtr, newSize, newAlignment)); + AZ_Assert(newPtr || newSize == 0, "OOM - Failed to reallocate %zu bytes from LegacyAllocator", newSize); + return newPtr; + } + + void LegacyAllocator::DeAllocate(pointer_type ptr, size_type byteSize, size_type alignment) + { + AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, 0, 0, nullptr)); + Base::DeAllocate(ptr, byteSize, alignment); + } + + LegacyAllocator::pointer_type LegacyAllocator::ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment) + { + if (newAlignment == 0) + { + // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement + // Take a look at _Allocate_manually_vector_aligned in xmemory0 + newAlignment = sizeof(void*) * 2; + } + + AZ_MEMORY_PROFILE(ProfileReallocationBegin(ptr, newSize)); + pointer_type newPtr = Base::ReAllocate(ptr, newSize, newAlignment); + AZ_MEMORY_PROFILE(ProfileReallocationEnd(ptr, newPtr, newSize, newAlignment)); + AZ_Assert(newPtr || newSize == 0, "OOM - Failed to reallocate %zu bytes from LegacyAllocator", newSize); + return newPtr; + } +} diff --git a/Code/Legacy/CryCommon/LegacyAllocator.h b/Code/Legacy/CryCommon/LegacyAllocator.h index 074c183de4..2b1055958b 100644 --- a/Code/Legacy/CryCommon/LegacyAllocator.h +++ b/Code/Legacy/CryCommon/LegacyAllocator.h @@ -11,118 +11,36 @@ #include #include -#define AZCORE_SYS_ALLOCATOR_HPPA -//#define AZCORE_SYS_ALLOCATOR_MALLOC - -#ifdef AZCORE_SYS_ALLOCATOR_HPPA -# include -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) -# include -#else -# include -#endif - namespace AZ { - -#ifdef AZCORE_SYS_ALLOCATOR_HPPA - typedef AZ::HphaSchema LegacyAllocatorSchema; -#elif defined(AZCORE_SYS_ALLOCATOR_MALLOC) - typedef AZ::MallocSchema LegacyAllocatorSchema; -#else - typedef AZ::HeapSchema LegacyAllocatorSchema; -#endif - - struct LegacyAllocatorDescriptor - : public LegacyAllocatorSchema::Descriptor - { - LegacyAllocatorDescriptor() - { - // pull 32MB from the OS at a time -#ifdef AZCORE_SYS_ALLOCATOR_HPPA - m_systemChunkSize = 32 * 1024 * 1024; -#endif - } - }; - class LegacyAllocator - : public SimpleSchemaAllocator + : public SimpleSchemaAllocator { public: AZ_TYPE_INFO(LegacyAllocator, "{17FC25A4-92D9-48C5-BB85-7F860FCA2C6F}"); - using Descriptor = LegacyAllocatorDescriptor; - using Base = SimpleSchemaAllocator; + using Descriptor = AZ::HphaSchema::Descriptor; + using Base = SimpleSchemaAllocator; + using pointer_type = typename Base::pointer_type; + using size_type = typename Base::size_type; + using difference_type = typename Base::difference_type; LegacyAllocator() : Base("LegacyAllocator", "Allocator for Legacy CryEngine systems") { } - pointer_type Allocate(size_type byteSize, size_type alignment, int flags = 0, const char* name = 0, const char* fileName = 0, int lineNum = 0, unsigned int suppressStackRecord = 0) override - { - if (alignment == 0) - { - // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement - // Take a look at _Allocate_manually_vector_aligned in xmemory0 - alignment = sizeof(void*) * 2; - } - - pointer_type ptr = m_schema->Allocate(byteSize, alignment, flags, name, fileName, lineNum, suppressStackRecord); - AZ_PROFILE_MEMORY_ALLOC_EX(AZ::Debug::ProfileCategory::MemoryReserved, fileName, lineNum, ptr, byteSize, name ? name : GetName()); - AZ_MEMORY_PROFILE(ProfileAllocation(ptr, byteSize, alignment, name, fileName, lineNum, suppressStackRecord)); - AZ_Assert(ptr || byteSize == 0, "OOM - Failed to allocate %zu bytes from LegacyAllocator", byteSize); - return ptr; - } + pointer_type Allocate(size_type byteSize, size_type alignment, int flags = 0, const char* name = 0, const char* fileName = 0, int lineNum = 0, unsigned int suppressStackRecord = 0) override; // DeAllocate with file/line, to track when allocs were freed from Cry - void DeAllocate(pointer_type ptr, [[maybe_unused]] const char* file, [[maybe_unused]] const int line, size_type byteSize = 0, size_type alignment = 0) - { - AZ_PROFILE_MEMORY_FREE_EX(AZ::Debug::ProfileCategory::MemoryReserved, file, line, ptr); - AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, byteSize, alignment, nullptr)); - m_schema->DeAllocate(ptr, byteSize, alignment); - } + void DeAllocate(pointer_type ptr, const char* file, const int line, size_type byteSize = 0, size_type alignment = 0); // Realloc with file/line, because Cry uses realloc(nullptr) and realloc(ptr, 0) to mimic malloc/free - pointer_type ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment, [[maybe_unused]] const char* file, [[maybe_unused]] const int line) - { - if (newAlignment == 0) - { - // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement - // Take a look at _Allocate_manually_vector_aligned in xmemory0 - newAlignment = sizeof(void*) * 2; - } + pointer_type ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment, const char* file, const int line); - AZ_MEMORY_PROFILE(ProfileReallocationBegin(ptr, newSize)); - AZ_PROFILE_MEMORY_FREE_EX(AZ::Debug::ProfileCategory::MemoryReserved, file, line, ptr); - pointer_type newPtr = m_schema->ReAllocate(ptr, newSize, newAlignment); - AZ_PROFILE_MEMORY_ALLOC_EX(AZ::Debug::ProfileCategory::MemoryReserved, file, line, newPtr, newSize, "LegacyAllocator Realloc"); - AZ_MEMORY_PROFILE(ProfileReallocationEnd(ptr, newPtr, newSize, newAlignment)); - AZ_Assert(newPtr || newSize == 0, "OOM - Failed to reallocate %zu bytes from LegacyAllocator", newSize); - return newPtr; - } + void DeAllocate(pointer_type ptr, size_type byteSize = 0, size_type alignment = 0) override; - void DeAllocate(pointer_type ptr, size_type byteSize = 0, size_type alignment = 0) override - { - AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, 0, 0, nullptr)); - Base::DeAllocate(ptr, byteSize, alignment); - } - - pointer_type ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment) override - { - if (newAlignment == 0) - { - // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement - // Take a look at _Allocate_manually_vector_aligned in xmemory0 - newAlignment = sizeof(void*) * 2; - } - - AZ_MEMORY_PROFILE(ProfileReallocationBegin(ptr, newSize)); - pointer_type newPtr = Base::ReAllocate(ptr, newSize, newAlignment); - AZ_MEMORY_PROFILE(ProfileReallocationEnd(ptr, newPtr, newSize, newAlignment)); - AZ_Assert(newPtr || newSize == 0, "OOM - Failed to reallocate %zu bytes from LegacyAllocator", newSize); - return newPtr; - } + pointer_type ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment) override; }; using StdLegacyAllocator = AZStdAlloc; @@ -133,57 +51,4 @@ namespace AZ class AllocatorInstance : public Internal::AllocatorInstanceBase { }; - -#if defined(AZ_PLATFORM_PROVO) || defined(AZ_PLATFORM_JASPER) - struct GlobalAllocatorDescriptor - : public AZ::HphaSchema::Descriptor - { - GlobalAllocatorDescriptor() - { - // pull 1MB from the OS at a time - m_systemChunkSize = 1024 * 1024; - } - }; - - class GlobalAllocator - : public SimpleSchemaAllocator - { - public: - AZ_TYPE_INFO(GlobalAllocator, "{BC7861DA-AF7F-4FFD-A2F5-BAD89BDD77FD}"); - - using Descriptor = GlobalAllocatorDescriptor; - using Base = SimpleSchemaAllocator; - - GlobalAllocator() - : Base("GlobalAllocator", "Allocator for untracked new/delete/malloc/free") - { - } - - //--------------------------------------------------------------------- - // IAllocatorAllocate - //--------------------------------------------------------------------- - pointer_type Allocate(size_type byteSize, size_type alignment, int flags = 0, const char* name = 0, const char* fileName = 0, int lineNum = 0, unsigned int suppressStackRecord = 0) override - { - // Note: We cannot put the asserts in the AllocateBase class because various allocators depend on allocations failing from some heap classes. - pointer_type ptr = Base::Allocate(byteSize, alignment, flags, name, fileName, lineNum, suppressStackRecord); - AZ_Assert(ptr, "OOM - Failed to allocate %zu bytes from GlobalAllocator", byteSize); - return ptr; - } - - pointer_type ReAllocate(pointer_type ptr, size_type newSize, size_type newAlignment) override - { - pointer_type newPtr = Base::ReAllocate(ptr, newSize, newAlignment); - AZ_Assert(newPtr, "OOM - Failed to reallocate %zu bytes from GlobalAllocator", newSize); - return newPtr; - } - - }; - - // Specialize for the GlobalAllocator to provide one per module that does not use the - // environment for its storage - template <> - class AllocatorInstance : public Internal::AllocatorInstanceBase> - { - }; -#endif } diff --git a/Code/Legacy/CryCommon/crycommon_files.cmake b/Code/Legacy/CryCommon/crycommon_files.cmake index 8d33b85eff..d3613baed6 100644 --- a/Code/Legacy/CryCommon/crycommon_files.cmake +++ b/Code/Legacy/CryCommon/crycommon_files.cmake @@ -92,6 +92,7 @@ set(FILES CryVersion.h FrameProfiler.h HeapAllocator.h + LegacyAllocator.cpp LegacyAllocator.h MetaUtils.h MiniQueue.h From b26107e98df688e1d411c397bdbb6e696d633d45 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Fri, 13 Aug 2021 10:07:35 -0700 Subject: [PATCH 05/14] Fix for NameDictionary Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- .../AzCore/AzCore/Name/NameDictionary.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp b/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp index c04ae0ea5e..4c4d6b31e8 100644 --- a/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp +++ b/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp @@ -185,8 +185,25 @@ namespace AZ return; } + // Get the hash before locking since another thread could be deleting the object within the lock + Internal::NameData::Hash hash = nameData->GetHash(); + AZStd::unique_lock lock(m_sharedMutex); + auto dictIt = m_dictionary.find(hash); + if (dictIt == m_dictionary.end()) + { + // This check is to safeguard around the following scenario + // T1, gets into TryReleaseName + // T2 gets into MakeName, acquires the lock, returns a new Name that increments the counter + // T2 deletes the Name decrements the counter, gets into TryReleaseName + // T1 gets the lock, goes to the compare_exchange if and has a counter of 0, deletes + // Then T2 continues, gets the lock and crashes because nameData was deleted + return; + } + + nameData = dictIt->second; // restore the pointer in case the intrusive ptr was already assigned by other thread + // Check m_hashCollision again inside the m_sharedMutex because a new collision could have happened // on another thread before taking the lock. if (nameData->m_hashCollision) From 7be2b0b6c37165104b80b6248370af61d6b33386 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 11:49:51 -0700 Subject: [PATCH 06/14] Fix for a leaky test Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/Name/Name.h | 6 ++++ .../Framework/AzCore/Tests/Name/NameTests.cpp | 31 ++++++++++++++----- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Name/Name.h b/Code/Framework/AzCore/AzCore/Name/Name.h index 46a9b5b7cc..16179c9cde 100644 --- a/Code/Framework/AzCore/AzCore/Name/Name.h +++ b/Code/Framework/AzCore/AzCore/Name/Name.h @@ -10,6 +10,11 @@ #include +namespace UnitTest +{ + class NameTest; +} + namespace AZ { class NameDictionary; @@ -29,6 +34,7 @@ namespace AZ class Name { friend NameDictionary; + friend UnitTest::NameTest; public: using Hash = Internal::NameData::Hash; diff --git a/Code/Framework/AzCore/Tests/Name/NameTests.cpp b/Code/Framework/AzCore/Tests/Name/NameTests.cpp index 3b6310b1de..a46c1724f6 100644 --- a/Code/Framework/AzCore/Tests/Name/NameTests.cpp +++ b/Code/Framework/AzCore/Tests/Name/NameTests.cpp @@ -171,7 +171,17 @@ namespace UnitTest azsnprintf(buffer, RandomStringBufferSize, "%d", m_random.GetRandom()); return buffer; } - + + AZ::Internal::NameData* GetNameData(AZ::Name& name) + { + return name.m_data.get(); + } + + void FreeMemoryFromNameData(AZ::Internal::NameData* nameData) + { + delete nameData; + } + AZ::SimpleLcgRandom m_random; }; @@ -488,13 +498,20 @@ namespace UnitTest TEST_F(NameTest, ReportLeakedNames) { - AZ::Name leakedName{"hello"}; - AZ_TEST_START_TRACE_SUPPRESSION; - AZ::NameDictionary::Destroy(); - AZ_TEST_STOP_TRACE_SUPPRESSION(1); + AZ::Internal::NameData* leakedNameData = nullptr; + { + AZ::Name leakedName{ "hello" }; + AZ_TEST_START_TRACE_SUPPRESSION; + AZ::NameDictionary::Destroy(); + AZ_TEST_STOP_TRACE_SUPPRESSION(1); - // Create the dictionary again to avoid error in TearDown() - AZ::NameDictionary::Create(); + leakedNameData = GetNameData(leakedName); + + // Create the dictionary again to avoid crash when the intrusive_ptr in Name tries to access NameDictionary to free it + AZ::NameDictionary::Create(); + } + + FreeMemoryFromNameData(leakedNameData); // free it to avoid memory system reporting the leak } TEST_F(NameTest, NullTerminatedTest) From e29479552b3f892f76bc82b3386ad91965024b5d Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 15:53:14 -0700 Subject: [PATCH 07/14] alignment fix Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/Tests/Math/SfmtTests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp b/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp index f40246b8e9..397e8bd24f 100644 --- a/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp +++ b/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp @@ -27,8 +27,8 @@ namespace UnitTest void SetUp() override { AllocatorsFixture::SetUp(); - array1 = (AZ::u64*)azmalloc(sizeof(AZ::u64) * 2 * (BLOCK_SIZE / 4), AZStd::alignment_of::value); - array2 = (AZ::u64*)azmalloc(sizeof(AZ::u64) * 2 * (10000 / 4), AZStd::alignment_of::value); + array1 = (AZ::u64*)azmalloc(sizeof(AZ::u64) * 2 * (BLOCK_SIZE / 4), AZStd::alignment_of::value); + array2 = (AZ::u64*)azmalloc(sizeof(AZ::u64) * 2 * (10000 / 4), AZStd::alignment_of::value); } void TearDown() override From 4adf5c051e73df2b5dfdd8ebb0d92a9ad4c15843 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 16:16:19 -0700 Subject: [PATCH 08/14] more NameTest fixes, AzCore passing ASan Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/Tests/Name/NameTests.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Code/Framework/AzCore/Tests/Name/NameTests.cpp b/Code/Framework/AzCore/Tests/Name/NameTests.cpp index a46c1724f6..34b0950aea 100644 --- a/Code/Framework/AzCore/Tests/Name/NameTests.cpp +++ b/Code/Framework/AzCore/Tests/Name/NameTests.cpp @@ -604,7 +604,8 @@ namespace UnitTest AZ::NameDictionary::Create(); // 3 threads per name effectively makes two readers and one writer (the first to run will write in the dictionary) - RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT, 3); + // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures + RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2, 3); } TEST_F(NameTest, ConcurrencyDataTest_EachThreadCreatesOneName_HighCollisions) @@ -614,7 +615,8 @@ namespace UnitTest AZ::NameDictionary::Create(); // 3 threads per name effectively makes two readers and one writer (the first to run will write in the dictionary) - RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT, 3); + // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures + RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2, 3); } TEST_F(NameTest, ConcurrencyDataTest_EachThreadRepeatedlyCreatesAndReleasesOneName_NoCollision) From a49e07c8e9451f1d028a84aed5e2f76c73466e1f Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 16:20:39 -0700 Subject: [PATCH 09/14] improving comment Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/Tests/Name/NameTests.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Code/Framework/AzCore/Tests/Name/NameTests.cpp b/Code/Framework/AzCore/Tests/Name/NameTests.cpp index 34b0950aea..c0dc37715f 100644 --- a/Code/Framework/AzCore/Tests/Name/NameTests.cpp +++ b/Code/Framework/AzCore/Tests/Name/NameTests.cpp @@ -604,7 +604,8 @@ namespace UnitTest AZ::NameDictionary::Create(); // 3 threads per name effectively makes two readers and one writer (the first to run will write in the dictionary) - // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures + // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures, + // likely because of https://devblogs.microsoft.com/oldnewthing/20050729-14/?p=34773 RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2, 3); } @@ -615,7 +616,8 @@ namespace UnitTest AZ::NameDictionary::Create(); // 3 threads per name effectively makes two readers and one writer (the first to run will write in the dictionary) - // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures + // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures, + // likely because of https://devblogs.microsoft.com/oldnewthing/20050729-14/?p=34773 RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2, 3); } From ea01904ecf33043ea44fc3f58a2f710e90aa7c0c Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 17:40:49 -0700 Subject: [PATCH 10/14] PR comments Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Legacy/CryCommon/LegacyAllocator.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Code/Legacy/CryCommon/LegacyAllocator.cpp b/Code/Legacy/CryCommon/LegacyAllocator.cpp index 9c4258530a..76c4712bd3 100644 --- a/Code/Legacy/CryCommon/LegacyAllocator.cpp +++ b/Code/Legacy/CryCommon/LegacyAllocator.cpp @@ -6,8 +6,6 @@ * */ -#pragma once - #include namespace AZ From 1d7f690e06a06f58cec23d3a8fc5377a14a6dfff Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 18:02:49 -0700 Subject: [PATCH 11/14] missing header includes Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- .../Plugins/EditorCommon/DrawingPrimitives/TimeSlider.cpp | 2 ++ Code/Tools/ProjectManager/Source/GemCatalog/GemModel.cpp | 1 + Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/ShaderSemantic.h | 1 + Gems/Atom/RHI/Code/Include/Atom/RHI/DispatchItem.h | 1 + .../Code/Source/Debug/MultiplayerDebugByteReporter.cpp | 2 ++ Gems/MultiplayerCompression/Code/Source/LZ4Compressor.h | 1 + .../Generation/Components/MeshOptimizer/MeshBuilderSubMesh.cpp | 1 + 7 files changed, 9 insertions(+) diff --git a/Code/Editor/Plugins/EditorCommon/DrawingPrimitives/TimeSlider.cpp b/Code/Editor/Plugins/EditorCommon/DrawingPrimitives/TimeSlider.cpp index 4774520d0c..291723ff6b 100644 --- a/Code/Editor/Plugins/EditorCommon/DrawingPrimitives/TimeSlider.cpp +++ b/Code/Editor/Plugins/EditorCommon/DrawingPrimitives/TimeSlider.cpp @@ -12,6 +12,8 @@ #include #include +#include + namespace DrawingPrimitives { void DrawTimeSlider(QPainter& painter, const QPalette& palette, const STimeSliderOptions& options) diff --git a/Code/Tools/ProjectManager/Source/GemCatalog/GemModel.cpp b/Code/Tools/ProjectManager/Source/GemCatalog/GemModel.cpp index a15dffdc94..3781106bdc 100644 --- a/Code/Tools/ProjectManager/Source/GemCatalog/GemModel.cpp +++ b/Code/Tools/ProjectManager/Source/GemCatalog/GemModel.cpp @@ -8,6 +8,7 @@ #include #include +#include namespace O3DE::ProjectManager { diff --git a/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/ShaderSemantic.h b/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/ShaderSemantic.h index da791bddfb..ee6e0feeb6 100644 --- a/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/ShaderSemantic.h +++ b/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/ShaderSemantic.h @@ -9,6 +9,7 @@ #include #include +#include namespace AZ { diff --git a/Gems/Atom/RHI/Code/Include/Atom/RHI/DispatchItem.h b/Gems/Atom/RHI/Code/Include/Atom/RHI/DispatchItem.h index 93ff1e85b7..72f190dcc8 100644 --- a/Gems/Atom/RHI/Code/Include/Atom/RHI/DispatchItem.h +++ b/Gems/Atom/RHI/Code/Include/Atom/RHI/DispatchItem.h @@ -10,6 +10,7 @@ #include #include #include +#include namespace AZ { diff --git a/Gems/Multiplayer/Code/Source/Debug/MultiplayerDebugByteReporter.cpp b/Gems/Multiplayer/Code/Source/Debug/MultiplayerDebugByteReporter.cpp index 45305e3b39..7f904479d1 100644 --- a/Gems/Multiplayer/Code/Source/Debug/MultiplayerDebugByteReporter.cpp +++ b/Gems/Multiplayer/Code/Source/Debug/MultiplayerDebugByteReporter.cpp @@ -8,6 +8,8 @@ #include "MultiplayerDebugByteReporter.h" +#include + #include // for std::setfill #include #include diff --git a/Gems/MultiplayerCompression/Code/Source/LZ4Compressor.h b/Gems/MultiplayerCompression/Code/Source/LZ4Compressor.h index fc0c9e1089..640cf03ee4 100644 --- a/Gems/MultiplayerCompression/Code/Source/LZ4Compressor.h +++ b/Gems/MultiplayerCompression/Code/Source/LZ4Compressor.h @@ -10,6 +10,7 @@ #include #include +#include namespace MultiplayerCompression { diff --git a/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshBuilderSubMesh.cpp b/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshBuilderSubMesh.cpp index 170549a25d..c10a8fb2ed 100644 --- a/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshBuilderSubMesh.cpp +++ b/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshBuilderSubMesh.cpp @@ -7,6 +7,7 @@ */ #include +#include #include "MeshBuilder.h" #include "MeshBuilderSkinningInfo.h" #include "MeshBuilderSubMesh.h" From ac7be2fb5a69f9a2517cf7d14545951995e830e5 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Tue, 17 Aug 2021 18:29:24 -0700 Subject: [PATCH 12/14] PR observation about another race condition Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- .../AzCore/AzCore/Name/Internal/NameData.cpp | 5 ++++- Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp | 12 ++---------- Code/Framework/AzCore/AzCore/Name/NameDictionary.h | 2 +- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Name/Internal/NameData.cpp b/Code/Framework/AzCore/AzCore/Name/Internal/NameData.cpp index cfbd640762..0086e68c6d 100644 --- a/Code/Framework/AzCore/AzCore/Name/Internal/NameData.cpp +++ b/Code/Framework/AzCore/AzCore/Name/Internal/NameData.cpp @@ -36,10 +36,13 @@ namespace AZ void NameData::release() { + // this could be released after we decrement the counter, therefore we will + // base the release on the hash which is stable + Hash hash = m_hash; AZ_Assert(m_useCount > 0, "m_useCount is already 0!"); if (m_useCount.fetch_sub(1) == 1) { - AZ::NameDictionary::Instance().TryReleaseName(this); + AZ::NameDictionary::Instance().TryReleaseName(hash); } } } diff --git a/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp b/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp index 4c4d6b31e8..6f8672bb92 100644 --- a/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp +++ b/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp @@ -166,7 +166,7 @@ namespace AZ } } - void NameDictionary::TryReleaseName(Internal::NameData* nameData) + void NameDictionary::TryReleaseName(Name::Hash hash) { // Note that we don't remove NameData from the dictionary if it has been involved in a collision. // This avoids specific edge cases where a Name object could get an incorrect hash value. Consider @@ -179,14 +179,6 @@ namespace AZ // the dictionary *again*, this time with hash value 1000. Name objects pointing to the original // entry and Name objects pointing to the new entry will fail comparison operations. - // Early exit to avoid locking the mutex unnecessarily. - if (nameData->m_hashCollision) - { - return; - } - - // Get the hash before locking since another thread could be deleting the object within the lock - Internal::NameData::Hash hash = nameData->GetHash(); AZStd::unique_lock lock(m_sharedMutex); @@ -202,7 +194,7 @@ namespace AZ return; } - nameData = dictIt->second; // restore the pointer in case the intrusive ptr was already assigned by other thread + Internal::NameData* nameData = dictIt->second; // restore the pointer in case the intrusive ptr was already assigned by other thread // Check m_hashCollision again inside the m_sharedMutex because a new collision could have happened // on another thread before taking the lock. diff --git a/Code/Framework/AzCore/AzCore/Name/NameDictionary.h b/Code/Framework/AzCore/AzCore/Name/NameDictionary.h index 7d4ffe80f6..fa13dbd682 100644 --- a/Code/Framework/AzCore/AzCore/Name/NameDictionary.h +++ b/Code/Framework/AzCore/AzCore/Name/NameDictionary.h @@ -83,7 +83,7 @@ namespace AZ // Attempts to release the name from the dictionary, but checks to make sure // a reference wasn't taken by another thread. - void TryReleaseName(Internal::NameData* data); + void TryReleaseName(Name::Hash hash); ////////////////////////////////////////////////////////////////////////// From dd80a3ebae87d2ffce5a077764937064f34227d2 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Wed, 18 Aug 2021 11:09:14 -0700 Subject: [PATCH 13/14] Linux and non-unity build fixes Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/Debug/IEventLogger.h | 1 + .../AzCore/AzCore/IO/ByteContainerStream.h | 1 + Code/Framework/AzCore/AzCore/IO/GenericStreams.cpp | 1 + .../Android/AzCore/AzCore_Traits_Android.h | 13 +++++++++++++ .../Platform/Linux/AzCore/AzCore_Traits_Linux.h | 14 ++++++++++++++ .../AzCore/Platform/Mac/AzCore/AzCore_Traits_Mac.h | 14 ++++++++++++++ .../Windows/AzCore/AzCore_Traits_Windows.h | 14 ++++++++++++++ .../AzCore/Platform/iOS/AzCore/AzCore_Traits_iOS.h | 14 ++++++++++++++ Code/Framework/AzCore/Tests/AZStd/String.cpp | 4 ++-- Code/Framework/AzCore/Tests/Math/SfmtTests.cpp | 1 + Code/Legacy/CryCommon/LegacyAllocator.cpp | 6 +++--- .../Include/Atom/Feature/Utils/IndexableList.h | 1 + .../Include/Atom/RHI.Reflect/CpuTimingStatistics.h | 1 + .../MysticQt/Source/KeyboardShortcutManager.cpp | 1 + 14 files changed, 81 insertions(+), 5 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Debug/IEventLogger.h b/Code/Framework/AzCore/AzCore/Debug/IEventLogger.h index a11b411ded..f5bca5fba6 100644 --- a/Code/Framework/AzCore/AzCore/Debug/IEventLogger.h +++ b/Code/Framework/AzCore/AzCore/Debug/IEventLogger.h @@ -13,6 +13,7 @@ #include #include #include +#include namespace AZ::Debug { diff --git a/Code/Framework/AzCore/AzCore/IO/ByteContainerStream.h b/Code/Framework/AzCore/AzCore/IO/ByteContainerStream.h index 17f72b9012..d198f47e00 100644 --- a/Code/Framework/AzCore/AzCore/IO/ByteContainerStream.h +++ b/Code/Framework/AzCore/AzCore/IO/ByteContainerStream.h @@ -12,6 +12,7 @@ #include #include #include +#include namespace AZ { diff --git a/Code/Framework/AzCore/AzCore/IO/GenericStreams.cpp b/Code/Framework/AzCore/AzCore/IO/GenericStreams.cpp index b3ab808711..e7ccf4c83a 100644 --- a/Code/Framework/AzCore/AzCore/IO/GenericStreams.cpp +++ b/Code/Framework/AzCore/AzCore/IO/GenericStreams.cpp @@ -11,6 +11,7 @@ #include #include #include +#include namespace AZ::IO { diff --git a/Code/Framework/AzCore/Platform/Android/AzCore/AzCore_Traits_Android.h b/Code/Framework/AzCore/Platform/Android/AzCore/AzCore_Traits_Android.h index 1b67f3b720..495c8d5f2c 100644 --- a/Code/Framework/AzCore/Platform/Android/AzCore/AzCore_Traits_Android.h +++ b/Code/Framework/AzCore/Platform/Android/AzCore/AzCore_Traits_Android.h @@ -109,6 +109,19 @@ #define AZ_TRAIT_USE_ERRNO_T_TYPEDEF 1 #define AZ_TRAIT_USE_POSIX_TEMP_FOLDER 0 +// wchar_t/char formatting +// Reason: https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-160 +// The Z type character, and the behavior of the c, C, s, and S type characters when they're used with the printf and wprintf functions, +// are Microsoft extensions. The ISO C standard uses c and s consistently for narrow characters and strings, and C and S for wide characters +// and strings, in all formatting functions. +#define AZ_TRAIT_FORMAT_STRING_PRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WSTRING "%S" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WSTRING "%S" // Legacy traits ... #define AZ_TRAIT_LEGACY_CRYCOMMON_USE_WINDOWS_STUBS 1 #define AZ_TRAIT_LEGACY_CRYPAK_UNIX_LIKE_FILE_SYSTEM 1 diff --git a/Code/Framework/AzCore/Platform/Linux/AzCore/AzCore_Traits_Linux.h b/Code/Framework/AzCore/Platform/Linux/AzCore/AzCore_Traits_Linux.h index d36be0f61a..6ba369e86d 100644 --- a/Code/Framework/AzCore/Platform/Linux/AzCore/AzCore_Traits_Linux.h +++ b/Code/Framework/AzCore/Platform/Linux/AzCore/AzCore_Traits_Linux.h @@ -109,6 +109,20 @@ #define AZ_TRAIT_USE_ERRNO_T_TYPEDEF 1 #define AZ_TRAIT_USE_POSIX_TEMP_FOLDER 1 +// wchar_t/char formatting +// Reason: https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-160 +// The Z type character, and the behavior of the c, C, s, and S type characters when they're used with the printf and wprintf functions, +// are Microsoft extensions. The ISO C standard uses c and s consistently for narrow characters and strings, and C and S for wide characters +// and strings, in all formatting functions. +#define AZ_TRAIT_FORMAT_STRING_PRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WSTRING "%S" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WSTRING "%S" + // Legacy traits ... #define AZ_TRAIT_LEGACY_CRYCOMMON_USE_WINDOWS_STUBS 1 #define AZ_TRAIT_LEGACY_CRYPAK_UNIX_LIKE_FILE_SYSTEM 1 diff --git a/Code/Framework/AzCore/Platform/Mac/AzCore/AzCore_Traits_Mac.h b/Code/Framework/AzCore/Platform/Mac/AzCore/AzCore_Traits_Mac.h index b449cac072..a41b5c6baa 100644 --- a/Code/Framework/AzCore/Platform/Mac/AzCore/AzCore_Traits_Mac.h +++ b/Code/Framework/AzCore/Platform/Mac/AzCore/AzCore_Traits_Mac.h @@ -109,6 +109,20 @@ #define AZ_TRAIT_USE_ERRNO_T_TYPEDEF 0 #define AZ_TRAIT_USE_POSIX_TEMP_FOLDER 1 +// wchar_t/char formatting +// Reason: https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-160 +// The Z type character, and the behavior of the c, C, s, and S type characters when they're used with the printf and wprintf functions, +// are Microsoft extensions. The ISO C standard uses c and s consistently for narrow characters and strings, and C and S for wide characters +// and strings, in all formatting functions. +#define AZ_TRAIT_FORMAT_STRING_PRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WSTRING "%S" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WSTRING "%S" + // Legacy traits ... #define AZ_TRAIT_LEGACY_CRYCOMMON_USE_WINDOWS_STUBS 1 #define AZ_TRAIT_LEGACY_CRYPAK_UNIX_LIKE_FILE_SYSTEM 1 diff --git a/Code/Framework/AzCore/Platform/Windows/AzCore/AzCore_Traits_Windows.h b/Code/Framework/AzCore/Platform/Windows/AzCore/AzCore_Traits_Windows.h index e9a06740a0..2f9fcefdbd 100644 --- a/Code/Framework/AzCore/Platform/Windows/AzCore/AzCore_Traits_Windows.h +++ b/Code/Framework/AzCore/Platform/Windows/AzCore/AzCore_Traits_Windows.h @@ -109,6 +109,20 @@ #define AZ_TRAIT_USE_ERRNO_T_TYPEDEF 0 #define AZ_TRAIT_USE_POSIX_TEMP_FOLDER 0 +// wchar_t/char formatting +// Reason: https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-160 +// The Z type character, and the behavior of the c, C, s, and S type characters when they're used with the printf and wprintf functions, +// are Microsoft extensions. The ISO C standard uses c and s consistently for narrow characters and strings, and C and S for wide characters +// and strings, in all formatting functions. +#define AZ_TRAIT_FORMAT_STRING_PRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_CHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WCHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WSTRING "%S" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_STRING "%S" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WSTRING "%s" + // Legacy traits ... #define AZ_TRAIT_LEGACY_CRYCOMMON_USE_WINDOWS_STUBS 0 #define AZ_TRAIT_LEGACY_CRYPAK_UNIX_LIKE_FILE_SYSTEM 0 diff --git a/Code/Framework/AzCore/Platform/iOS/AzCore/AzCore_Traits_iOS.h b/Code/Framework/AzCore/Platform/iOS/AzCore/AzCore_Traits_iOS.h index 56bc747c09..d53f4b057e 100644 --- a/Code/Framework/AzCore/Platform/iOS/AzCore/AzCore_Traits_iOS.h +++ b/Code/Framework/AzCore/Platform/iOS/AzCore/AzCore_Traits_iOS.h @@ -110,6 +110,20 @@ #define AZ_TRAIT_USE_ERRNO_T_TYPEDEF 0 #define AZ_TRAIT_USE_POSIX_TEMP_FOLDER 0 +// wchar_t/char formatting +// Reason: https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-160 +// The Z type character, and the behavior of the c, C, s, and S type characters when they're used with the printf and wprintf functions, +// are Microsoft extensions. The ISO C standard uses c and s consistently for narrow characters and strings, and C and S for wide characters +// and strings, in all formatting functions. +#define AZ_TRAIT_FORMAT_STRING_PRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_CHAR "%c" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WCHAR "%C" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_PRINTF_WSTRING "%S" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_STRING "%s" +#define AZ_TRAIT_FORMAT_STRING_WPRINTF_WSTRING "%S" + // Legacy traits ... #define AZ_TRAIT_LEGACY_CRYCOMMON_USE_WINDOWS_STUBS 1 #define AZ_TRAIT_LEGACY_CRYPAK_UNIX_LIKE_FILE_SYSTEM 1 diff --git a/Code/Framework/AzCore/Tests/AZStd/String.cpp b/Code/Framework/AzCore/Tests/AZStd/String.cpp index a88de48b9b..0ff12a352c 100644 --- a/Code/Framework/AzCore/Tests/AZStd/String.cpp +++ b/Code/Framework/AzCore/Tests/AZStd/String.cpp @@ -1462,12 +1462,12 @@ namespace UnitTest // This shouldn't give a compile error AZStd::string::format( - "%i %c %uc %hc %lc %i %i %u %i %lu %li %llu %lli %f %f %hs %ls %p", + "%i %c %uc " AZ_TRAIT_FORMAT_STRING_PRINTF_CHAR AZ_TRAIT_FORMAT_STRING_PRINTF_WCHAR " %i %i %u %i %lu %li %llu %lli %f %f " AZ_TRAIT_FORMAT_STRING_PRINTF_STRING AZ_TRAIT_FORMAT_STRING_PRINTF_WSTRING " %p", v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18); // This shouldn't give a compile error AZStd::wstring::format( - L"%i %c %uc %hc %lc %i %i %u %i %lu %li %llu %lli %f %f %hs %ls %p", + L"%i %c %uc " AZ_TRAIT_FORMAT_STRING_WPRINTF_CHAR AZ_TRAIT_FORMAT_STRING_WPRINTF_WCHAR " %i %i %u %i %lu %li %llu %lli %f %f " AZ_TRAIT_FORMAT_STRING_WPRINTF_STRING AZ_TRAIT_FORMAT_STRING_WPRINTF_WSTRING " %p", v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18); class WrappedInt diff --git a/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp b/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp index 397e8bd24f..73fdd6e2fc 100644 --- a/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp +++ b/Code/Framework/AzCore/Tests/Math/SfmtTests.cpp @@ -8,6 +8,7 @@ #include #include +#include using namespace AZ; diff --git a/Code/Legacy/CryCommon/LegacyAllocator.cpp b/Code/Legacy/CryCommon/LegacyAllocator.cpp index 76c4712bd3..d4f66d4c42 100644 --- a/Code/Legacy/CryCommon/LegacyAllocator.cpp +++ b/Code/Legacy/CryCommon/LegacyAllocator.cpp @@ -14,7 +14,7 @@ namespace AZ { if (alignment == 0) { - // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement + // Some STL containers, like std::vector, seem to have a requirement where a specific minimum alignment will be chosen when the alignment is set to 0 // Take a look at _Allocate_manually_vector_aligned in xmemory0 alignment = sizeof(void*) * 2; } @@ -39,7 +39,7 @@ namespace AZ { if (newAlignment == 0) { - // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement + // Some STL containers, like std::vector, seem to have a requirement where a specific minimum alignment will be chosen when the alignment is set to 0 // Take a look at _Allocate_manually_vector_aligned in xmemory0 newAlignment = sizeof(void*) * 2; } @@ -63,7 +63,7 @@ namespace AZ { if (newAlignment == 0) { - // Some STL containers, like std::vector, are assuming a specific minimum alignment. seems to have a requirement + // Some STL containers, like std::vector, seem to have a requirement where a specific minimum alignment will be chosen when the alignment is set to 0 // Take a look at _Allocate_manually_vector_aligned in xmemory0 newAlignment = sizeof(void*) * 2; } diff --git a/Gems/Atom/Feature/Common/Code/Include/Atom/Feature/Utils/IndexableList.h b/Gems/Atom/Feature/Common/Code/Include/Atom/Feature/Utils/IndexableList.h index fbc524314a..aae229ac9d 100644 --- a/Gems/Atom/Feature/Common/Code/Include/Atom/Feature/Utils/IndexableList.h +++ b/Gems/Atom/Feature/Common/Code/Include/Atom/Feature/Utils/IndexableList.h @@ -9,6 +9,7 @@ #pragma once #include +#include namespace AZ { diff --git a/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/CpuTimingStatistics.h b/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/CpuTimingStatistics.h index 99751bbde7..2ab23def08 100644 --- a/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/CpuTimingStatistics.h +++ b/Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/CpuTimingStatistics.h @@ -12,6 +12,7 @@ #include #include #include +#include namespace AZ { diff --git a/Gems/EMotionFX/Code/MysticQt/Source/KeyboardShortcutManager.cpp b/Gems/EMotionFX/Code/MysticQt/Source/KeyboardShortcutManager.cpp index 6b381bf9d8..ffedae5a99 100644 --- a/Gems/EMotionFX/Code/MysticQt/Source/KeyboardShortcutManager.cpp +++ b/Gems/EMotionFX/Code/MysticQt/Source/KeyboardShortcutManager.cpp @@ -10,6 +10,7 @@ #include "KeyboardShortcutManager.h" #include #include +#include #include #include From cb7108e336d53449ccf87a759f80f6520b4a0ca5 Mon Sep 17 00:00:00 2001 From: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> Date: Wed, 18 Aug 2021 17:51:43 -0700 Subject: [PATCH 14/14] Addresses comments around AZ_TRAIT_UNIT_TEST_NAME_COUNT from PR Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp | 4 ++-- Code/Framework/AzCore/Tests/Name/NameTests.cpp | 12 ++++-------- .../AzTest/Platform/Android/AzTest_Traits_Android.h | 1 - .../AzTest/Platform/Linux/AzTest_Traits_Linux.h | 1 - .../AzTest/AzTest/Platform/Mac/AzTest_Traits_Mac.h | 1 - .../AzTest/Platform/Windows/AzTest_Traits_Windows.h | 1 - .../AzTest/AzTest/Platform/iOS/AzTest_Traits_iOS.h | 1 - 7 files changed, 6 insertions(+), 15 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp b/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp index 6f8672bb92..cf85e0f4e0 100644 --- a/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp +++ b/Code/Framework/AzCore/AzCore/Name/NameDictionary.cpp @@ -194,9 +194,9 @@ namespace AZ return; } - Internal::NameData* nameData = dictIt->second; // restore the pointer in case the intrusive ptr was already assigned by other thread + Internal::NameData* nameData = dictIt->second; - // Check m_hashCollision again inside the m_sharedMutex because a new collision could have happened + // Check m_hashCollision inside the m_sharedMutex because a new collision could have happened // on another thread before taking the lock. if (nameData->m_hashCollision) { diff --git a/Code/Framework/AzCore/Tests/Name/NameTests.cpp b/Code/Framework/AzCore/Tests/Name/NameTests.cpp index c0dc37715f..3db77ac4c4 100644 --- a/Code/Framework/AzCore/Tests/Name/NameTests.cpp +++ b/Code/Framework/AzCore/Tests/Name/NameTests.cpp @@ -604,9 +604,7 @@ namespace UnitTest AZ::NameDictionary::Create(); // 3 threads per name effectively makes two readers and one writer (the first to run will write in the dictionary) - // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures, - // likely because of https://devblogs.microsoft.com/oldnewthing/20050729-14/?p=34773 - RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2, 3); + RunConcurrencyTest(AZStd::thread::hardware_concurrency(), 3); } TEST_F(NameTest, ConcurrencyDataTest_EachThreadCreatesOneName_HighCollisions) @@ -616,9 +614,7 @@ namespace UnitTest AZ::NameDictionary::Create(); // 3 threads per name effectively makes two readers and one writer (the first to run will write in the dictionary) - // Using AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2 since the following line generates 3000 threads and that triggers ASan failures, - // likely because of https://devblogs.microsoft.com/oldnewthing/20050729-14/?p=34773 - RunConcurrencyTest(AZ_TRAIT_UNIT_TEST_NAME_COUNT / 2, 3); + RunConcurrencyTest(AZStd::thread::hardware_concurrency() / 2, 3); } TEST_F(NameTest, ConcurrencyDataTest_EachThreadRepeatedlyCreatesAndReleasesOneName_NoCollision) @@ -645,7 +641,7 @@ namespace UnitTest TEST_F(NameTest, DISABLED_NameVsStringPerf_Creation) { - constexpr int CreateCount = AZ_TRAIT_UNIT_TEST_NAME_COUNT; + constexpr int CreateCount = 1000; char buffer[RandomStringBufferSize]; @@ -654,7 +650,7 @@ namespace UnitTest AZStd::sys_time_t stringTime; { - const size_t dictionaryNoiseSize = AZ_TRAIT_UNIT_TEST_NAME_COUNT; + const size_t dictionaryNoiseSize = 1000; AZStd::vector existingNames; existingNames.reserve(dictionaryNoiseSize); diff --git a/Code/Framework/AzTest/AzTest/Platform/Android/AzTest_Traits_Android.h b/Code/Framework/AzTest/AzTest/Platform/Android/AzTest_Traits_Android.h index b210fb9b62..a8b4b3cc8c 100644 --- a/Code/Framework/AzTest/AzTest/Platform/Android/AzTest_Traits_Android.h +++ b/Code/Framework/AzTest/AzTest/Platform/Android/AzTest_Traits_Android.h @@ -12,7 +12,6 @@ #define AZ_TRAIT_UNIT_TEST_ASSET_MANAGER_TEST_DEFAULT_TIMEOUT_SECS 5 #define AZ_TRAIT_UNIT_TEST_ENTITY_ID_GEN_TEST_COUNT 10000 #define AZ_TRAIT_UNIT_TEST_DILLER_TRIGGER_EVENT_COUNT 100000 -#define AZ_TRAIT_UNIT_TEST_NAME_COUNT 1000 #define AZ_TRAIT_TEST_APPEND_ROOT_FOLDER_TO_PATH true diff --git a/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h b/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h index 8b8d87a5b9..d9b48b7835 100644 --- a/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h +++ b/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h @@ -12,7 +12,6 @@ #define AZ_TRAIT_UNIT_TEST_ASSET_MANAGER_TEST_DEFAULT_TIMEOUT_SECS 5 #define AZ_TRAIT_UNIT_TEST_ENTITY_ID_GEN_TEST_COUNT 10000 #define AZ_TRAIT_UNIT_TEST_DILLER_TRIGGER_EVENT_COUNT 100000 -#define AZ_TRAIT_UNIT_TEST_NAME_COUNT 1000 #define AZ_TRAIT_DISABLE_ALL_SAVE_DATA_TESTS true diff --git a/Code/Framework/AzTest/AzTest/Platform/Mac/AzTest_Traits_Mac.h b/Code/Framework/AzTest/AzTest/Platform/Mac/AzTest_Traits_Mac.h index a43c62ac98..69dd592a2b 100644 --- a/Code/Framework/AzTest/AzTest/Platform/Mac/AzTest_Traits_Mac.h +++ b/Code/Framework/AzTest/AzTest/Platform/Mac/AzTest_Traits_Mac.h @@ -12,7 +12,6 @@ #define AZ_TRAIT_UNIT_TEST_ASSET_MANAGER_TEST_DEFAULT_TIMEOUT_SECS 5 #define AZ_TRAIT_UNIT_TEST_ENTITY_ID_GEN_TEST_COUNT 10000 #define AZ_TRAIT_UNIT_TEST_DILLER_TRIGGER_EVENT_COUNT 100000 -#define AZ_TRAIT_UNIT_TEST_NAME_COUNT 1000 #define AZ_TRAIT_DISABLE_ASSET_JOB_PARALLEL_TESTS true #define AZ_TRAIT_DISABLE_ASSET_MANAGER_FLOOD_TEST true diff --git a/Code/Framework/AzTest/AzTest/Platform/Windows/AzTest_Traits_Windows.h b/Code/Framework/AzTest/AzTest/Platform/Windows/AzTest_Traits_Windows.h index 3721d8891a..a11b8586f4 100644 --- a/Code/Framework/AzTest/AzTest/Platform/Windows/AzTest_Traits_Windows.h +++ b/Code/Framework/AzTest/AzTest/Platform/Windows/AzTest_Traits_Windows.h @@ -13,4 +13,3 @@ #define AZ_TRAIT_UNIT_TEST_ASSET_MANAGER_TEST_DEFAULT_TIMEOUT_SECS 5 #define AZ_TRAIT_UNIT_TEST_ENTITY_ID_GEN_TEST_COUNT 10000 #define AZ_TRAIT_UNIT_TEST_DILLER_TRIGGER_EVENT_COUNT 100000 -#define AZ_TRAIT_UNIT_TEST_NAME_COUNT 1000 diff --git a/Code/Framework/AzTest/AzTest/Platform/iOS/AzTest_Traits_iOS.h b/Code/Framework/AzTest/AzTest/Platform/iOS/AzTest_Traits_iOS.h index a43c62ac98..69dd592a2b 100644 --- a/Code/Framework/AzTest/AzTest/Platform/iOS/AzTest_Traits_iOS.h +++ b/Code/Framework/AzTest/AzTest/Platform/iOS/AzTest_Traits_iOS.h @@ -12,7 +12,6 @@ #define AZ_TRAIT_UNIT_TEST_ASSET_MANAGER_TEST_DEFAULT_TIMEOUT_SECS 5 #define AZ_TRAIT_UNIT_TEST_ENTITY_ID_GEN_TEST_COUNT 10000 #define AZ_TRAIT_UNIT_TEST_DILLER_TRIGGER_EVENT_COUNT 100000 -#define AZ_TRAIT_UNIT_TEST_NAME_COUNT 1000 #define AZ_TRAIT_DISABLE_ASSET_JOB_PARALLEL_TESTS true #define AZ_TRAIT_DISABLE_ASSET_MANAGER_FLOOD_TEST true