Make concurrently accessed globals atomic
Eliminate data races on gHaveTLS, gShutdown, and
gDisableBackgroundScheduling by declaring them as atomic. Use
explicit load() and store() operations to access them.
Note that gHaveTLS requires memory ordering guarantees, and thus,
even with a cooperative compiler, the old code would never have been
compiled to correct ARM object code.
Document a couple of weirdnesses remaining in the code.
Bug: 36697681
Test: Boot AOSP, TreeHugger
Change-Id: If44a10e31c1e091f06179154e07ced3da0a37b68
diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp
index 4b70e2e..7eec245 100644
--- a/libs/binder/IPCThreadState.cpp
+++ b/libs/binder/IPCThreadState.cpp
@@ -33,6 +33,7 @@
#include <private/binder/binder_module.h>
#include <private/binder/Static.h>
+#include <atomic>
#include <errno.h>
#include <inttypes.h>
#include <pthread.h>
@@ -276,14 +277,14 @@
}
static pthread_mutex_t gTLSMutex = PTHREAD_MUTEX_INITIALIZER;
-static bool gHaveTLS = false;
+static std::atomic<bool> gHaveTLS(false);
static pthread_key_t gTLS = 0;
-static bool gShutdown = false;
-static bool gDisableBackgroundScheduling = false;
+static std::atomic<bool> gShutdown = false;
+static std::atomic<bool> gDisableBackgroundScheduling = false;
IPCThreadState* IPCThreadState::self()
{
- if (gHaveTLS) {
+ if (gHaveTLS.load(std::memory_order_acquire)) {
restart:
const pthread_key_t k = gTLS;
IPCThreadState* st = (IPCThreadState*)pthread_getspecific(k);
@@ -291,13 +292,14 @@
return new IPCThreadState;
}
- if (gShutdown) {
+ // Racey, heuristic test for simultaneous shutdown.
+ if (gShutdown.load(std::memory_order_relaxed)) {
ALOGW("Calling IPCThreadState::self() during shutdown is dangerous, expect a crash.\n");
return nullptr;
}
pthread_mutex_lock(&gTLSMutex);
- if (!gHaveTLS) {
+ if (!gHaveTLS.load(std::memory_order_relaxed)) {
int key_create_value = pthread_key_create(&gTLS, threadDestructor);
if (key_create_value != 0) {
pthread_mutex_unlock(&gTLSMutex);
@@ -305,7 +307,7 @@
strerror(key_create_value));
return nullptr;
}
- gHaveTLS = true;
+ gHaveTLS.store(true, std::memory_order_release);
}
pthread_mutex_unlock(&gTLSMutex);
goto restart;
@@ -313,7 +315,7 @@
IPCThreadState* IPCThreadState::selfOrNull()
{
- if (gHaveTLS) {
+ if (gHaveTLS.load(std::memory_order_acquire)) {
const pthread_key_t k = gTLS;
IPCThreadState* st = (IPCThreadState*)pthread_getspecific(k);
return st;
@@ -323,9 +325,9 @@
void IPCThreadState::shutdown()
{
- gShutdown = true;
+ gShutdown.store(true, std::memory_order_relaxed);
- if (gHaveTLS) {
+ if (gHaveTLS.load(std::memory_order_acquire)) {
// XXX Need to wait for all thread pool threads to exit!
IPCThreadState* st = (IPCThreadState*)pthread_getspecific(gTLS);
if (st) {
@@ -333,18 +335,18 @@
pthread_setspecific(gTLS, nullptr);
}
pthread_key_delete(gTLS);
- gHaveTLS = false;
+ gHaveTLS.store(false, std::memory_order_release);
}
}
void IPCThreadState::disableBackgroundScheduling(bool disable)
{
- gDisableBackgroundScheduling = disable;
+ gDisableBackgroundScheduling.store(disable, std::memory_order_relaxed);
}
bool IPCThreadState::backgroundSchedulingDisabled()
{
- return gDisableBackgroundScheduling;
+ return gDisableBackgroundScheduling.load(std::memory_order_relaxed);
}
sp<ProcessState> IPCThreadState::process()