From c6baaaf75a76919286c9e535db39a8f821fec9d9 Mon Sep 17 00:00:00 2001 From: Petr Hosek Date: Sun, 5 Aug 2018 18:25:43 -0700 Subject: [PATCH] Replace acquire+release thread annotation with excludes (#5944) The behavior of acquire+release annotation handling has changed in https://reviews.llvm.org/D49355 which breaks the build with the new Clang. However, as has been pointed out, the acquire+release isn't the right way to prevent double locking as the annotations negate each other; the correct way is to use excludes or negative requires. Using excludes annotations also requires using std::lock_guard instead of std::unique_lock because the latter doesn't have the thread annotations due to deferred locking which is not needed in Flutter and so std::lock_guard is a sufficient alternative. --- lib/ui/isolate_name_server/isolate_name_server.cc | 6 +++--- lib/ui/isolate_name_server/isolate_name_server.h | 9 ++++----- lib/ui/plugins/callback_cache.cc | 6 +++--- lib/ui/plugins/callback_cache.h | 7 +++---- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/ui/isolate_name_server/isolate_name_server.cc b/lib/ui/isolate_name_server/isolate_name_server.cc index 7a6a1fe58..9d78d145b 100644 --- a/lib/ui/isolate_name_server/isolate_name_server.cc +++ b/lib/ui/isolate_name_server/isolate_name_server.cc @@ -7,7 +7,7 @@ namespace blink { Dart_Port IsolateNameServer::LookupIsolatePortByName(const std::string& name) { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); return LookupIsolatePortByNameUnprotected(name); } @@ -22,7 +22,7 @@ Dart_Port IsolateNameServer::LookupIsolatePortByNameUnprotected( bool IsolateNameServer::RegisterIsolatePortWithName(Dart_Port port, const std::string& name) { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); if (LookupIsolatePortByNameUnprotected(name) != ILLEGAL_PORT) { // Name is already registered. return false; @@ -32,7 +32,7 @@ bool IsolateNameServer::RegisterIsolatePortWithName(Dart_Port port, } bool IsolateNameServer::RemoveIsolateNameMapping(const std::string& name) { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); auto port_iterator = port_mapping_.find(name); if (port_iterator == port_mapping_.end()) { return false; diff --git a/lib/ui/isolate_name_server/isolate_name_server.h b/lib/ui/isolate_name_server/isolate_name_server.h index 88c426873..906d94332 100644 --- a/lib/ui/isolate_name_server/isolate_name_server.h +++ b/lib/ui/isolate_name_server/isolate_name_server.h @@ -13,8 +13,6 @@ #include "flutter/fml/synchronization/thread_annotations.h" #include "third_party/dart/runtime/include/dart_api.h" -#define LOCK_UNLOCK(m) FML_ACQUIRE(m) FML_RELEASE(m) - namespace blink { class IsolateNameServer { @@ -24,16 +22,17 @@ class IsolateNameServer { // Looks up the Dart_Port associated with a given name. Returns ILLEGAL_PORT // if the name does not exist. Dart_Port LookupIsolatePortByName(const std::string& name) - LOCK_UNLOCK(mutex_); + FML_LOCKS_EXCLUDED(mutex_); // Registers a Dart_Port with a given name. Returns true if registration is // successful, false if the name entry already exists. bool RegisterIsolatePortWithName(Dart_Port port, const std::string& name) - LOCK_UNLOCK(mutex_); + FML_LOCKS_EXCLUDED(mutex_); // Removes a name to Dart_Port mapping given a name. Returns true if the // mapping was successfully removed, false if the mapping does not exist. - bool RemoveIsolateNameMapping(const std::string& name) LOCK_UNLOCK(mutex_); + bool RemoveIsolateNameMapping(const std::string& name) + FML_LOCKS_EXCLUDED(mutex_); private: Dart_Port LookupIsolatePortByNameUnprotected(const std::string& name) diff --git a/lib/ui/plugins/callback_cache.cc b/lib/ui/plugins/callback_cache.cc index 031f7d7cf..0e46493e2 100644 --- a/lib/ui/plugins/callback_cache.cc +++ b/lib/ui/plugins/callback_cache.cc @@ -14,7 +14,7 @@ std::mutex DartCallbackCache::mutex_; std::map DartCallbackCache::cache_; Dart_Handle DartCallbackCache::GetCallback(int64_t handle) { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); auto iterator = cache_.find(handle); if (iterator != cache_.end()) { DartCallbackRepresentation cb = iterator->second; @@ -26,7 +26,7 @@ Dart_Handle DartCallbackCache::GetCallback(int64_t handle) { int64_t DartCallbackCache::GetCallbackHandle(const std::string& name, const std::string& class_name, const std::string& library_path) { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); std::hash hasher; int64_t hash = hasher(name); hash += hasher(class_name); @@ -40,7 +40,7 @@ int64_t DartCallbackCache::GetCallbackHandle(const std::string& name, std::unique_ptr DartCallbackCache::GetCallbackInformation(int64_t handle) { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); auto iterator = cache_.find(handle); if (iterator != cache_.end()) { return std::make_unique(iterator->second); diff --git a/lib/ui/plugins/callback_cache.h b/lib/ui/plugins/callback_cache.h index 4922ab333..04e919a47 100644 --- a/lib/ui/plugins/callback_cache.h +++ b/lib/ui/plugins/callback_cache.h @@ -15,7 +15,6 @@ #include "third_party/dart/runtime/include/dart_api.h" #define DART_CALLBACK_INVALID_HANDLE -1 -#define LOCK_UNLOCK(m) FML_ACQUIRE(m) FML_RELEASE(m) namespace blink { @@ -30,12 +29,12 @@ class DartCallbackCache { static int64_t GetCallbackHandle(const std::string& name, const std::string& class_name, const std::string& library_path) - LOCK_UNLOCK(mutex_); + FML_LOCKS_EXCLUDED(mutex_); - static Dart_Handle GetCallback(int64_t handle) LOCK_UNLOCK(mutex_); + static Dart_Handle GetCallback(int64_t handle) FML_LOCKS_EXCLUDED(mutex_); static std::unique_ptr GetCallbackInformation( - int64_t handle) LOCK_UNLOCK(mutex_); + int64_t handle) FML_LOCKS_EXCLUDED(mutex_); private: static Dart_Handle LookupDartClosure(const std::string& name, -- GitLab