Reland^2 "ART: Rewrite compiled code check in FaultHandler."

This reverts commit 0110e952e488bc41429f6f33f36e8884f41a26d8.

Reason for revert: Reland with a fix: Release the
mutator lock before requesting an empty checkpoint.

Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing --jit
Test: run-gtests.sh
Test: testrunner.py --target --optimizing
Bug: 38383823
Change-Id: Idf82d3b77465453b8e70b40e32af193f266b357b

1 files changed, 219 insertions, 191 deletions

diff --git a/runtime/fault_handler.cc b/runtime/fault_handler.cc
index c6940fa17b..cf7369a927 100644
--- a/runtime/fault_handler.cc
+++ b/runtime/fault_handler.cc
@@ -16,16 +16,17 @@
 
 #include "fault_handler.h"
 
+#include <atomic>
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/ucontext.h>
 
 #include "art_method-inl.h"
 #include "base/logging.h"  // For VLOG
+#include "base/membarrier.h"
 #include "base/safe_copy.h"
 #include "base/stl_util.h"
 #include "dex/dex_file_types.h"
-#include "gc/space/bump_pointer_space.h"
 #include "jit/jit.h"
 #include "jit/jit_code_cache.h"
 #include "mirror/class.h"
@@ -52,103 +53,16 @@ static bool art_fault_handler(int sig, siginfo_t* info, void* context) {
   return fault_manager.HandleFault(sig, info, context);
 }
 
-#if defined(__linux__)
-
-// Change to verify the safe implementations against the original ones.
-constexpr bool kVerifySafeImpls = false;
-
-// Provide implementations of ArtMethod::GetDeclaringClass and VerifyClassClass that use SafeCopy
-// to safely dereference pointers which are potentially garbage.
-// Only available on Linux due to availability of SafeCopy.
-
-static mirror::Class* SafeGetDeclaringClass(ArtMethod* method)
-    REQUIRES_SHARED(Locks::mutator_lock_) {
-  if (gUseUserfaultfd) {
-    // Avoid SafeCopy on userfaultfd updated memory ranges as kernel-space
-    // userfaults are not allowed, which can otherwise happen if compaction is
-    // simultaneously going on.
-    Runtime* runtime = Runtime::Current();
-    DCHECK_NE(runtime->GetHeap()->MarkCompactCollector(), nullptr);
-    GcVisitedArenaPool* pool = static_cast<GcVisitedArenaPool*>(runtime->GetLinearAllocArenaPool());
-    if (pool->Contains(method)) {
-      return method->GetDeclaringClassUnchecked<kWithoutReadBarrier>().Ptr();
-    }
-  }
-
-  char* method_declaring_class =
-      reinterpret_cast<char*>(method) + ArtMethod::DeclaringClassOffset().SizeValue();
-  // ArtMethod::declaring_class_ is a GcRoot<mirror::Class>.
-  // Read it out into as a CompressedReference directly for simplicity's sake.
-  mirror::CompressedReference<mirror::Class> cls;
-  ssize_t rc = SafeCopy(&cls, method_declaring_class, sizeof(cls));
-  CHECK_NE(-1, rc);
-
-  if (kVerifySafeImpls) {
-    ObjPtr<mirror::Class> actual_class = method->GetDeclaringClassUnchecked<kWithoutReadBarrier>();
-    CHECK_EQ(actual_class, cls.AsMirrorPtr());
-  }
-
-  if (rc != sizeof(cls)) {
-    return nullptr;
-  }
-
-  return cls.AsMirrorPtr();
-}
-
-static mirror::Class* SafeGetClass(mirror::Object* obj) REQUIRES_SHARED(Locks::mutator_lock_) {
-  if (gUseUserfaultfd) {
-    // Avoid SafeCopy on userfaultfd updated memory ranges as kernel-space
-    // userfaults are not allowed, which can otherwise happen if compaction is
-    // simultaneously going on.
-    gc::Heap* heap = Runtime::Current()->GetHeap();
-    DCHECK_NE(heap->MarkCompactCollector(), nullptr);
-    if (heap->GetBumpPointerSpace()->Contains(obj)) {
-      return obj->GetClass();
-    }
-  }
-
-  char* obj_cls = reinterpret_cast<char*>(obj) + mirror::Object::ClassOffset().SizeValue();
-  mirror::HeapReference<mirror::Class> cls;
-  ssize_t rc = SafeCopy(&cls, obj_cls, sizeof(cls));
-  CHECK_NE(-1, rc);
-
-  if (kVerifySafeImpls) {
-    mirror::Class* actual_class = obj->GetClass<kVerifyNone>();
-    CHECK_EQ(actual_class, cls.AsMirrorPtr());
-  }
-
-  if (rc != sizeof(cls)) {
-    return nullptr;
-  }
-
-  return cls.AsMirrorPtr();
-}
-
-static bool SafeVerifyClassClass(mirror::Class* cls) REQUIRES_SHARED(Locks::mutator_lock_) {
-  mirror::Class* c_c = SafeGetClass(cls);
-  bool result = c_c != nullptr && c_c == SafeGetClass(c_c);
-
-  if (kVerifySafeImpls) {
-    CHECK_EQ(VerifyClassClass(cls), result);
-  }
-
-  return result;
-}
-
-#else
-
-static mirror::Class* SafeGetDeclaringClass(ArtMethod* method_obj)
-    REQUIRES_SHARED(Locks::mutator_lock_) {
-  return method_obj->GetDeclaringClassUnchecked<kWithoutReadBarrier>().Ptr();
-}
+struct FaultManager::GeneratedCodeRange {
+  std::atomic<GeneratedCodeRange*> next;
+  const void* start;
+  size_t size;
+};
 
