From d55a1ae2d48f1035a5700d40aec4b75b942ab82a Mon Sep 17 00:00:00 2001 From: Bryan Bernhart Date: Mon, 27 Jun 2022 16:24:30 -0700 Subject: [PATCH] Fix AV by merging events from exited thread. --- .github/workflows/win_msvc_rel_x64_cmake.yaml | 2 +- src/gpgmm/common/EventTraceWriter.cpp | 55 ++++++++++++++--- src/gpgmm/common/EventTraceWriter.h | 19 ++++-- src/gpgmm/common/TraceEvent.cpp | 7 +++ src/gpgmm/common/TraceEvent.h | 8 ++- src/tests/BUILD.gn | 1 + src/tests/CMakeLists.txt | 33 +++++----- src/tests/unittests/EventTraceWriterTests.cpp | 61 +++++++++++++++++++ 8 files changed, 155 insertions(+), 31 deletions(-) create mode 100644 src/tests/unittests/EventTraceWriterTests.cpp diff --git a/.github/workflows/win_msvc_rel_x64_cmake.yaml b/.github/workflows/win_msvc_rel_x64_cmake.yaml index 64b6e731d..73d98278a 100644 --- a/.github/workflows/win_msvc_rel_x64_cmake.yaml +++ b/.github/workflows/win_msvc_rel_x64_cmake.yaml @@ -71,7 +71,7 @@ jobs: shell: cmd run: | cd test - cmake . -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DCMAKE_TOOLCHAIN_FILE=..\vcpkg\scripts\buildsystems\vcpkg.cmake + cmake . -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DGPGMM_FORCE_TRACING=ON -DCMAKE_TOOLCHAIN_FILE=..\vcpkg\scripts\buildsystems\vcpkg.cmake - name: Build for main branch (with patch) shell: cmd diff --git a/src/gpgmm/common/EventTraceWriter.cpp b/src/gpgmm/common/EventTraceWriter.cpp index c26f913fe..056d51e31 100644 --- a/src/gpgmm/common/EventTraceWriter.cpp +++ b/src/gpgmm/common/EventTraceWriter.cpp @@ -28,6 +28,26 @@ namespace gpgmm { + // Trace buffer that flushes and unlinks itself from the cache once destroyed. + class ScopedTraceBufferInTLS { + public: + ScopedTraceBufferInTLS(EventTraceWriter* writer) : mWriter(writer) { + ASSERT(writer != nullptr); + } + + ~ScopedTraceBufferInTLS() { + mWriter->FlushAndRemoveBufferEntry(GetBuffer()); + } + + std::vector* GetBuffer() { + return &mBuffer; + } + + private: + EventTraceWriter* mWriter = nullptr; + std::vector mBuffer; + }; + EventTraceWriter::EventTraceWriter() : mTraceFile(kDefaultTraceFile), mPlatformTime(CreatePlatformTime()) { } @@ -182,25 +202,46 @@ namespace gpgmm { } std::vector* EventTraceWriter::GetOrCreateBufferFromTLS() { - thread_local std::unique_ptr> bufferInTLS; + thread_local std::unique_ptr bufferInTLS; if (bufferInTLS == nullptr) { - bufferInTLS.reset(new std::vector()); + bufferInTLS.reset(new ScopedTraceBufferInTLS(this)); std::lock_guard mutex(mMutex); mBufferPerThread[std::this_thread::get_id()] = bufferInTLS.get(); } ASSERT(bufferInTLS != nullptr); - return bufferInTLS.get(); + return bufferInTLS->GetBuffer(); } - std::vector EventTraceWriter::MergeAndClearBuffers() const { + void EventTraceWriter::FlushAndRemoveBufferEntry(std::vector* buffer) { + std::lock_guard mutex(mMutex); + const size_t removed = mBufferPerThread.erase(std::this_thread::get_id()); + ASSERT(removed == 1); + mUnmergedBuffer.insert(mUnmergedBuffer.end(), buffer->begin(), buffer->end()); + } + + std::vector EventTraceWriter::MergeAndClearBuffers() { std::vector mergedBuffer; + + mergedBuffer.insert(mergedBuffer.end(), mUnmergedBuffer.begin(), mUnmergedBuffer.end()); + mUnmergedBuffer.clear(); + for (auto& bufferOfThread : mBufferPerThread) { - mergedBuffer.insert(mergedBuffer.end(), bufferOfThread.second->begin(), - bufferOfThread.second->end()); - bufferOfThread.second->clear(); + std::vector* bufferToMerge = bufferOfThread.second->GetBuffer(); + mergedBuffer.insert(mergedBuffer.end(), bufferToMerge->begin(), bufferToMerge->end()); + bufferToMerge->clear(); } return mergedBuffer; } + size_t EventTraceWriter::GetQueuedEventsForTesting() const { + std::lock_guard mutex(mMutex); + size_t numOfEvents = 0; + numOfEvents += mUnmergedBuffer.size(); + for (auto& bufferOfThread : mBufferPerThread) { + numOfEvents += bufferOfThread.second->GetBuffer()->size(); + } + return numOfEvents; + } + } // namespace gpgmm diff --git a/src/gpgmm/common/EventTraceWriter.h b/src/gpgmm/common/EventTraceWriter.h index 3f66eaf03..7520a3eb9 100644 --- a/src/gpgmm/common/EventTraceWriter.h +++ b/src/gpgmm/common/EventTraceWriter.h @@ -25,37 +25,44 @@ namespace gpgmm { class PlatformTime; + class ScopedTraceBufferInTLS; class EventTraceWriter { public: EventTraceWriter(); + ~EventTraceWriter(); void SetConfiguration(const std::string& traceFile, const TraceEventPhase& ignoreMask, bool flushOnDestruct); - ~EventTraceWriter(); - void EnqueueTraceEvent(char phase, TraceEventCategory category, const char* name, uint64_t id, uint32_t flags, const JSONDict& args); + void FlushQueuedEventsToDisk(); + void FlushAndRemoveBufferEntry(std::vector* buffer); + + size_t GetQueuedEventsForTesting() const; + private: std::vector* GetOrCreateBufferFromTLS(); - std::vector MergeAndClearBuffers() const; + std::vector MergeAndClearBuffers(); std::string mTraceFile; std::unique_ptr mPlatformTime; - mutable std::mutex mMutex; - - std::unordered_map*> mBufferPerThread; TraceEventPhase mIgnoreMask; bool mFlushOnDestruct = true; + + mutable std::mutex mMutex; + + std::unordered_map mBufferPerThread; + std::vector mUnmergedBuffer; }; } // namespace gpgmm diff --git a/src/gpgmm/common/TraceEvent.cpp b/src/gpgmm/common/TraceEvent.cpp index 4b6adca07..e5be1537c 100644 --- a/src/gpgmm/common/TraceEvent.cpp +++ b/src/gpgmm/common/TraceEvent.cpp @@ -54,6 +54,13 @@ namespace gpgmm { return gEventTrace != nullptr; } + size_t GetQueuedEventsForTesting() { + if (!IsEventTraceEnabled()) { + return 0; + } + return GetInstance()->GetQueuedEventsForTesting(); + } + TraceEvent::TraceEvent(char phase, TraceEventCategory category, const std::string& name, diff --git a/src/gpgmm/common/TraceEvent.h b/src/gpgmm/common/TraceEvent.h index 21b7f3a62..86f3c73ce 100644 --- a/src/gpgmm/common/TraceEvent.h +++ b/src/gpgmm/common/TraceEvent.h @@ -39,7 +39,8 @@ #ifdef GPGMM_DISABLE_TRACING #define TRACE_EVENT0(category_group, name) TRACE_EMPTY -#define TRACE_EVENT_INSTANT0(category_group, name, scope, args) TRACE_EMPTY +#define TRACE_EVENT_INSTANT0(category_group, name) TRACE_EMPTY +#define TRACE_EVENT_INSTANT1(category_group, name, args) TRACE_EMPTY #define TRACE_COUNTER1(category_group, name, value) TRACE_EMPTY #define TRACE_EVENT_METADATA1(category_group, name, arg1_name, arg1_value) TRACE_EMPTY @@ -67,6 +68,9 @@ const uint64_t kNoId = 0; INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_COUNTER, category_group, name, TRACE_EVENT_FLAG_NONE, "value", \ static_cast(value)) +#define TRACE_EVENT_INSTANT0(category_group, name) \ + INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, category_group, name, TRACE_EVENT_FLAG_NONE) + #define TRACE_EVENT_INSTANT1(category_group, name, args) \ INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, category_group, name, TRACE_EVENT_FLAG_NONE, args) @@ -179,6 +183,8 @@ namespace gpgmm { bool IsEventTraceEnabled(); + size_t GetQueuedEventsForTesting(); + class TraceEventID { public: explicit TraceEventID(const void* id) diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn index 028d58bef..b8055c35c 100644 --- a/src/tests/BUILD.gn +++ b/src/tests/BUILD.gn @@ -107,6 +107,7 @@ test("gpgmm_unittests") { "unittests/BuddyBlockAllocatorTests.cpp", "unittests/BuddyMemoryAllocatorTests.cpp", "unittests/ConditionalMemoryAllocatorTests.cpp", + "unittests/EventTraceWriterTests.cpp", "unittests/FlagsTests.cpp", "unittests/LinkedListTests.cpp", "unittests/MathTests.cpp", diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index 7d233ae6d..8b2a4cee6 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -23,22 +23,23 @@ target_link_libraries(gpgmm_unittests PRIVATE ) target_sources(gpgmm_unittests PRIVATE - "DummyMemoryAllocator.h" - "unittests/BuddyBlockAllocatorTests.cpp" - "unittests/ConditionalMemoryAllocatorTests.cpp" - "unittests/FlagsTests.cpp" - "unittests/LinkedListTests.cpp" - "unittests/MathTests.cpp" - "unittests/MemoryAllocatorTests.cpp" - "unittests/MemoryCacheTests.cpp" - "unittests/MemoryPoolTests.cpp" - "unittests/PooledMemoryAllocatorTests.cpp" - "unittests/RefCountTests.cpp" - "unittests/SegmentedMemoryAllocatorTests.cpp" - "unittests/SlabBlockAllocatorTests.cpp" - "unittests/SlabMemoryAllocatorTests.cpp" - "unittests/UtilsTest.cpp" - "UnittestsMain.cpp" + "DummyMemoryAllocator.h" + "unittests/BuddyBlockAllocatorTests.cpp" + "unittests/ConditionalMemoryAllocatorTests.cpp" + "unittests/EventTraceWriterTests.cpp" + "unittests/FlagsTests.cpp" + "unittests/LinkedListTests.cpp" + "unittests/MathTests.cpp" + "unittests/MemoryAllocatorTests.cpp" + "unittests/MemoryCacheTests.cpp" + "unittests/MemoryPoolTests.cpp" + "unittests/PooledMemoryAllocatorTests.cpp" + "unittests/RefCountTests.cpp" + "unittests/SegmentedMemoryAllocatorTests.cpp" + "unittests/SlabBlockAllocatorTests.cpp" + "unittests/SlabMemoryAllocatorTests.cpp" + "unittests/UtilsTest.cpp" + "UnittestsMain.cpp" ) target_link_libraries(gpgmm_unittests PRIVATE diff --git a/src/tests/unittests/EventTraceWriterTests.cpp b/src/tests/unittests/EventTraceWriterTests.cpp new file mode 100644 index 000000000..17b6b8f64 --- /dev/null +++ b/src/tests/unittests/EventTraceWriterTests.cpp @@ -0,0 +1,61 @@ +// Copyright 2021 The GPGMM Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include "gpgmm/common/TraceEvent.h" + +#include +#include + +static constexpr const char* kDummyTrace = "DummyTrace.json"; + +using namespace gpgmm; + +class EventTraceWriterTests : public testing::Test { + public: + void SetUp() override { + StartupEventTrace(kDummyTrace, TraceEventPhase::None, /*flushOnDestruct*/ true); + } + + void TearDown() override { + ShutdownEventTrace(); + } +}; + +TEST_F(EventTraceWriterTests, SingleThreadWrites) { + constexpr uint32_t kEventCount = 64u; + for (size_t i = 0; i < kEventCount; i++) { + TRACE_EVENT_INSTANT0(TraceEventCategory::Default, "InstantEvent"); + } + + // 1 event per thread + 1 metadata event for main thread name. + EXPECT_EQ(GetQueuedEventsForTesting(), 64 + 1u); +} + +TEST_F(EventTraceWriterTests, MultiThreadWrites) { + constexpr uint32_t kThreadCount = 64u; + std::vector threads(kThreadCount); + for (size_t threadIdx = 0; threadIdx < threads.size(); threadIdx++) { + threads[threadIdx] = std::thread( + [&]() { TRACE_EVENT_INSTANT0(TraceEventCategory::Default, "InstantEvent"); }); + } + + for (std::thread& thread : threads) { + thread.join(); + } + + // 1 event per thread + 1 metadata event for main thread name. + EXPECT_EQ(GetQueuedEventsForTesting(), 64 + 1u); +}