From 6ae43c40de8cc35af3b3ba401fa6504ff782cd86 Mon Sep 17 00:00:00 2001 From: Matthew Bentham Date: Mon, 10 Jan 2022 13:34:12 +0000 Subject: Fix thread safety issues in TimelineDecoder and associated unit tests Enforce serialized access to TimelineDecoder::m_Model by removing public access funtion and replacing with an 'Apply' method taking a lambda and uses a std::lock. Use the new lambda when invoking callbacks. Change-Id: I6ea2fbca990736f3be63e80897f175421f19f0c1 Signed-off-by: Matthew Bentham --- .../include/timelineDecoder/TimelineDecoder.hpp | 11 ++- .../server/src/timelineDecoder/TimelineDecoder.cpp | 28 +++--- .../src/timelineDecoder/tests/TimelineTests.cpp | 77 +++++++-------- src/backends/backendsCommon/test/MockBackend.hpp | 2 +- tests/profiling/gatordmock/GatordMockService.hpp | 2 +- .../profiling/gatordmock/tests/GatordMockTests.cpp | 106 +++++++++++---------- 6 files changed, 124 insertions(+), 102 deletions(-) diff --git a/profiling/server/include/timelineDecoder/TimelineDecoder.hpp b/profiling/server/include/timelineDecoder/TimelineDecoder.hpp index ea4b144860..9776ec91f9 100644 --- a/profiling/server/include/timelineDecoder/TimelineDecoder.hpp +++ b/profiling/server/include/timelineDecoder/TimelineDecoder.hpp @@ -6,6 +6,8 @@ #include "ITimelineDecoder.hpp" +#include +#include #include namespace arm @@ -40,7 +42,11 @@ public: virtual TimelineStatus CreateLabel(const Label &) override; virtual TimelineStatus CreateRelationship(const Relationship &) override; - const Model& GetModel(); + template + decltype(auto) ApplyToModel(F&& f){ + std::lock_guard lock(m_ModelMutex); + return f(m_Model); + } TimelineStatus SetEntityCallback(const OnNewEntityCallback); TimelineStatus SetEventClassCallback(const OnNewEventClassCallback); @@ -54,6 +60,7 @@ public: private: Model m_Model; + std::mutex m_ModelMutex; OnNewEntityCallback m_OnNewEntityCallback; OnNewEventClassCallback m_OnNewEventClassCallback; @@ -69,4 +76,4 @@ private: }; } // namespace pipe -} // namespace arm \ No newline at end of file +} // namespace arm diff --git a/profiling/server/src/timelineDecoder/TimelineDecoder.cpp b/profiling/server/src/timelineDecoder/TimelineDecoder.cpp index df967de1f5..6eaaff6e54 100644 --- a/profiling/server/src/timelineDecoder/TimelineDecoder.cpp +++ b/profiling/server/src/timelineDecoder/TimelineDecoder.cpp @@ -20,8 +20,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateEntity(const Entity &enti { return TimelineStatus::TimelineStatus_Fail; } - m_OnNewEntityCallback(m_Model, entity); - + ApplyToModel([&](Model& m){ + m_OnNewEntityCallback(m, entity); + }); return TimelineStatus::TimelineStatus_Success; } @@ -31,7 +32,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateEventClass(const EventCla { return TimelineStatus::TimelineStatus_Fail; } - m_OnNewEventClassCallback(m_Model, eventClass); + ApplyToModel([&](Model& m){ + m_OnNewEventClassCallback(m, eventClass); + }); return TimelineStatus::TimelineStatus_Success; } @@ -42,7 +45,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateEvent(const Event &event) { return TimelineStatus::TimelineStatus_Fail; } - m_OnNewEventCallback(m_Model, event); + ApplyToModel([&](Model& m){ + m_OnNewEventCallback(m, event); + }); return TimelineStatus::TimelineStatus_Success; } @@ -53,7 +58,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateLabel(const Label &label) { return TimelineStatus::TimelineStatus_Fail; } - m_OnNewLabelCallback(m_Model, label); + ApplyToModel([&](Model& m){ + m_OnNewLabelCallback(m, label); + }); return TimelineStatus::TimelineStatus_Success; } @@ -64,15 +71,12 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateRelationship(const Relati { return TimelineStatus::TimelineStatus_Fail; } - m_OnNewRelationshipCallback(m_Model, relationship); + ApplyToModel([&](Model& m){ + m_OnNewRelationshipCallback(m, relationship); + }); return TimelineStatus::TimelineStatus_Success; } -const TimelineDecoder::Model &TimelineDecoder::GetModel() -{ - return m_Model; -} - TimelineDecoder::TimelineStatus TimelineDecoder::SetEntityCallback(OnNewEntityCallback cb) { if (cb == nullptr) @@ -323,4 +327,4 @@ void TimelineDecoder::printRelationships() } } // namespace pipe -} // namespace arm \ No newline at end of file +} // namespace arm diff --git a/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp b/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp index f09848f52c..82c16fee79 100644 --- a/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp +++ b/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp @@ -154,8 +154,6 @@ TEST_CASE("TimelineCaptureTest") arm::pipe::PacketVersionResolver packetVersionResolver; arm::pipe::TimelineDecoder timelineDecoder; - const arm::pipe::TimelineDecoder::Model& model = timelineDecoder.GetModel(); - arm::pipe::TimelineCaptureCommandHandler timelineCaptureCommandHandler( 1, 1, packetVersionResolver.ResolvePacketVersion(1, 1).GetEncodedValue(), timelineDecoder, @@ -238,25 +236,27 @@ TEST_CASE("TimelineCaptureTest") timelineCaptureCommandHandler); } - for (unsigned long i = 0; i < 10; ++i) - { - CHECK(model.m_Entities[i].m_Guid == entityGuid); + timelineDecoder.ApplyToModel([&](const arm::pipe::TimelineDecoder::Model& model){ + for (unsigned long i = 0; i < 10; ++i) + { + CHECK(model.m_Entities[i].m_Guid == entityGuid); - CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid); + CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid); - CHECK(model.m_Events[i].m_TimeStamp == timestamp); - CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId); - CHECK(model.m_Events[i].m_Guid == eventGuid); + CHECK(model.m_Events[i].m_TimeStamp == timestamp); + CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId); + CHECK(model.m_Events[i].m_Guid == eventGuid); - CHECK(model.m_Labels[i].m_Guid == labelGuid); - CHECK(model.m_Labels[i].m_Name == labelName); + CHECK(model.m_Labels[i].m_Guid == labelGuid); + CHECK(model.m_Labels[i].m_Name == labelName); - CHECK(model.m_Relationships[i].m_RelationshipType == - arm::pipe::ITimelineDecoder::RelationshipType::DataLink); - CHECK(model.m_Relationships[i].m_Guid == relationshipGuid); - CHECK(model.m_Relationships[i].m_HeadGuid == headGuid); - CHECK(model.m_Relationships[i].m_TailGuid == tailGuid); - } + CHECK(model.m_Relationships[i].m_RelationshipType == + arm::pipe::ITimelineDecoder::RelationshipType::DataLink); + CHECK(model.m_Relationships[i].m_Guid == relationshipGuid); + CHECK(model.m_Relationships[i].m_HeadGuid == headGuid); + CHECK(model.m_Relationships[i].m_TailGuid == tailGuid); + } + }); } TEST_CASE("TimelineCaptureTestMultipleStringsInBuffer") @@ -270,7 +270,6 @@ TEST_CASE("TimelineCaptureTestMultipleStringsInBuffer") arm::pipe::PacketVersionResolver packetVersionResolver; arm::pipe::TimelineDecoder timelineDecoder; - const arm::pipe::TimelineDecoder::Model& model = timelineDecoder.GetModel(); arm::pipe::TimelineCaptureCommandHandler timelineCaptureCommandHandler( 1, 1, packetVersionResolver.ResolvePacketVersion(1, 1).GetEncodedValue(), timelineDecoder, @@ -343,30 +342,32 @@ TEST_CASE("TimelineCaptureTestMultipleStringsInBuffer") SendTimelinePacketToCommandHandler(bufferManager.GetReadableBuffer()->GetReadableData(), timelineCaptureCommandHandler); - for ( unsigned long i = 0; i < 9; ++i ) - { - CHECK(model.m_Entities[i].m_Guid == entityGuid); + timelineDecoder.ApplyToModel([&](const arm::pipe::TimelineDecoder::Model& model){ + for ( unsigned long i = 0; i < 9; ++i ) + { + CHECK(model.m_Entities[i].m_Guid == entityGuid); - CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid); + CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid); - CHECK(model.m_Labels[i].m_Guid == labelGuid); + CHECK(model.m_Labels[i].m_Guid == labelGuid); - CHECK(model.m_Events[i].m_TimeStamp == timestamp); - CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId); - CHECK(model.m_Events[i].m_Guid == eventGuid); + CHECK(model.m_Events[i].m_TimeStamp == timestamp); + CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId); + CHECK(model.m_Events[i].m_Guid == eventGuid); - CHECK(model.m_Relationships[i].m_RelationshipType == - arm::pipe::ITimelineDecoder::RelationshipType::DataLink); - CHECK(model.m_Relationships[i].m_Guid == relationshipGuid); - CHECK(model.m_Relationships[i].m_HeadGuid == headGuid); - CHECK(model.m_Relationships[i].m_TailGuid == tailGuid); - } - for ( unsigned long i = 0; i < 9; i += 3 ) - { - CHECK(model.m_Labels[i].m_Name == labelName); - CHECK(model.m_Labels[i+1].m_Name == labelName2); - CHECK(model.m_Labels[i+2].m_Name == labelName3); - } + CHECK(model.m_Relationships[i].m_RelationshipType == + arm::pipe::ITimelineDecoder::RelationshipType::DataLink); + CHECK(model.m_Relationships[i].m_Guid == relationshipGuid); + CHECK(model.m_Relationships[i].m_HeadGuid == headGuid); + CHECK(model.m_Relationships[i].m_TailGuid == tailGuid); + } + for ( unsigned long i = 0; i < 9; i += 3 ) + { + CHECK(model.m_Labels[i].m_Name == labelName); + CHECK(model.m_Labels[i+1].m_Name == labelName2); + CHECK(model.m_Labels[i+2].m_Name == labelName3); + } + }); } } diff --git a/src/backends/backendsCommon/test/MockBackend.hpp b/src/backends/backendsCommon/test/MockBackend.hpp index 6761ce5f08..3a5e79a224 100644 --- a/src/backends/backendsCommon/test/MockBackend.hpp +++ b/src/backends/backendsCommon/test/MockBackend.hpp @@ -113,7 +113,7 @@ private: IBackendInternal::IBackendProfilingPtr m_BackendProfiling; uint32_t m_CapturePeriod; std::vector m_ActiveCounters; - bool m_IsTimelineEnabled; + std::atomic m_IsTimelineEnabled; }; class MockBackendProfilingService diff --git a/tests/profiling/gatordmock/GatordMockService.hpp b/tests/profiling/gatordmock/GatordMockService.hpp index 1c45d4ea98..30c5444f4e 100644 --- a/tests/profiling/gatordmock/GatordMockService.hpp +++ b/tests/profiling/gatordmock/GatordMockService.hpp @@ -34,7 +34,7 @@ namespace gatordmock { /// A class that implements a Mock Gatord server. It will listen on a specified Unix domain socket (UDS) -/// namespace for client connections. It will then allow opertaions to manage coutners while receiving counter data. +/// namespace for client connections. It will then allow opertaions to manage counters while receiving counter data. class GatordMockService { public: diff --git a/tests/profiling/gatordmock/tests/GatordMockTests.cpp b/tests/profiling/gatordmock/tests/GatordMockTests.cpp index a7afdeaffe..388de368dd 100644 --- a/tests/profiling/gatordmock/tests/GatordMockTests.cpp +++ b/tests/profiling/gatordmock/tests/GatordMockTests.cpp @@ -186,53 +186,55 @@ void CheckTimelineDirectory(arm::pipe::TimelineDirectoryCaptureCommandHandler& c void CheckTimelinePackets(arm::pipe::TimelineDecoder& timelineDecoder) { unsigned int i = 0; // Use a postfix increment to avoid changing indexes each time the packet gets updated. - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::NAME_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::NAME_LABEL); + timelineDecoder.ApplyToModel([&](arm::pipe::TimelineDecoder::Model& m) { + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::NAME_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::NAME_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::TYPE_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::TYPE_LABEL); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::TYPE_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::TYPE_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::INDEX_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::INDEX_LABEL); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::INDEX_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::INDEX_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::BACKENDID_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::BACKENDID_LABEL); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::BACKENDID_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::BACKENDID_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::CHILD_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::CHILD_LABEL); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::CHILD_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::CHILD_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::EXECUTION_OF_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == - profiling::LabelsAndEventClasses::EXECUTION_OF_LABEL); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::EXECUTION_OF_GUID); + CHECK(m.m_Labels[i++].m_Name == + profiling::LabelsAndEventClasses::EXECUTION_OF_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::PROCESS_ID_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == - profiling::LabelsAndEventClasses::PROCESS_ID_LABEL); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::PROCESS_ID_GUID); + CHECK(m.m_Labels[i++].m_Name == + profiling::LabelsAndEventClasses::PROCESS_ID_LABEL); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::LAYER_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::LAYER); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::LAYER_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::LAYER); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::WORKLOAD_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::WORKLOAD); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::WORKLOAD_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::WORKLOAD); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::NETWORK_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::NETWORK); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::NETWORK_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::NETWORK); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::CONNECTION_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::CONNECTION); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::CONNECTION_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::CONNECTION); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::INFERENCE_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::INFERENCE); + CHECK(m.m_Labels[i].m_Guid == profiling::LabelsAndEventClasses::INFERENCE_GUID); + CHECK(m.m_Labels[i++].m_Name == profiling::LabelsAndEventClasses::INFERENCE); - CHECK(timelineDecoder.GetModel().m_Labels[i].m_Guid == - profiling::LabelsAndEventClasses::WORKLOAD_EXECUTION_GUID); - CHECK(timelineDecoder.GetModel().m_Labels[i++].m_Name == - profiling::LabelsAndEventClasses::WORKLOAD_EXECUTION); + CHECK(m.m_Labels[i].m_Guid == + profiling::LabelsAndEventClasses::WORKLOAD_EXECUTION_GUID); + CHECK(m.m_Labels[i++].m_Name == + profiling::LabelsAndEventClasses::WORKLOAD_EXECUTION); - CHECK(timelineDecoder.GetModel().m_EventClasses[0].m_Guid == - profiling::LabelsAndEventClasses::ARMNN_PROFILING_SOL_EVENT_CLASS); - CHECK(timelineDecoder.GetModel().m_EventClasses[1].m_Guid == - profiling::LabelsAndEventClasses::ARMNN_PROFILING_EOL_EVENT_CLASS); + CHECK(m.m_EventClasses[0].m_Guid == + profiling::LabelsAndEventClasses::ARMNN_PROFILING_SOL_EVENT_CLASS); + CHECK(m.m_EventClasses[1].m_Guid == + profiling::LabelsAndEventClasses::ARMNN_PROFILING_EOL_EVENT_CLASS); + }); } TEST_CASE("GatorDMockEndToEnd") @@ -294,7 +296,8 @@ TEST_CASE("GatorDMockEndToEnd") "MockGatord did not receive counter directory packet"); // Following that we will receive a collection of well known timeline labels and event classes - WaitFor([&](){return timelineDecoder.GetModel().m_EventClasses.size() >= 2;}, + WaitFor([&](){return timelineDecoder.ApplyToModel([&](arm::pipe::TimelineDecoder::Model& m){ + return m.m_EventClasses.size() >= 2;});}, "MockGatord did not receive well known timeline labels and event classes"); CheckTimelineDirectory(mockService.GetTimelineDirectoryCaptureCommandHandler()); @@ -446,18 +449,22 @@ TEST_CASE("GatorDMockTimeLineActivation") arm::pipe::TimelineDecoder& timelineDecoder = mockService.GetTimelineDecoder(); - WaitFor([&](){return timelineDecoder.GetModel().m_EventClasses.size() >= 2;}, + WaitFor([&](){return timelineDecoder.ApplyToModel([&](arm::pipe::TimelineDecoder::Model& m){ + return m.m_EventClasses.size() >= 2;});}, "MockGatord did not receive well known timeline labels"); - WaitFor([&](){return timelineDecoder.GetModel().m_Entities.size() >= 1;}, + WaitFor([&](){return timelineDecoder.ApplyToModel([&](arm::pipe::TimelineDecoder::Model& m){ + return m.m_Entities.size() >= 1;});}, "MockGatord did not receive mock backend test entity"); // Packets we expect from SendWellKnownLabelsAndEventClassesTest - CHECK(timelineDecoder.GetModel().m_Entities.size() == 1); - CHECK(timelineDecoder.GetModel().m_EventClasses.size() == 2); - CHECK(timelineDecoder.GetModel().m_Labels.size() == 15); - CHECK(timelineDecoder.GetModel().m_Relationships.size() == 0); - CHECK(timelineDecoder.GetModel().m_Events.size() == 0); + timelineDecoder.ApplyToModel([&](const arm::pipe::TimelineDecoder::Model& m){ + CHECK(m.m_Entities.size() == 1); + CHECK(m.m_EventClasses.size() == 2); + CHECK(m.m_Labels.size() == 15); + CHECK(m.m_Relationships.size() == 0); + CHECK(m.m_Events.size() == 0); + }); mockService.SendDeactivateTimelinePacket(); @@ -480,15 +487,18 @@ TEST_CASE("GatorDMockTimeLineActivation") // Once timeline packets have been reactivated the ActivateTimelineReportingCommandHandler will resend the // SendWellKnownLabelsAndEventClasses and then send the structure of any loaded networks - WaitFor([&](){return timelineDecoder.GetModel().m_Labels.size() >= 24;}, + WaitFor([&](){return timelineDecoder.ApplyToModel([&](arm::pipe::TimelineDecoder::Model& m){ + return m.m_Labels.size() >= 24;});}, "MockGatord did not receive well known timeline labels"); // Packets we expect from SendWellKnownLabelsAndEventClassesTest * 2 + network above (input, norm, backend, output) - CHECK(timelineDecoder.GetModel().m_Entities.size() == 6); - CHECK(timelineDecoder.GetModel().m_EventClasses.size() == 4); - CHECK(timelineDecoder.GetModel().m_Labels.size() == 34); - CHECK(timelineDecoder.GetModel().m_Relationships.size() == 15); - CHECK(timelineDecoder.GetModel().m_Events.size() == 0); + timelineDecoder.ApplyToModel([&](const arm::pipe::TimelineDecoder::Model& m){ + CHECK(m.m_Entities.size() == 6); + CHECK(m.m_EventClasses.size() == 4); + CHECK(m.m_Labels.size() == 34); + CHECK(m.m_Relationships.size() == 15); + CHECK(m.m_Events.size() == 0); + }); mockService.WaitForReceivingThread(); GetProfilingService(&runtime).Disconnect(); -- cgit v1.2.1