Skip to content

Commit

Permalink
Replace acquire+release thread annotation with excludes (flutter#5944)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
petrhosek authored Aug 6, 2018
1 parent 63ede2e commit c6baaaf
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 15 deletions.
6 changes: 3 additions & 3 deletions lib/ui/isolate_name_server/isolate_name_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
namespace blink {

Dart_Port IsolateNameServer::LookupIsolatePortByName(const std::string& name) {
std::unique_lock<std::mutex> lock(mutex_);
std::lock_guard<std::mutex> lock(mutex_);
return LookupIsolatePortByNameUnprotected(name);
}

Expand All @@ -22,7 +22,7 @@ Dart_Port IsolateNameServer::LookupIsolatePortByNameUnprotected(

bool IsolateNameServer::RegisterIsolatePortWithName(Dart_Port port,
const std::string& name) {
std::unique_lock<std::mutex> lock(mutex_);
std::lock_guard<std::mutex> lock(mutex_);
if (LookupIsolatePortByNameUnprotected(name) != ILLEGAL_PORT) {
// Name is already registered.
return false;
Expand All @@ -32,7 +32,7 @@ bool IsolateNameServer::RegisterIsolatePortWithName(Dart_Port port,
}

bool IsolateNameServer::RemoveIsolateNameMapping(const std::string& name) {
std::unique_lock<std::mutex> lock(mutex_);
std::lock_guard<std::mutex> lock(mutex_);
auto port_iterator = port_mapping_.find(name);
if (port_iterator == port_mapping_.end()) {
return false;
Expand Down
9 changes: 4 additions & 5 deletions lib/ui/isolate_name_server/isolate_name_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions lib/ui/plugins/callback_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ std::mutex DartCallbackCache::mutex_;
std::map<int64_t, DartCallbackRepresentation> DartCallbackCache::cache_;

Dart_Handle DartCallbackCache::GetCallback(int64_t handle) {
std::unique_lock<std::mutex> lock(mutex_);
std::lock_guard<std::mutex> lock(mutex_);
auto iterator = cache_.find(handle);
if (iterator != cache_.end()) {
DartCallbackRepresentation cb = iterator->second;
Expand All @@ -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<std::mutex> lock(mutex_);
std::lock_guard<std::mutex> lock(mutex_);
std::hash<std::string> hasher;
int64_t hash = hasher(name);
hash += hasher(class_name);
Expand All @@ -40,7 +40,7 @@ int64_t DartCallbackCache::GetCallbackHandle(const std::string& name,

std::unique_ptr<DartCallbackRepresentation>
DartCallbackCache::GetCallbackInformation(int64_t handle) {
std::unique_lock<std::mutex> lock(mutex_);
std::lock_guard<std::mutex> lock(mutex_);
auto iterator = cache_.find(handle);
if (iterator != cache_.end()) {
return std::make_unique<DartCallbackRepresentation>(iterator->second);
Expand Down
7 changes: 3 additions & 4 deletions lib/ui/plugins/callback_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -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<DartCallbackRepresentation> GetCallbackInformation(
int64_t handle) LOCK_UNLOCK(mutex_);
int64_t handle) FML_LOCKS_EXCLUDED(mutex_);

private:
static Dart_Handle LookupDartClosure(const std::string& name,
Expand Down

0 comments on commit c6baaaf

Please sign in to comment.