Tidy gAborting.

Reduce scope to Runtime::Abort and short-cut recursive case earlier. gAborting
remains global to avoid two fatal errors in thread and the verifier.

Change-Id: Ibc893f891ffee9a763c65cde9507d99083d47b3f

12 files changed, 52 insertions, 64 deletions

diff --git a/runtime/base/logging.cc b/runtime/base/logging.cc
index b781d60..bdc4cf6 100644
--- a/runtime/base/logging.cc
+++ b/runtime/base/logging.cc
@@ -35,8 +35,6 @@ namespace art {
 
 LogVerbosity gLogVerbosity;
 
-unsigned int gAborting = 0;
-
 static LogSeverity gMinimumLogSeverity = INFO;
 static std::unique_ptr<std::string> gCmdLine;
 static std::unique_ptr<std::string> gProgramInvocationName;
diff --git a/runtime/base/logging.h b/runtime/base/logging.h
index ae83e33..a9cc99b 100644
--- a/runtime/base/logging.h
+++ b/runtime/base/logging.h
@@ -55,11 +55,6 @@ struct LogVerbosity {
 // Global log verbosity setting, initialized by InitLogging.
 extern LogVerbosity gLogVerbosity;
 
-// 0 if not abort, non-zero if an abort is in progress. Used on fatal exit to prevents recursive
-// aborts. Global declaration allows us to disable some error checking to ensure fatal shutdown
-// makes forward progress.
-extern unsigned int gAborting;
-
 // Configure logging based on ANDROID_LOG_TAGS environment variable.
 // We need to parse a string that looks like
 //
diff --git a/runtime/base/mutex-inl.h b/runtime/base/mutex-inl.h
index cb69817..0206341 100644
--- a/runtime/base/mutex-inl.h
+++ b/runtime/base/mutex-inl.h
@@ -97,9 +97,7 @@ inline void BaseMutex::RegisterAsLocked(Thread* self) {
         }
       }
     }
-    if (gAborting == 0) {  // Avoid recursive aborts.
-      CHECK(!bad_mutexes_held);
-    }
+    CHECK(!bad_mutexes_held);
   }
   // Don't record monitors as they are outside the scope of analysis. They may be inspected off of
   // the monitor list.
@@ -114,7 +112,7 @@ inline void BaseMutex::RegisterAsUnlocked(Thread* self) {
     return;
   }
   if (level_ != kMonitorLock) {
-    if (kDebugLocking && gAborting == 0) {  // Avoid recursive aborts.
+    if (kDebugLocking) {
       CHECK(self->GetHeldMutex(level_) == this) << "Unlocking on unacquired mutex: " << name_;
     }
     self->SetHeldMutex(level_, NULL);
@@ -178,7 +176,7 @@ inline bool Mutex::IsExclusiveHeld(const Thread* self) const {
   bool result = (GetExclusiveOwnerTid() == SafeGetTid(self));
   if (kDebugLocking) {
     // Sanity debug check that if we think it is locked we have it in our held mutexes.
-    if (result && self != NULL && level_ != kMonitorLock && !gAborting) {
+    if (result && self != NULL && level_ != kMonitorLock) {
       CHECK_EQ(self->GetHeldMutex(level_), this);
     }
   }
diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc
index aa2aefc..4957988 100644
--- a/runtime/base/mutex.cc
+++ b/runtime/base/mutex.cc
@@ -209,9 +209,7 @@ void BaseMutex::CheckSafeToWait(Thread* self) {
         }
       }
     }
-    if (gAborting == 0) {  // Avoid recursive aborts.
-      CHECK(!bad_mutexes_held);
-    }
+    CHECK(!bad_mutexes_held);
   }
 }
 
diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h
index 9c93cc6..41b5f12 100644
--- a/runtime/base/mutex.h
+++ b/runtime/base/mutex.h
@@ -220,7 +220,7 @@ class LOCKABLE Mutex : public BaseMutex {
 
   // Assert that the Mutex is exclusively held by the current thread.
   void AssertExclusiveHeld(const Thread* self) {
-    if (kDebugLocking && (gAborting == 0)) {
+    if (kDebugLocking) {
       CHECK(IsExclusiveHeld(self)) << *this;
     }
   }
@@ -228,7 +228,7 @@ class LOCKABLE Mutex : public BaseMutex {
 
   // Assert that the Mutex is not held by the current thread.
   void AssertNotHeldExclusive(const Thread* self) {
-    if (kDebugLocking && (gAborting == 0)) {
+    if (kDebugLocking) {
       CHECK(!IsExclusiveHeld(self)) << *this;
     }
   }
@@ -318,7 +318,7 @@ class LOCKABLE ReaderWriterMutex : public BaseMutex {
 
   // Assert the current thread has exclusive access to the ReaderWriterMutex.
   void AssertExclusiveHeld(const Thread* self) {
-    if (kDebugLocking && (gAborting == 0)) {
+    if (kDebugLocking) {
       CHECK(IsExclusiveHeld(self)) << *this;
     }
   }
@@ -326,7 +326,7 @@ class LOCKABLE ReaderWriterMutex : public BaseMutex {
 
   // Assert the current thread doesn't have exclusive access to the ReaderWriterMutex.
   void AssertNotExclusiveHeld(const Thread* self) {
-    if (kDebugLocking && (gAborting == 0)) {
+    if (kDebugLocking) {
       CHECK(!IsExclusiveHeld(self)) << *this;
     }
   }
@@ -337,7 +337,7 @@ class LOCKABLE ReaderWriterMutex : public BaseMutex {
 
   // Assert the current thread has shared access to the ReaderWriterMutex.
   void AssertSharedHeld(const Thread* self) {
-    if (kDebugLocking && (gAborting == 0)) {
+    if (kDebugLocking) {
       // TODO: we can only assert this well when self != NULL.
       CHECK(IsSharedHeld(self) || self == NULL) << *this;
     }
@@ -347,7 +347,7 @@ class LOCKABLE ReaderWriterMutex : public BaseMutex {
   // Assert the current thread doesn't hold this ReaderWriterMutex either in shared or exclusive
   // mode.
   void AssertNotHeld(const Thread* self) {
-    if (kDebugLocking && (gAborting == 0)) {
+    if (kDebugLocking) {
       CHECK(!IsSharedHeld(self)) << *this;
     }
   }
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 078e7d2..e792031 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -126,6 +126,8 @@ namespace art {
 static constexpr bool kEnableJavaStackTraceHandler = false;
 Runtime* Runtime::instance_ = nullptr;
 
+volatile unsigned int gAborting = 0;
+
 Runtime::Runtime()
     : instruction_set_(kNone),
       compiler_callbacks_(nullptr),
@@ -236,13 +238,8 @@ Runtime::~Runtime() {
 
 struct AbortState {
   void Dump(std::ostream& os) const {
-    if (gAborting > 1) {
-      os << "Runtime aborting --- recursively, so no thread-specific detail!\n";
-      return;
-    }
-    gAborting++;
     os << "Runtime aborting...\n";
-    if (Runtime::Current() == NULL) {
+    if (Runtime::Current() == nullptr) {
       os << "(Runtime does not yet exist!)\n";
       return;
     }
@@ -300,13 +297,18 @@ struct AbortState {
 
 void Runtime::Abort() {
   gAborting++;  // set before taking any locks
+  if (gAborting > 1) {
+    LogMessage::LogLine(__FILE__, __LINE__, INTERNAL_FATAL,
+                        "Runtime aborting --- recursively, so no thread-specific detail!\n");
+    return;
+  }
 
   // Ensure that we don't have multiple threads trying to abort at once,
   // which would result in significantly worse diagnostics.
   MutexLock mu(Thread::Current(), *Locks::abort_lock_);
 
   // Get any pending output out of the way.
-  fflush(NULL);
+  fflush(nullptr);
 
   // Many people have difficulty distinguish aborts from crashes,
   // so be explicit.
@@ -314,7 +316,7 @@ void Runtime::Abort() {
   LOG(INTERNAL_FATAL) << Dumpable<AbortState>(state);
 
   // Call the abort hook if we have one.
-  if (Runtime::Current() != NULL && Runtime::Current()->abort_ != NULL) {
+  if (Runtime::Current() != nullptr && Runtime::Current()->abort_ != nullptr) {
     LOG(INTERNAL_FATAL) << "Calling abort hook...";
     Runtime::Current()->abort_();
     // notreached
@@ -342,7 +344,7 @@ void Runtime::PreZygoteFork() {
 }
 
 void Runtime::CallExitHook(jint status) {
-  if (exit_ != NULL) {
+  if (exit_ != nullptr) {
     ScopedThreadStateChange tsc(Thread::Current(), kNative);
     exit_(status);
     LOG(WARNING) << "Exit hook returned instead of exiting!";
@@ -357,14 +359,14 @@ void Runtime::SweepSystemWeaks(IsMarkedCallback* visitor, void* arg) {
 
 bool Runtime::Create(const RuntimeOptions& options, bool ignore_unrecognized) {
   // TODO: acquire a static mutex on Runtime to avoid racing.
-  if (Runtime::instance_ != NULL) {
+  if (Runtime::instance_ != nullptr) {
     return false;
   }
-  InitLogging(NULL);  // Calls Locks::Init() as a side effect.
+  InitLogging(nullptr);  // Calls Locks::Init() as a side effect.
   instance_ = new Runtime;
   if (!instance_->Init(options, ignore_unrecognized)) {
     delete instance_;
-    instance_ = NULL;
+    instance_ = nullptr;
     return false;
   }
   return true;
@@ -372,7 +374,7 @@ bool Runtime::Create(const RuntimeOptions& options, bool ignore_unrecognized) {
 
 static jobject CreateSystemClassLoader() {
   if (Runtime::Current()->UseCompileTimeClassPath()) {
-    return NULL;
+    return nullptr;
   }
 
   ScopedObjectAccess soa(Thread::Current());
@@ -385,7 +387,7 @@ static jobject CreateSystemClassLoader() {
 
   mirror::ArtMethod* getSystemClassLoader =
       class_loader_class->FindDirectMethod("getSystemClassLoader", "()Ljava/lang/ClassLoader;");
-  CHECK(getSystemClassLoader != NULL);
+  CHECK(getSystemClassLoader != nullptr);
 
   JValue result = InvokeWithJValues(soa, nullptr, soa.EncodeMethod(getSystemClassLoader), nullptr);
   JNIEnv* env = soa.Self()->GetJniEnv();
@@ -401,7 +403,7 @@ static jobject CreateSystemClassLoader() {
 
   mirror::ArtField* contextClassLoader =
       thread_class->FindDeclaredInstanceField("contextClassLoader", "Ljava/lang/ClassLoader;");
-  CHECK(contextClassLoader != NULL);
+  CHECK(contextClassLoader != nullptr);
 
   // We can't run in a transaction yet.
   contextClassLoader->SetObject<false>(soa.Self()->GetPeer(),
@@ -527,7 +529,7 @@ bool Runtime::InitZygote() {
 
   // Mark rootfs as being a slave so that changes from default
   // namespace only flow into our children.
-  if (mount("rootfs", "/", NULL, (MS_SLAVE | MS_REC), NULL) == -1) {
+  if (mount("rootfs", "/", nullptr, (MS_SLAVE | MS_REC), nullptr) == -1) {
     PLOG(WARNING) << "Failed to mount() rootfs as MS_SLAVE";
     return false;
   }
@@ -536,7 +538,7 @@ bool Runtime::InitZygote() {
   // bind mount storage into their respective private namespaces, which
   // are isolated from each other.
   const char* target_base = getenv("EMULATED_STORAGE_TARGET");
-  if (target_base != NULL) {
+  if (target_base != nullptr) {
     if (mount("tmpfs", target_base, "tmpfs", MS_NOSUID | MS_NODEV,
               "uid=0,gid=1028,mode=0751") == -1) {
       LOG(WARNING) << "Failed to mount tmpfs to " << target_base;
@@ -893,14 +895,14 @@ bool Runtime::Init(const RuntimeOptions& raw_options, bool ignore_unrecognized)
   self->ThrowNewException(ThrowLocation(), "Ljava/lang/OutOfMemoryError;",
                           "OutOfMemoryError thrown while trying to throw OutOfMemoryError; "
                           "no stack trace available");
-  pre_allocated_OutOfMemoryError_ = GcRoot<mirror::Throwable>(self->GetException(NULL));
+  pre_allocated_OutOfMemoryError_ = GcRoot<mirror::Throwable>(self->GetException(nullptr));
   self->ClearException();
 
   // Pre-allocate a NoClassDefFoundError for the common case of failing to find a system class
   // ahead of checking the application's class loader.
   self->ThrowNewException(ThrowLocation(), "Ljava/lang/NoClassDefFoundError;",
                           "Class not found using the boot class loader; no stack trace available");
-  pre_allocated_NoClassDefFoundError_ = GcRoot<mirror::Throwable>(self->GetException(NULL));
+  pre_allocated_NoClassDefFoundError_ = GcRoot<mirror::Throwable>(self->GetException(nullptr));
   self->ClearException();
 
   // Look for a native bridge.
@@ -976,26 +978,26 @@ void Runtime::InitThreadGroups(Thread* self) {
       env->NewGlobalRef(env->GetStaticObjectField(
           WellKnownClasses::java_lang_ThreadGroup,
           WellKnownClasses::java_lang_ThreadGroup_mainThreadGroup));
-  CHECK(main_thread_group_ != NULL || IsCompiler());
+  CHECK(main_thread_group_ != nullptr || IsCompiler());
   system_thread_group_ =
       env->NewGlobalRef(env->GetStaticObjectField(
           WellKnownClasses::java_lang_ThreadGroup,
           WellKnownClasses::java_lang_ThreadGroup_systemThreadGroup));
-  CHECK(system_thread_group_ != NULL || IsCompiler());
+  CHECK(system_thread_group_ != nullptr || IsCompiler());
 }
 
 jobject Runtime::GetMainThreadGroup() const {
-  CHECK(main_thread_group_ != NULL || IsCompiler());
+  CHECK(main_thread_group_ != nullptr || IsCompiler());
   return main_thread_group_;
 }
 
 jobject Runtime::GetSystemThreadGroup() const {
-  CHECK(system_thread_group_ != NULL || IsCompiler());
+  CHECK(system_thread_group_ != nullptr || IsCompiler());
   return system_thread_group_;
 }
 
 jobject Runtime::GetSystemClassLoader() const {
-  CHECK(system_class_loader_ != NULL || IsCompiler());
+  CHECK(system_class_loader_ != nullptr || IsCompiler());
   return system_class_loader_;
 }
 
@@ -1121,12 +1123,12 @@ void Runtime::BlockSignals() {
 
 bool Runtime::AttachCurrentThread(const char* thread_name, bool as_daemon, jobject thread_group,
                                   bool create_peer) {
-  return Thread::Attach(thread_name, as_daemon, thread_group, create_peer) != NULL;
+  return Thread::Attach(thread_name, as_daemon, thread_group, create_peer) != nullptr;
 }
 
 void Runtime::DetachCurrentThread() {
   Thread* self = Thread::Current();
-  if (self == NULL) {
+  if (self == nullptr) {
     LOG(FATAL) << "attempting to detach thread that is not attached";
   }
   if (self->HasManagedStack()) {
@@ -1351,7 +1353,7 @@ void Runtime::SetCalleeSaveMethod(mirror::ArtMethod* method, CalleeSaveType type
 }
 
 const std::vector<const DexFile*>& Runtime::GetCompileTimeClassPath(jobject class_loader) {
-  if (class_loader == NULL) {
+  if (class_loader == nullptr) {
     return GetClassLinker()->GetBootClassPath();
   }
   CHECK(UseCompileTimeClassPath());
diff --git a/runtime/runtime.h b/runtime/runtime.h
index 39fd910..e334764 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -71,6 +71,11 @@ class ThreadList;
 class Trace;
 class Transaction;
 
+// 0 if not abort, non-zero if an abort is in progress. Used on fatal exit to prevents recursive
+// aborts. Global declaration allows us to disable some error checking to ensure fatal shutdown
+// makes forward progress.
+extern volatile unsigned int gAborting;
+
 typedef std::vector<std::pair<std::string, const void*>> RuntimeOptions;
 
 // Not all combinations of flags are valid. You may not visit all roots as well as the new roots
@@ -175,9 +180,9 @@ class Runtime {
     return instance_;
   }
 
-  // Aborts semi-cleanly. Used in the implementation of LOG(FATAL), which most
-  // callers should prefer.
-  [[noreturn]] static void Abort() LOCKS_EXCLUDED(Locks::abort_lock_);
+  // Aborts semi-cleanly. Used in the implementation of LOG(FATAL), which most callers should
+  // prefer. Not [[noreturn]] due to returning early in the case of recursive aborts.
+  static void Abort() LOCKS_EXCLUDED(Locks::abort_lock_);
 
   // Returns the "main" ThreadGroup, used when attaching user threads.
   jobject GetMainThreadGroup() const;
diff --git a/runtime/runtime_android.cc b/runtime/runtime_android.cc
index 33600dd..33641ed 100644
--- a/runtime/runtime_android.cc
+++ b/runtime/runtime_android.cc
@@ -38,7 +38,6 @@ void HandleUnexpectedSignal(int signal_number, siginfo_t* info, void* raw_contex
     _exit(1);
   }
   handling_unexpected_signal = true;
-  gAborting++;  // set before taking any locks
   MutexLock mu(Thread::Current(), *Locks::unexpected_signal_lock_);
 
   Runtime* runtime = Runtime::Current();
diff --git a/runtime/runtime_linux.cc b/runtime/runtime_linux.cc
index 1de035c..9273091 100644
--- a/runtime/runtime_linux.cc
+++ b/runtime/runtime_linux.cc
@@ -284,7 +284,6 @@ void HandleUnexpectedSignal(int signal_number, siginfo_t* info, void* raw_contex
   }
   handlingUnexpectedSignal = true;
 
-  gAborting++;  // set before taking any locks
   MutexLock mu(Thread::Current(), *Locks::unexpected_signal_lock_);
 
   bool has_address = (signal_number == SIGILL || signal_number == SIGBUS ||
diff --git a/runtime/thread-inl.h b/runtime/thread-inl.h
index 7aed8b0..49b7be9 100644
--- a/runtime/thread-inl.h
+++ b/runtime/thread-inl.h
@@ -83,9 +83,7 @@ inline ThreadState Thread::SetState(ThreadState new_state) {
 
 inline void Thread::AssertThreadSuspensionIsAllowable(bool check_locks) const {
   if (kIsDebugBuild) {
-    if (gAborting == 0) {
-      CHECK_EQ(0u, tls32_.no_thread_suspension) << tlsPtr_.last_no_thread_suspension_cause;
-    }
+    CHECK_EQ(0u, tls32_.no_thread_suspension) << tlsPtr_.last_no_thread_suspension_cause;
     if (check_locks) {
       bool bad_mutexes_held = false;
       for (int i = kLockLevelCount - 1; i >= 0; --i) {
@@ -99,9 +97,7 @@ inline void Thread::AssertThreadSuspensionIsAllowable(bool check_locks) const {
           }
         }
       }
-      if (gAborting == 0) {
-        CHECK(!bad_mutexes_held);
-      }
+      CHECK(!bad_mutexes_held);
     }
   }
 }
diff --git a/runtime/thread.cc b/runtime/thread.cc
index f7c7106..a6e5b0c 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -742,7 +742,7 @@ void Thread::DumpState(std::ostream& os, const Thread* thread, pid_t tid) {
 
   // Don't do this if we are aborting since the GC may have all the threads suspended. This will
   // cause ScopedObjectAccessUnchecked to deadlock.
-  if (gAborting == 0 && self != nullptr && thread != nullptr && thread->tlsPtr_.opeer != nullptr) {
+  if (self != nullptr && thread != nullptr && thread->tlsPtr_.opeer != nullptr) {
     ScopedObjectAccessUnchecked soa(self);
     priority = soa.DecodeField(WellKnownClasses::java_lang_Thread_priority)
         ->GetInt(thread->tlsPtr_.opeer);
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index beafcda..71325a5 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -168,9 +168,7 @@ class DumpCheckpoint FINAL : public Closure {
     const uint32_t kWaitTimeoutMs = 10000;
     bool timed_out = barrier_.Increment(self, threads_running_checkpoint, kWaitTimeoutMs);
     if (timed_out) {
-      // Avoid a recursive abort.
-      LOG((kIsDebugBuild && (gAborting == 0)) ? FATAL : ERROR)
-          << "Unexpected time out during dump checkpoint.";
+      LOG(kIsDebugBuild ? FATAL : ERROR) << "Unexpected time out during dump checkpoint.";
     }
   }
 
@@ -243,7 +241,7 @@ size_t ThreadList::RunCheckpoint(Closure* checkpoint_function) {
   Locks::mutator_lock_->AssertNotExclusiveHeld(self);
   Locks::thread_list_lock_->AssertNotHeld(self);
   Locks::thread_suspend_count_lock_->AssertNotHeld(self);
-  if (kDebugLocking && gAborting == 0) {
+  if (kDebugLocking) {
     CHECK_NE(self->GetState(), kRunnable);
   }