From 9abb2978f09643227664ab70c0677744b8f6e946 Mon Sep 17 00:00:00 2001
From: Nicolas Geoffray <ngeoffray@google.com>
Date: Fri, 4 Mar 2016 14:32:59 +0000
Subject: [PATCH] Fix very rare bug around JIT code cache collection.

The bug is the following:
1) JIT thread: We start a code cache collection.
2) JIT thread: We mark all code that is in the call stack of all
   threads.
3) Mutator thread: after marking its stack, resumes and does call
   that pushes JIT compiled code to the call stack.
4) Mutator thread: deoptimizes compiled code of ArtMethod Foo,
   and therefore updates the entry point of Foo through
   JitCodeCache::InvalidateCompiledCodeFor.
   (Note that updating the entrypoint could also be done through
   instrumentation).
5) JIT thread: Call JitCodeCache::RemoveUnusedAndUnmarkedCode.
   The method used to remove entries that were not entrypoints.
   It sees the compiled code for Foo but that is not an entrypoint
   anymore, so deletes it.
6) Mutator thread problem: it now has compiled code in its call
   stack that is deleted.

If it's only one mutator thread, we only hit a DCHECK when walking
the stack, as we are now seeing an invalid pc. The deoptimization
will longjmp to the caller of that invalid entry anyway.

However, if multiple mutator threads are involved, one thread
might invalidate the compiled code while the other is still
running it. And we end up deleting code that is in the call
stack of a thread, and we will crash.

The fix is to mark entrypoints before marking call stacks,
so that anything a thread might jump to is marked and kept.

bug:27424509
bug:23128949
bug:26846185

Change-Id: I07cd08cedd96b9900629f7535e95404f622104ea
---
 runtime/jit/jit_code_cache.cc | 32 ++++++++++++++++++++++----------
 runtime/jit/jit_code_cache.h  |  2 +-
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 8c69bc810..e5be2a477 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -639,21 +639,19 @@ void JitCodeCache::GarbageCollectCache(Thread* self) {
   Runtime::Current()->GetJit()->AddTimingLogger(logger);
 }
 
-void JitCodeCache::RemoveUnusedAndUnmarkedCode(Thread* self) {
+void JitCodeCache::RemoveUnmarkedCode(Thread* self) {
   MutexLock mu(self, lock_);
   ScopedCodeCacheWrite scc(code_map_.get());
-  // Iterate over all compiled code and remove entries that are not marked and not
-  // the entrypoint of their corresponding ArtMethod.
+  // Iterate over all compiled code and remove entries that are not marked.
   for (auto it = method_code_map_.begin(); it != method_code_map_.end();) {
     const void* code_ptr = it->first;
     ArtMethod* method = it->second;
     uintptr_t allocation = FromCodeToAllocation(code_ptr);
-    const OatQuickMethodHeader* method_header = OatQuickMethodHeader::FromCodePointer(code_ptr);
-    const void* entrypoint = method->GetEntryPointFromQuickCompiledCode();
-    if ((entrypoint == method_header->GetEntryPoint()) || GetLiveBitmap()->Test(allocation)) {
+    if (GetLiveBitmap()->Test(allocation)) {
       ++it;
     } else {
-      if (entrypoint == GetQuickToInterpreterBridge()) {
+      const OatQuickMethodHeader* method_header = OatQuickMethodHeader::FromCodePointer(code_ptr);
+      if (method_header->GetEntryPoint() == GetQuickToInterpreterBridge()) {
         method->ClearCounter();
       }
       FreeCode(code_ptr, method);
@@ -682,6 +680,19 @@ void JitCodeCache::DoCollection(Thread* self, bool collect_profiling_info) {
       }
     }
 
+    // Mark compiled code that are entrypoints of ArtMethods. Compiled code that is not
+    // an entry point is either:
+    // - an osr compiled code, that will be removed if not in a thread call stack.
+    // - discarded compiled code, that will be removed if not in a thread call stack.
+    for (const auto& it : method_code_map_) {
+      ArtMethod* method = it.second;
+      const void* code_ptr = it.first;
+      const OatQuickMethodHeader* method_header = OatQuickMethodHeader::FromCodePointer(code_ptr);
+      if (method_header->GetEntryPoint() == method->GetEntryPointFromQuickCompiledCode()) {
+        GetLiveBitmap()->AtomicTestAndSet(FromCodeToAllocation(code_ptr));
+      }
+    }
+
     // Empty osr method map, as osr compiled code will be deleted (except the ones
     // on thread stacks).
     osr_code_map_.clear();
@@ -690,9 +701,10 @@ void JitCodeCache::DoCollection(Thread* self, bool collect_profiling_info) {
   // Run a checkpoint on all threads to mark the JIT compiled code they are running.
   MarkCompiledCodeOnThreadStacks(self);
 
-  // Remove compiled code that is not the entrypoint of their method and not in the call
-  // stack.
-  RemoveUnusedAndUnmarkedCode(self);
+  // At this point, mutator threads are still running, and entrypoints of methods can
+  // change. We do know they cannot change to a code cache entry that is not marked,
+  // therefore we can safely remove those entries.
+  RemoveUnmarkedCode(self);
 
   if (collect_profiling_info) {
     MutexLock mu(self, lock_);
diff --git a/runtime/jit/jit_code_cache.h b/runtime/jit/jit_code_cache.h
index 2a41a70dc..0bd4f7dd1 100644
--- a/runtime/jit/jit_code_cache.h
+++ b/runtime/jit/jit_code_cache.h
@@ -238,7 +238,7 @@ class JitCodeCache {
       REQUIRES(!lock_)
       SHARED_REQUIRES(Locks::mutator_lock_);
 
-  void RemoveUnusedAndUnmarkedCode(Thread* self)
+  void RemoveUnmarkedCode(Thread* self)
       REQUIRES(!lock_)
       SHARED_REQUIRES(Locks::mutator_lock_);
 
-- 
2.11.0