-static bool SafeVerifyClassClass(mirror::Class* cls) REQUIRES_SHARED(Locks::mutator_lock_) {
-  return VerifyClassClass(cls);
-}
-#endif
-
-
-FaultManager::FaultManager() : initialized_(false) {
+FaultManager::FaultManager()
+    : generated_code_ranges_lock_("FaultHandler generated code ranges lock",
+                                  LockLevel::kGenericBottomLock),
+      initialized_(false) {
   sigaction(SIGSEGV, nullptr, &oldaction_);
 }
 
@@ -172,6 +86,14 @@ void FaultManager::Init() {
   };
 
   AddSpecialSignalHandlerFn(SIGSEGV, &sa);
+
+  // Notify the kernel that we intend to use a specific `membarrier()` command.
+  int result = art::membarrier(MembarrierCommand::kRegisterPrivateExpedited);
+  if (result != 0) {
+    LOG(WARNING) << "FaultHandler: MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED failed: "
+                 << errno << " " << strerror(errno);
+  }
+
   initialized_ = true;
 }
 
@@ -189,6 +111,20 @@ void FaultManager::Shutdown() {
     // Free all handlers.
     STLDeleteElements(&generated_code_handlers_);
     STLDeleteElements(&other_handlers_);
+
+    // Delete remaining code ranges if any (such as nterp code or oat code from
+    // oat files that have not been unloaded, including boot image oat files).
+    GeneratedCodeRange* range;
+    {
+      MutexLock lock(Thread::Current(), generated_code_ranges_lock_);
+      range = generated_code_ranges_.load(std::memory_order_acquire);
+      generated_code_ranges_.store(nullptr, std::memory_order_release);
+    }
+    while (range != nullptr) {
+      GeneratedCodeRange* next_range = range->next.load(std::memory_order_relaxed);
+      delete range;
+      range = next_range;
+    }
   }
 }
 
