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 --- .../server/src/timelineDecoder/TimelineDecoder.cpp | 28 ++++---- .../src/timelineDecoder/tests/TimelineTests.cpp | 77 +++++++++++----------- 2 files changed, 55 insertions(+), 50 deletions(-) (limited to 'profiling/server/src') 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); + } + }); } } -- cgit v1.2.1