From 8a39d781ce517846d298bc80ea4fdf2863b40b1f Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Wed, 8 Feb 2023 19:57:48 +0000 Subject: [PATCH] [VM/Timeline] Add test that makes TSAN check the track metadata map TEST=CI Change-Id: I9e4fa4bbc08e77a1365e84f2d048384d7f0b1350 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/281720 Reviewed-by: Ben Konyi Commit-Queue: Derek Xu --- runtime/vm/os_thread.cc | 4 ++- runtime/vm/timeline.cc | 22 ++++++++-------- runtime/vm/timeline.h | 4 ++- runtime/vm/timeline_test.cc | 50 +++++++++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 13 deletions(-) diff --git a/runtime/vm/os_thread.cc b/runtime/vm/os_thread.cc index e101536ee41..e04dc4a13a4 100644 --- a/runtime/vm/os_thread.cc +++ b/runtime/vm/os_thread.cc @@ -27,7 +27,9 @@ inline void UpdateTimelineTrackMetadata(const OSThread& thread) { RecorderLockScope rl; TimelineEventRecorder* recorder = Timeline::recorder(); if (recorder != nullptr && !rl.IsShuttingDown()) { - recorder->AddTrackMetadataBasedOnThread(thread); + recorder->AddTrackMetadataBasedOnThread( + OS::ProcessId(), OSThread::ThreadIdToIntPtr(thread.trace_id()), + thread.name()); } } #endif // defined(SUPPORT_TIMELINE) diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc index f70b6b444d1..43a17b0097e 100644 --- a/runtime/vm/timeline.cc +++ b/runtime/vm/timeline.cc @@ -1072,8 +1072,10 @@ TimelineEventRecorder::TimelineEventRecorder() // were initialized before this point. OSThreadIterator it; while (it.HasNext()) { - OSThread* thread = it.Next(); - AddTrackMetadataBasedOnThread(*thread); + OSThread& thread = *it.Next(); + AddTrackMetadataBasedOnThread(OS::ProcessId(), + OSThread::ThreadIdToIntPtr(thread.trace_id()), + thread.name()); } } @@ -1246,7 +1248,9 @@ TimelineEventBlock* TimelineEventRecorder::GetNewBlock() { } void TimelineEventRecorder::AddTrackMetadataBasedOnThread( - const OSThread& thread) { + const intptr_t process_id, + const intptr_t trace_id, + const char* thread_name) { if (FLAG_timeline_recorder != nullptr && // There is no way to retrieve track metadata when a callback or systrace // recorder is in use, so we don't need to update the map in these cases. @@ -1254,23 +1258,19 @@ void TimelineEventRecorder::AddTrackMetadataBasedOnThread( strcmp("systrace", FLAG_timeline_recorder) != 0) { MutexLocker ml(&track_uuid_to_track_metadata_lock_); - intptr_t pid = OS::ProcessId(); - intptr_t tid = OSThread::ThreadIdToIntPtr(thread.trace_id()); - const char* thread_name = thread.name(); - - void* key = reinterpret_cast(tid); - const intptr_t hash = Utils::WordHash(tid); + void* key = reinterpret_cast(trace_id); + const intptr_t hash = Utils::WordHash(trace_id); SimpleHashMap::Entry* entry = track_uuid_to_track_metadata_.Lookup(key, hash, true); if (entry->value == nullptr) { entry->value = new TimelineTrackMetadata( - pid, tid, + process_id, trace_id, Utils::CreateCStringUniquePtr( Utils::StrDup(thread_name == nullptr ? "" : thread_name))); } else { TimelineTrackMetadata* value = static_cast(entry->value); - ASSERT(pid == value->pid()); + ASSERT(process_id == value->pid()); value->set_track_name(Utils::CreateCStringUniquePtr( Utils::StrDup(thread_name == nullptr ? "" : thread_name))); } diff --git a/runtime/vm/timeline.h b/runtime/vm/timeline.h index 962f935c999..edbc12575e8 100644 --- a/runtime/vm/timeline.h +++ b/runtime/vm/timeline.h @@ -848,7 +848,9 @@ class TimelineEventRecorder : public MallocAllocated { void FinishBlock(TimelineEventBlock* block); // This function must be called at least once for each thread that corresponds // to a track in the trace. - void AddTrackMetadataBasedOnThread(const OSThread& thread); + void AddTrackMetadataBasedOnThread(const intptr_t process_id, + const intptr_t trace_id, + const char* thread_name); protected: #ifndef PRODUCT diff --git a/runtime/vm/timeline_test.cc b/runtime/vm/timeline_test.cc index ee3ff18f1e7..3ada2b2ed8d 100644 --- a/runtime/vm/timeline_test.cc +++ b/runtime/vm/timeline_test.cc @@ -3,6 +3,7 @@ // BSD-style license that can be found in the LICENSE file. #include +#include #include "platform/assert.h" @@ -373,6 +374,55 @@ TEST_CASE(TimelineRingRecorderJSONOrder) { EXPECT(alpha < beta); } +TEST_CASE(TimelineTrackMetadataRace) { + struct ReportMetadataArguments { + TimelineEventRingRecorder* recorder; + ThreadJoinId join_id = OSThread::kInvalidThreadJoinId; + }; + + TimelineEventRingRecorder recorder; + const intptr_t fake_process_id = 1; + const intptr_t fake_trace_id = 1; + + // Try concurrently reading from / writing to the metadata map. I don't think + // it's possible to assert anything about the outcome, because of scheduling + // uncertainty. This test is just used to ensure that TSAN checks the metadata + // map code. + JSONStream js; + TimelineEventFilter filter; + const ReportMetadataArguments report_metadata_1_arguments{&recorder}; + const ReportMetadataArguments report_metadata_2_arguments{&recorder}; + OSThread::Start( + "ReportMetadata1", + [](uword arguments_ptr) { + ReportMetadataArguments& arguments = + *reinterpret_cast(arguments_ptr); + arguments.recorder->AddTrackMetadataBasedOnThread( + fake_process_id, fake_trace_id, "Thread 1"); + arguments.join_id = + OSThread::GetCurrentThreadJoinId(OSThread::Current()); + }, + reinterpret_cast(&report_metadata_1_arguments)); + OSThread::Start( + "ReportMetadata2", + [](uword arguments_ptr) { + ReportMetadataArguments& arguments = + *reinterpret_cast(arguments_ptr); + arguments.recorder->AddTrackMetadataBasedOnThread( + fake_process_id, fake_trace_id, "Incorrect Name"); + arguments.join_id = + OSThread::GetCurrentThreadJoinId(OSThread::Current()); + }, + reinterpret_cast(&report_metadata_2_arguments)); + recorder.PrintJSON(&js, &filter); + while (report_metadata_1_arguments.join_id == OSThread::kInvalidThreadId || + report_metadata_2_arguments.join_id == OSThread::kInvalidThreadId) { + // Spin until the join IDs have been set. + } + OSThread::Join(report_metadata_1_arguments.join_id); + OSThread::Join(report_metadata_2_arguments.join_id); +} + #endif // !PRODUCT #if defined(SUPPORT_TIMELINE)