From ae093d69f58c2b6257c3e5b82a32c135a1f33641 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Tue, 19 Jul 2016 17:06:23 +0100 Subject: [PATCH] JIT: Don't update the dex cache of another class loader. This only works for properly delegating class loaders. But Java allows non-delegating ones :( bug:29964720 test:612-jit-dex-cache (cherry picked from commit 491617a612c8a1c890e72d8ba780a151ddee8e11) (cherry picked from commit a2d7cbb44e570ec0a9064dc93f57441e6ab9e45a) Change-Id: I2523ce1fd4cd36dc83394d0f77ffc3f844e80c69 --- compiler/optimizing/inliner.cc | 44 +++++--------- .../hot_static_interface.cc | 57 ------------------ test/604-hot-static-interface/src/Main.java | 4 +- test/612-jit-dex-cache/expected.txt | 1 + test/612-jit-dex-cache/info.txt | 2 + test/612-jit-dex-cache/src-ex/B.java | 18 ++++++ .../src-ex/LoadedByAppClassLoader.java | 36 ++++++++++++ test/612-jit-dex-cache/src/A.java | 21 +++++++ test/612-jit-dex-cache/src/B.java | 18 ++++++ test/612-jit-dex-cache/src/Main.java | 67 ++++++++++++++++++++++ test/Android.libarttest.mk | 3 +- test/common/runtime_state.cc | 38 ++++++++++++ 12 files changed, 217 insertions(+), 92 deletions(-) delete mode 100644 test/604-hot-static-interface/hot_static_interface.cc create mode 100644 test/612-jit-dex-cache/expected.txt create mode 100644 test/612-jit-dex-cache/info.txt create mode 100644 test/612-jit-dex-cache/src-ex/B.java create mode 100644 test/612-jit-dex-cache/src-ex/LoadedByAppClassLoader.java create mode 100644 test/612-jit-dex-cache/src/A.java create mode 100644 test/612-jit-dex-cache/src/B.java create mode 100644 test/612-jit-dex-cache/src/Main.java diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc index 27b689615..932fad508 100644 --- a/compiler/optimizing/inliner.cc +++ b/compiler/optimizing/inliner.cc @@ -185,17 +185,6 @@ static ArtMethod* FindVirtualOrInterfaceTarget(HInvoke* invoke, ArtMethod* resol } } -static uint32_t FindMethodIndexIn(ArtMethod* method, - const DexFile& dex_file, - uint32_t referrer_index) - SHARED_REQUIRES(Locks::mutator_lock_) { - if (IsSameDexFile(*method->GetDexFile(), dex_file)) { - return method->GetDexMethodIndex(); - } else { - return method->FindDexMethodIndexInOtherDexFile(dex_file, referrer_index); - } -} - static uint32_t FindClassIndexIn(mirror::Class* cls, const DexFile& dex_file, Handle dex_cache) @@ -208,12 +197,8 @@ static uint32_t FindClassIndexIn(mirror::Class* cls, DCHECK(cls->IsProxyClass()) << PrettyClass(cls); // TODO: deal with proxy classes. } else if (IsSameDexFile(cls->GetDexFile(), dex_file)) { + DCHECK_EQ(cls->GetDexCache(), dex_cache.Get()); index = cls->GetDexTypeIndex(); - } else { - index = cls->FindTypeIndexInOtherDexFile(dex_file); - } - - if (index != DexFile::kDexNoIndex) { // Update the dex cache to ensure the class is in. The generated code will // consider it is. We make it safe by updating the dex cache, as other // dex files might also load the class, and there is no guarantee the dex @@ -221,6 +206,14 @@ static uint32_t FindClassIndexIn(mirror::Class* cls, if (dex_cache->GetResolvedType(index) == nullptr) { dex_cache->SetResolvedType(index, cls); } + } else { + index = cls->FindTypeIndexInOtherDexFile(dex_file); + // We cannot guarantee the entry in the dex cache will resolve to the same class, + // as there may be different class loaders. So only return the index if it's + // the right class in the dex cache already. + if (index != DexFile::kDexNoIndex && dex_cache->GetResolvedType(index) != cls) { + index = DexFile::kDexNoIndex; + } } return index; @@ -273,7 +266,7 @@ bool HInliner::TryInline(HInvoke* invoke_instruction) { return false; } MethodReference ref = invoke_instruction->AsInvokeStaticOrDirect()->GetTargetMethod(); - mirror::DexCache* const dex_cache = (&caller_dex_file == ref.dex_file) + mirror::DexCache* const dex_cache = IsSameDexFile(caller_dex_file, *ref.dex_file) ? caller_compilation_unit_.GetDexCache().Get() : class_linker->FindDexCache(soa.Self(), *ref.dex_file); resolved_method = dex_cache->GetResolvedMethod( @@ -763,8 +756,6 @@ bool HInliner::TryInlineAndReplace(HInvoke* invoke_instruction, ArtMethod* metho bool HInliner::TryBuildAndInline(HInvoke* invoke_instruction, ArtMethod* method, HInstruction** return_replacement) { - const DexFile& caller_dex_file = *caller_compilation_unit_.GetDexFile(); - if (method->IsProxyMethod()) { VLOG(compiler) << "Method " << PrettyMethod(method) << " is not inlined because of unimplemented inline support for proxy methods."; @@ -787,15 +778,6 @@ bool HInliner::TryBuildAndInline(HInvoke* invoke_instruction, return false; } - uint32_t method_index = FindMethodIndexIn( - method, caller_dex_file, invoke_instruction->GetDexMethodIndex()); - if (method_index == DexFile::kDexNoIndex) { - VLOG(compiler) << "Call to " - << PrettyMethod(method) - << " cannot be inlined because unaccessible to caller"; - return false; - } - bool same_dex_file = IsSameDexFile(*outer_compilation_unit_.GetDexFile(), *method->GetDexFile()); const DexFile::CodeItem* code_item = method->GetCodeItem(); @@ -832,7 +814,7 @@ bool HInliner::TryBuildAndInline(HInvoke* invoke_instruction, if (Runtime::Current()->UseJitCompilation() || !compiler_driver_->IsMethodVerifiedWithoutFailures( method->GetDexMethodIndex(), class_def_idx, *method->GetDexFile())) { - VLOG(compiler) << "Method " << PrettyMethod(method_index, caller_dex_file) + VLOG(compiler) << "Method " << PrettyMethod(method) << " couldn't be verified, so it cannot be inlined"; return false; } @@ -842,7 +824,7 @@ bool HInliner::TryBuildAndInline(HInvoke* invoke_instruction, invoke_instruction->AsInvokeStaticOrDirect()->IsStaticWithImplicitClinitCheck()) { // Case of a static method that cannot be inlined because it implicitly // requires an initialization check of its declaring class. - VLOG(compiler) << "Method " << PrettyMethod(method_index, caller_dex_file) + VLOG(compiler) << "Method " << PrettyMethod(method) << " is not inlined because it is static and requires a clinit" << " check that cannot be emitted due to Dex cache limitations"; return false; @@ -852,7 +834,7 @@ bool HInliner::TryBuildAndInline(HInvoke* invoke_instruction, return false; } - VLOG(compiler) << "Successfully inlined " << PrettyMethod(method_index, caller_dex_file); + VLOG(compiler) << "Successfully inlined " << PrettyMethod(method); MaybeRecordStat(kInlinedInvoke); return true; } diff --git a/test/604-hot-static-interface/hot_static_interface.cc b/test/604-hot-static-interface/hot_static_interface.cc deleted file mode 100644 index 9c51ca60a..000000000 --- a/test/604-hot-static-interface/hot_static_interface.cc +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Copyright (C) 2016 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "art_method.h" -#include "jit/jit.h" -#include "jit/jit_code_cache.h" -#include "oat_quick_method_header.h" -#include "scoped_thread_state_change.h" -#include "ScopedUtfChars.h" -#include "stack_map.h" - -namespace art { - -extern "C" JNIEXPORT void JNICALL Java_Main_waitUntilJitted(JNIEnv* env, - jclass, - jclass itf, - jstring method_name) { - jit::Jit* jit = Runtime::Current()->GetJit(); - if (jit == nullptr) { - return; - } - - ScopedObjectAccess soa(Thread::Current()); - - ScopedUtfChars chars(env, method_name); - CHECK(chars.c_str() != nullptr); - - mirror::Class* klass = soa.Decode(itf); - ArtMethod* method = klass->FindDeclaredDirectMethodByName(chars.c_str(), sizeof(void*)); - - jit::JitCodeCache* code_cache = jit->GetCodeCache(); - OatQuickMethodHeader* header = nullptr; - while (true) { - header = OatQuickMethodHeader::FromEntryPoint(method->GetEntryPointFromQuickCompiledCode()); - if (code_cache->ContainsPc(header->GetCode())) { - break; - } else { - // yield to scheduler to give time to the JIT compiler. - sched_yield(); - } - } -} - -} // namespace art diff --git a/test/604-hot-static-interface/src/Main.java b/test/604-hot-static-interface/src/Main.java index 559f15d38..04d7cd656 100644 --- a/test/604-hot-static-interface/src/Main.java +++ b/test/604-hot-static-interface/src/Main.java @@ -22,14 +22,14 @@ public class Main { Itf.foo(new Object()); } - waitUntilJitted(Itf.class, "foo"); + ensureJitCompiled(Itf.class, "foo"); if (!Itf.foo(new Object())) { throw new Error("Unexpected result"); } } - private static native void waitUntilJitted(Class itf, String method_name); + private static native void ensureJitCompiled(Class itf, String method_name); } interface Itf { diff --git a/test/612-jit-dex-cache/expected.txt b/test/612-jit-dex-cache/expected.txt new file mode 100644 index 000000000..6a5618ebc --- /dev/null +++ b/test/612-jit-dex-cache/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/612-jit-dex-cache/info.txt b/test/612-jit-dex-cache/info.txt new file mode 100644 index 000000000..e80f642f3 --- /dev/null +++ b/test/612-jit-dex-cache/info.txt @@ -0,0 +1,2 @@ +Regression test for the JIT compiler which used to +wrongly update the dex cache of a class loader. diff --git a/test/612-jit-dex-cache/src-ex/B.java b/test/612-jit-dex-cache/src-ex/B.java new file mode 100644 index 000000000..4da9a1da6 --- /dev/null +++ b/test/612-jit-dex-cache/src-ex/B.java @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class B { +} diff --git a/test/612-jit-dex-cache/src-ex/LoadedByAppClassLoader.java b/test/612-jit-dex-cache/src-ex/LoadedByAppClassLoader.java new file mode 100644 index 000000000..1d6158a59 --- /dev/null +++ b/test/612-jit-dex-cache/src-ex/LoadedByAppClassLoader.java @@ -0,0 +1,36 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class LoadedByAppClassLoader { + public static void letMeInlineYou(A a) { + a.foo(); + } + + public static ClassLoader areYouB() { + // Ensure letMeInlineYou is JITted and tries to do inlining of A.foo. + // The compiler used to wrongly update the dex cache of letMeInlineYou's + // class loader. + Main.ensureJitCompiled(LoadedByAppClassLoader.class, "letMeInlineYou"); + return OtherClass.getB().getClassLoader(); + } +} + +class OtherClass { + public static Class getB() { + // This used to return the B class of another class loader. + return B.class; + } +} diff --git a/test/612-jit-dex-cache/src/A.java b/test/612-jit-dex-cache/src/A.java new file mode 100644 index 000000000..415c71247 --- /dev/null +++ b/test/612-jit-dex-cache/src/A.java @@ -0,0 +1,21 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class A { + public int foo() { + return 42; + } +} diff --git a/test/612-jit-dex-cache/src/B.java b/test/612-jit-dex-cache/src/B.java new file mode 100644 index 000000000..46c878b57 --- /dev/null +++ b/test/612-jit-dex-cache/src/B.java @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class B extends A { +} diff --git a/test/612-jit-dex-cache/src/Main.java b/test/612-jit-dex-cache/src/Main.java new file mode 100644 index 000000000..0e4bd2245 --- /dev/null +++ b/test/612-jit-dex-cache/src/Main.java @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.lang.reflect.Method; +import java.lang.reflect.InvocationTargetException; + +import dalvik.system.PathClassLoader; + +// ClassLoader not delegating for non java. packages. +class DelegateLastPathClassLoader extends PathClassLoader { + + public DelegateLastPathClassLoader(String dexPath, ClassLoader parent) { + super(dexPath, parent); + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + if (!name.startsWith("java.")) { + try { + return findClass(name); + } catch (ClassNotFoundException ignore) { + // Ignore and fall through to parent class loader. + } + } + return super.loadClass(name, resolve); + } +} + +public class Main { + + private static Class classFromDifferentLoader() throws Exception { + final String DEX_FILE = System.getenv("DEX_LOCATION") + "/612-jit-dex-cache-ex.jar"; + ClassLoader loader = new DelegateLastPathClassLoader(DEX_FILE, Main.class.getClassLoader()); + return loader.loadClass("LoadedByAppClassLoader"); + } + + public static void main(String[] args) throws Exception { + System.loadLibrary(args[0]); + Class cls = classFromDifferentLoader(); + Method m = cls.getDeclaredMethod("letMeInlineYou", A.class); + B b = new B(); + // Invoke the method enough times to get an inline cache and get JITted. + for (int i = 0; i < 10000; ++i) { + m.invoke(null, b); + } + m = cls.getDeclaredMethod("areYouB", null); + ClassLoader loader = (ClassLoader) m.invoke(null); + if (loader != cls.getClassLoader()) { + throw new Error("Wrong class loader"); + } + } + + public static native void ensureJitCompiled(Class cls, String method_name); +} diff --git a/test/Android.libarttest.mk b/test/Android.libarttest.mk index 6d40cdf7c..5b1af277b 100644 --- a/test/Android.libarttest.mk +++ b/test/Android.libarttest.mk @@ -44,8 +44,7 @@ LIBARTTEST_COMMON_SRC_FILES := \ 566-polymorphic-inlining/polymorphic_inline.cc \ 570-checker-osr/osr.cc \ 595-profile-saving/profile-saving.cc \ - 597-deopt-new-string/deopt.cc \ - 604-hot-static-interface/hot_static_interface.cc + 597-deopt-new-string/deopt.cc ART_TARGET_LIBARTTEST_$(ART_PHONY_TEST_TARGET_SUFFIX) += $(ART_TARGET_TEST_OUT)/$(TARGET_ARCH)/libarttest.so ART_TARGET_LIBARTTEST_$(ART_PHONY_TEST_TARGET_SUFFIX) += $(ART_TARGET_TEST_OUT)/$(TARGET_ARCH)/libarttestd.so diff --git a/test/common/runtime_state.cc b/test/common/runtime_state.cc index fd41fd281..e70a95cbb 100644 --- a/test/common/runtime_state.cc +++ b/test/common/runtime_state.cc @@ -18,10 +18,14 @@ #include "base/logging.h" #include "dex_file-inl.h" +#include "jit/jit.h" +#include "jit/jit_code_cache.h" #include "mirror/class-inl.h" #include "nth_caller_visitor.h" +#include "oat_quick_method_header.h" #include "runtime.h" #include "scoped_thread_state_change.h" +#include "ScopedUtfChars.h" #include "stack.h" #include "thread-inl.h" @@ -116,4 +120,38 @@ extern "C" JNIEXPORT jboolean JNICALL Java_Main_compiledWithOptimizing(JNIEnv* e return JNI_TRUE; } +extern "C" JNIEXPORT void JNICALL Java_Main_ensureJitCompiled(JNIEnv* env, + jclass, + jclass cls, + jstring method_name) { + jit::Jit* jit = Runtime::Current()->GetJit(); + if (jit == nullptr) { + return; + } + + ScopedObjectAccess soa(Thread::Current()); + + ScopedUtfChars chars(env, method_name); + CHECK(chars.c_str() != nullptr); + + mirror::Class* klass = soa.Decode(cls); + ArtMethod* method = klass->FindDeclaredDirectMethodByName(chars.c_str(), sizeof(void*)); + + jit::JitCodeCache* code_cache = jit->GetCodeCache(); + OatQuickMethodHeader* header = nullptr; + // Make sure there is a profiling info, required by the compiler. + ProfilingInfo::Create(soa.Self(), method, /* retry_allocation */ true); + while (true) { + header = OatQuickMethodHeader::FromEntryPoint(method->GetEntryPointFromQuickCompiledCode()); + if (code_cache->ContainsPc(header->GetCode())) { + break; + } else { + // Sleep to yield to the compiler thread. + usleep(1000); + // Will either ensure it's compiled or do the compilation itself. + jit->CompileMethod(method, soa.Self(), /* osr */ false); + } + } +} + } // namespace art -- 2.11.0