@@ -243,7 +179,7 @@ bool FaultManager::HandleFault(int sig, siginfo_t* info, void* context) {
   raise(SIGSEGV);
 #endif
 
-  if (IsInGeneratedCode(info, context, true)) {
+  if (IsInGeneratedCode(info, context)) {
     VLOG(signals) << "in generated code, looking for handler";
     for (const auto& handler : generated_code_handlers_) {
       VLOG(signals) << "invoking Action on handler " << handler;
@@ -290,37 +226,110 @@ void FaultManager::RemoveHandler(FaultHandler* handler) {
   LOG(FATAL) << "Attempted to remove non existent handler " << handler;
 }
 
-static bool IsKnownPc(uintptr_t pc, ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_) {
-  // Check whether the pc is within nterp range.
-  if (OatQuickMethodHeader::IsNterpPc(pc)) {
-    return true;
-  }
-
-  // Check whether the pc is in the JIT code cache.
-  jit::Jit* jit = Runtime::Current()->GetJit();
-  if (jit != nullptr && jit->GetCodeCache()->ContainsPc(reinterpret_cast<const void*>(pc))) {
-    return true;
-  }
+void FaultManager::AddGeneratedCodeRange(const void* start, size_t size) {
+  GeneratedCodeRange* new_range = new GeneratedCodeRange{nullptr, start, size};
+  {
+    MutexLock lock(Thread::Current(), generated_code_ranges_lock_);
+    GeneratedCodeRange* old_head = generated_code_ranges_.load(std::memory_order_relaxed);
+    new_range->next.store(old_head, std::memory_order_relaxed);
+    generated_code_ranges_.store(new_range, std::memory_order_release);
+  }
+
+  // The above release operation on `generated_code_ranges_` with an acquire operation
+  // on the same atomic object in `IsInGeneratedCode()` ensures the correct memory
+  // visibility for the contents of `*new_range` for any thread that loads the value
+  // written above (or a value written by a release sequence headed by that write).
+  //
+  // However, we also need to ensure that any thread that encounters a segmentation
+  // fault in the provided range shall actually see the written value. For JIT code
+  // cache and nterp, the registration happens while the process is single-threaded
+  // but the synchronization is more complicated for code in oat files.
+  //
+  // Threads that load classes register dex files under the `Locks::dex_lock_` and
+  // the first one to register a dex file with a given oat file shall add the oat
+  // code range; the memory visibility for these threads is guaranteed by the lock.
+  // However a thread that did not try to load a class with oat code can execute the
+  // code if a direct or indirect reference to such class escapes from one of the
+  // threads that loaded it. Use `membarrier()` for memory visibility in this case.
+  art::membarrier(MembarrierCommand::kPrivateExpedited);
+}
 
-  if (method->IsObsolete()) {
-    // Obsolete methods never happen on AOT code.
-    return false;
+void FaultManager::RemoveGeneratedCodeRange(const void* start, size_t size) {
+  Thread* self = Thread::Current();
+  GeneratedCodeRange* range = nullptr;
+  {
+    MutexLock lock(self, generated_code_ranges_lock_);
+    std::atomic<GeneratedCodeRange*>* before = &generated_code_ranges_;
+    range = before->load(std::memory_order_relaxed);
+    while (range != nullptr && range->start != start) {
+      before = &range->next;
+      range = before->load(std::memory_order_relaxed);
+    }
+    if (range != nullptr) {
+      GeneratedCodeRange* next = range->next.load(std::memory_order_relaxed);
+      if (before == &generated_code_ranges_) {
+        // Relaxed store directly to `generated_code_ranges_` would not satisfy
+        // conditions for a release sequence, so we need to use store-release.
+        before->store(next, std::memory_order_release);
+      } else {
+        // In the middle of the list, we can use a relaxed store as we're not
+        // publishing any newly written memory to potential reader threads.
+        // Whether they see the removed node or not is unimportant as we should
+        // not execute that code anymore. We're keeping the `next` link of the
+        // removed node, so that concurrent walk can use it to reach remaining
+        // retained nodes, if any.
+        before->store(next, std::memory_order_relaxed);
+      }
+    }
   }
-
-  // Note: at this point, we trust it's truly an ArtMethod we found at the bottom of the stack,
-  // and we can find its oat file through it.
-  const OatDexFile* oat_dex_file = method->GetDeclaringClass()->GetDexFile().GetOatDexFile();
-  if (oat_dex_file != nullptr &&
-      oat_dex_file->GetOatFile()->Contains(reinterpret_cast<const void*>(pc))) {
-    return true;
+  CHECK(range != nullptr);
+  DCHECK_EQ(range->start, start);
+  CHECK_EQ(range->size, size);
+
+  Runtime* runtime = Runtime::Current();
+  CHECK(runtime != nullptr);
+  if (runtime->IsStarted() && runtime->GetThreadList() != nullptr) {
+    // Run a checkpoint before deleting the range to ensure that no thread holds a
+    // pointer to the removed range while walking the list in `IsInGeneratedCode()`.
+    // That walk is guarded by checking that the thread is `Runnable`, so any walk
+    // started before the removal shall be done when running the checkpoint and the
+    // checkpoint also ensures the correct memory visibility of `next` links,
+    // so the thread shall not see the pointer during future walks.
+
+    // This function is currently called in different mutex and thread states.
+    // Semi-space GC performs the cleanup during its `MarkingPhase()` while holding
+    // the mutator exclusively, so we do not need a checkpoint. All other GCs perform
+    // the cleanup in their `ReclaimPhase()` while holding the mutator lock as shared
+    // and it's safe to release and re-acquire the mutator lock. Despite holding the
+    // mutator lock as shared, the thread is not always marked as `Runnable`.
+    // TODO: Clean up state transitions in different GC implementations. b/259440389
+    if (Locks::mutator_lock_->IsExclusiveHeld(self)) {
+      // We do not need a checkpoint because no other thread is Runnable.
+    } else {
+      DCHECK(Locks::mutator_lock_->IsSharedHeld(self));
+      // Use explicit state transitions or unlock/lock.
+      bool runnable = (self->GetState() == ThreadState::kRunnable);
+      if (runnable) {
+        self->TransitionFromRunnableToSuspended(ThreadState::kNative);
+      } else {
+        Locks::mutator_lock_->SharedUnlock(self);
+      }
+      DCHECK(!Locks::mutator_lock_->IsSharedHeld(self));
+      runtime->GetThreadList()->RunEmptyCheckpoint();
+      if (runnable) {
+        self->TransitionFromSuspendedToRunnable();
+      } else {
+        Locks::mutator_lock_->SharedLock(self);
+      }
+    }
   }
-
-  return false;
+  delete range;
 }
 
-// This function is called within the signal handler.  It checks that
-// the mutator_lock is held (shared).  No annotalysis is done.
-bool FaultManager::IsInGeneratedCode(siginfo_t* siginfo, void* context, bool check_dex_pc) {
+// This function is called within the signal handler. It checks that the thread
+// is `Runnable`, the `mutator_lock_` is held (shared) and the fault PC is in one
+// of the registered generated code ranges. No annotalysis is done.
+bool FaultManager::IsInGeneratedCode(siginfo_t* siginfo, void* context) {
   // We can only be running Java code in the current thread if it
   // is in Runnable state.
   VLOG(signals) << "Checking for generated code";
@@ -343,86 +352,109 @@ bool FaultManager::IsInGeneratedCode(siginfo_t* siginfo, void* context, bool che
     return false;
   }
 
-  ArtMethod* method_obj = nullptr;
-  uintptr_t return_pc = 0;
-  uintptr_t sp = 0;
-  bool is_stack_overflow = false;
-
-  // Get the architecture specific method address and return address.  These
-  // are in architecture specific files in arch/<arch>/fault_handler_<arch>.
-  GetMethodAndReturnPcAndSp(siginfo, context, &method_obj, &return_pc, &sp, &is_stack_overflow);
-
-  // If we don't have a potential method, we're outta here.
-  VLOG(signals) << "potential method: " << method_obj;
-  // TODO: Check linear alloc and image.
-  DCHECK_ALIGNED(ArtMethod::Size(kRuntimePointerSize), sizeof(void*))
-      << "ArtMethod is not pointer aligned";
-  if (method_obj == nullptr || !IsAligned<sizeof(void*)>(method_obj)) {
-    VLOG(signals) << "no method";
+  uintptr_t fault_pc = GetFaultPc(siginfo, context);
+  if (fault_pc == 0u) {
+    VLOG(signals) << "no fault PC";
     return false;
   }
 
-  // Verify that the potential method is indeed a method.
-  // TODO: check the GC maps to make sure it's an object.
-  // Check that the class pointer inside the object is not null and is aligned.
-  // No read barrier because method_obj may not be a real object.
-  mirror::Class* cls = SafeGetDeclaringClass(method_obj);
-  if (cls == nullptr) {
-    VLOG(signals) << "not a class";
+  // Walk over the list of registered code ranges.
+  GeneratedCodeRange* range = generated_code_ranges_.load(std::memory_order_acquire);
+  while (range != nullptr) {
+    if (fault_pc - reinterpret_cast<uintptr_t>(range->start) < range->size) {
+      return true;
+    }
+    // We may or may not see ranges that were concurrently removed, depending
+    // on when the relaxed writes of the `next` links become visible. However,
+    // even if we're currently at a node that is being removed, we shall visit
+    // all remaining ranges that are not being removed as the removed nodes
+    // retain the `next` link at the time of removal (which may lead to other
+    // removed nodes before reaching remaining retained nodes, if any). Correct
+    // memory visibility of `start` and `size` fields of the visited ranges is
+    // ensured by the release and acquire operations on `generated_code_ranges_`.
+    range = range->next.load(std::memory_order_relaxed);
+  }
+  return false;
+}
+
+FaultHandler::FaultHandler(FaultManager* manager) : manager_(manager) {
+}
+
+//
+// Null pointer fault handler
+//
+NullPointerHandler::NullPointerHandler(FaultManager* manager) : FaultHandler(manager) {
+  manager_->AddHandler(this, true);
+}
+
+bool NullPointerHandler::IsValidMethod(ArtMethod* method) {
+  // At this point we know that the thread is `Runnable` and the PC is in one of
+  // the registered code ranges. The `method` was read from the top of the stack
+  // and should really point to an actual `ArtMethod`, unless we're crashing during
+  // prologue or epilogue, or somehow managed to jump to the compiled code by some
+  // unexpected path, other than method invoke or exception delivery. We do a few
+  // quick checks without guarding from another fault.
+  VLOG(signals) << "potential method: " << method;
+
+  static_assert(IsAligned<sizeof(void*)>(ArtMethod::Size(kRuntimePointerSize)));
+  if (method == nullptr || !IsAligned<sizeof(void*)>(method)) {
+    VLOG(signals) << ((method == nullptr) ? "null method" : "unaligned method");
     return false;
   }
 
-  if (!IsAligned<kObjectAlignment>(cls)) {
-    VLOG(signals) << "not aligned";
+  // Check that the presumed method actually points to a class. Read barriers
+  // are not needed (and would be undesirable in a signal handler) when reading
+  // a chain of constant references to get to a non-movable `Class.class` object.
+
+  // Note: Allowing nested faults. Checking that the method is in one of the
+  // `LinearAlloc` spaces, or that objects we look at are in the `Heap` would be
+  // slow and require locking a mutex, which is undesirable in a signal handler.
+  // (Though we could register valid ranges similarly to the generated code ranges.)
+
+  mirror::Object* klass =
+      method->GetDeclaringClassAddressWithoutBarrier()->AsMirrorPtr();
+  if (klass == nullptr || !IsAligned<kObjectAlignment>(klass)) {
+    VLOG(signals) << ((klass == nullptr) ? "null class" : "unaligned class");
     return false;
   }
 
-  if (!SafeVerifyClassClass(cls)) {
-    VLOG(signals) << "not a class class";
+  mirror::Class* class_class = klass->GetClass<kVerifyNone, kWithoutReadBarrier>();
+  if (class_class == nullptr || !IsAligned<kObjectAlignment>(class_class)) {
+    VLOG(signals) << ((klass == nullptr) ? "null class_class" : "unaligned class_class");
     return false;
   }
 
-  if (!IsKnownPc(return_pc, method_obj)) {
-    VLOG(signals) << "PC not in Java code";
+  if (class_class != class_class->GetClass<kVerifyNone, kWithoutReadBarrier>()) {
+    VLOG(signals) << "invalid class_class";
     return false;
   }
 
-  const OatQuickMethodHeader* method_header = method_obj->GetOatQuickMethodHeader(return_pc);
+  return true;
+}
 
+bool NullPointerHandler::IsValidReturnPc(ArtMethod** sp, uintptr_t return_pc) {
+  // Check if we can associate a dex PC with the return PC, whether from Nterp,
+  // or with an existing stack map entry for a compiled method.
+  // Note: Allowing nested faults if `IsValidMethod()` returned a false positive.
+  // Note: The `ArtMethod::GetOatQuickMethodHeader()` can acquire locks (at least
+  // `Locks::jit_lock_`) and if the thread already held such a lock, the signal
+  // handler would deadlock. However, if a thread is holding one of the locks
+  // below the mutator lock, the PC should be somewhere in ART code and should
+  // not match any registered generated code range, so such as a deadlock is
+  // unlikely. If it happens anyway, the worst case is that an internal ART crash
+  // would be reported as ANR.
+  ArtMethod* method = *sp;
+  const OatQuickMethodHeader* method_header = method->GetOatQuickMethodHeader(return_pc);
   if (method_header == nullptr) {
-    VLOG(signals) << "no compiled code";
+    VLOG(signals) << "No method header.";
     return false;
   }
-
-  // We can be certain that this is a method now.  Check if we have a GC map
-  // at the return PC address.
-  if (true || kIsDebugBuild) {
-    VLOG(signals) << "looking for dex pc for return pc " << std::hex << return_pc;
-    uint32_t sought_offset = return_pc -
-        reinterpret_cast<uintptr_t>(method_header->GetEntryPoint());
-    VLOG(signals) << "pc offset: " << std::hex << sought_offset;
-  }
-  uint32_t dexpc = dex::kDexNoIndex;
-  if (is_stack_overflow) {
-    // If it's an implicit stack overflow check, the frame is not setup, so we
-    // just infer the dex PC as zero.
-    dexpc = 0;
-  } else {
-    CHECK_EQ(*reinterpret_cast<ArtMethod**>(sp), method_obj);
-    dexpc = method_header->ToDexPc(reinterpret_cast<ArtMethod**>(sp), return_pc, false);
-  }
+  VLOG(signals) << "looking for dex pc for return pc 0x" << std::hex << return_pc
+                << " pc offset: 0x" << std::hex
+                << (return_pc - reinterpret_cast<uintptr_t>(method_header->GetEntryPoint()));
+  uint32_t dexpc = method_header->ToDexPc(reinterpret_cast<ArtMethod**>(sp), return_pc, false);
   VLOG(signals) << "dexpc: " << dexpc;
-  return !check_dex_pc || dexpc != dex::kDexNoIndex;
-}
-
-FaultHandler::FaultHandler(FaultManager* manager) : manager_(manager) {
-}
-
-//
-// Null pointer fault handler
-//
-NullPointerHandler::NullPointerHandler(FaultManager* manager) : FaultHandler(manager) {
-  manager_->AddHandler(this, true);
+  return dexpc != dex::kDexNoIndex;
 }
 
 //
@@ -448,17 +480,13 @@ JavaStackTraceHandler::JavaStackTraceHandler(FaultManager* manager) : FaultHandl
 
 bool JavaStackTraceHandler::Action(int sig ATTRIBUTE_UNUSED, siginfo_t* siginfo, void* context) {
   // Make sure that we are in the generated code, but we may not have a dex pc.
-  bool in_generated_code = manager_->IsInGeneratedCode(siginfo, context, false);
+  bool in_generated_code = manager_->IsInGeneratedCode(siginfo, context);
   if (in_generated_code) {
     LOG(ERROR) << "Dumping java stack trace for crash in generated code";
-    ArtMethod* method = nullptr;
-    uintptr_t return_pc = 0;
-    uintptr_t sp = 0;
-    bool is_stack_overflow = false;
     Thread* self = Thread::Current();
 
-    manager_->GetMethodAndReturnPcAndSp(
-        siginfo, context, &method, &return_pc, &sp, &is_stack_overflow);
+    uintptr_t sp = FaultManager::GetFaultSp(context);
+    CHECK_NE(sp, 0u);  // Otherwise we should not have reached this handler.
     // Inside of generated code, sp[0] is the method, so sp is the frame.
     self->SetTopOfStack(reinterpret_cast<ArtMethod**>(sp));
     self->DumpJavaStack(LOG_STREAM(ERROR));