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
diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 8c69bc8..e5be2a4 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -639,21 +639,19 @@
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 @@
}
}
+ // 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 @@
// 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 2a41a70..0bd4f7d 100644
--- a/runtime/jit/jit_code_cache.h
+++ b/runtime/jit/jit_code_cache.h
@@ -238,7 +238,7 @@
REQUIRES(!lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
- void RemoveUnusedAndUnmarkedCode(Thread* self)
+ void RemoveUnmarkedCode(Thread* self)
REQUIRES(!lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
