From 504a69081f63818ca332ddaf54e8198448554538 Mon Sep 17 00:00:00 2001 From: Mingyao Yang Date: Thu, 28 Apr 2016 16:23:01 -0700 Subject: [PATCH] Fix assert failure in interpreter after deoptimization. There is code that does new String(chararr, 0, chararr_count); which under debuggable turns into a call into pNewEmptyString() plus a call into pNewStringFromChars_CII(). Even though we currently don't patch return pc in a runtime method, calling into pNewEmptyString() is special since it's hacked such that it's acting as if the caller calls into the java method StringFactory.newEmptyString() directly. So deoptimization can now happen at the NewEmptyString site and the assert is triggered since it's a new instance instead of an invoke instruction. The fix relaxes the assert to allow the special case. Bug: 28555675 Change-Id: Idbb159b5aa450df2344cd93ae74fef5f55bdc534 --- runtime/interpreter/interpreter.cc | 20 ++++++++- test/597-deopt-new-string/deopt.cc | 59 ++++++++++++++++++++++++++ test/597-deopt-new-string/expected.txt | 2 + test/597-deopt-new-string/info.txt | 1 + test/597-deopt-new-string/run | 18 ++++++++ test/597-deopt-new-string/src/Main.java | 75 +++++++++++++++++++++++++++++++++ test/Android.libarttest.mk | 3 +- 7 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 test/597-deopt-new-string/deopt.cc create mode 100644 test/597-deopt-new-string/expected.txt create mode 100644 test/597-deopt-new-string/info.txt create mode 100644 test/597-deopt-new-string/run create mode 100644 test/597-deopt-new-string/src/Main.java diff --git a/runtime/interpreter/interpreter.cc b/runtime/interpreter/interpreter.cc index 81a396a92..6c630cc48 100644 --- a/runtime/interpreter/interpreter.cc +++ b/runtime/interpreter/interpreter.cc @@ -515,8 +515,24 @@ void EnterInterpreterFromDeoptimize(Thread* self, // instruction, as it already executed. // TODO: should be tested more once b/17586779 is fixed. const Instruction* instr = Instruction::At(&code_item->insns_[dex_pc]); - DCHECK(instr->IsInvoke()); - new_dex_pc = dex_pc + instr->SizeInCodeUnits(); + if (instr->IsInvoke()) { + new_dex_pc = dex_pc + instr->SizeInCodeUnits(); + } else if (instr->Opcode() == Instruction::NEW_INSTANCE) { + // It's possible to deoptimize at a NEW_INSTANCE dex instruciton that's for a + // java string, which is turned into a call into StringFactory.newEmptyString(); + if (kIsDebugBuild) { + ClassLinker* class_linker = Runtime::Current()->GetClassLinker(); + mirror::Class* klass = class_linker->ResolveType( + instr->VRegB_21c(), shadow_frame->GetMethod()); + DCHECK(klass->IsStringClass()); + } + // Skip the dex instruction since we essentially come back from an invocation. + new_dex_pc = dex_pc + instr->SizeInCodeUnits(); + } else { + DCHECK(false) << "Unexpected instruction opcode " << instr->Opcode() + << " at dex_pc " << dex_pc + << " of method: " << PrettyMethod(shadow_frame->GetMethod(), false); + } } else { // Nothing to do, the dex_pc is the one at which the code requested // the deoptimization. diff --git a/test/597-deopt-new-string/deopt.cc b/test/597-deopt-new-string/deopt.cc new file mode 100644 index 000000000..844a786e1 --- /dev/null +++ b/test/597-deopt-new-string/deopt.cc @@ -0,0 +1,59 @@ +/* + * 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 "jni.h" +#include "mirror/class-inl.h" +#include "runtime.h" +#include "thread_list.h" +#include "thread_state.h" +#include "gc/gc_cause.h" +#include "gc/scoped_gc_critical_section.h" + +namespace art { + +extern "C" JNIEXPORT void JNICALL Java_Main_deoptimizeAll( + JNIEnv* env, + jclass cls ATTRIBUTE_UNUSED) { + ScopedObjectAccess soa(env); + ScopedThreadSuspension sts(Thread::Current(), kWaitingForDeoptimization); + gc::ScopedGCCriticalSection gcs(Thread::Current(), + gc::kGcCauseInstrumentation, + gc::kCollectorTypeInstrumentation); + // We need to suspend mutator threads first. + ScopedSuspendAll ssa(__FUNCTION__); + static bool first = true; + if (first) { + // We need to enable deoptimization once in order to call DeoptimizeEverything(). + Runtime::Current()->GetInstrumentation()->EnableDeoptimization(); + first = false; + } + Runtime::Current()->GetInstrumentation()->DeoptimizeEverything("test"); +} + +extern "C" JNIEXPORT void JNICALL Java_Main_undeoptimizeAll( + JNIEnv* env, + jclass cls ATTRIBUTE_UNUSED) { + ScopedObjectAccess soa(env); + ScopedThreadSuspension sts(Thread::Current(), kWaitingForDeoptimization); + gc::ScopedGCCriticalSection gcs(Thread::Current(), + gc::kGcCauseInstrumentation, + gc::kCollectorTypeInstrumentation); + // We need to suspend mutator threads first. + ScopedSuspendAll ssa(__FUNCTION__); + Runtime::Current()->GetInstrumentation()->UndeoptimizeEverything("test"); +} + +} // namespace art diff --git a/test/597-deopt-new-string/expected.txt b/test/597-deopt-new-string/expected.txt new file mode 100644 index 000000000..f993efcda --- /dev/null +++ b/test/597-deopt-new-string/expected.txt @@ -0,0 +1,2 @@ +JNI_OnLoad called +Finishing diff --git a/test/597-deopt-new-string/info.txt b/test/597-deopt-new-string/info.txt new file mode 100644 index 000000000..1bd1f79c0 --- /dev/null +++ b/test/597-deopt-new-string/info.txt @@ -0,0 +1 @@ +Regression test for b/28555675 diff --git a/test/597-deopt-new-string/run b/test/597-deopt-new-string/run new file mode 100644 index 000000000..9776ab3eb --- /dev/null +++ b/test/597-deopt-new-string/run @@ -0,0 +1,18 @@ +#!/bin/bash +# +# 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. + +# We want to run in debuggable mode which keeps the call into StringFactory.newEmptyString(). +exec ${RUN} -Xcompiler-option --debuggable "${@}" diff --git a/test/597-deopt-new-string/src/Main.java b/test/597-deopt-new-string/src/Main.java new file mode 100644 index 000000000..1224e407b --- /dev/null +++ b/test/597-deopt-new-string/src/Main.java @@ -0,0 +1,75 @@ +/* + * 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 Main implements Runnable { + static final int numberOfThreads = 2; + static final int totalOperations = 40000; + static boolean sFlag = false; + static volatile boolean done = false; + int threadIndex; + + public static native void deoptimizeAll(); + public static native void undeoptimizeAll(); + + Main(int index) { + threadIndex = index; + } + + public static void main(String[] args) throws Exception { + System.loadLibrary(args[0]); + + final Thread[] threads = new Thread[numberOfThreads]; + for (int t = 0; t < threads.length; t++) { + threads[t] = new Thread(new Main(t)); + threads[t].start(); + } + for (Thread t : threads) { + t.join(); + } + System.out.println("Finishing"); + } + + public String $noinline$run0() { + // Prevent inlining. + if (sFlag) { + throw new Error(); + } + char[] arr = {'a', 'b', 'c'}; + return new String(arr, 0, arr.length); + } + + public void run() { + if (threadIndex == 0) { + // This thread keeps doing deoptimization of all threads. + // Hopefully that will trigger one deoptimization when returning from + // StringFactory.newEmptyString() in one of the other threads. + for (int i = 0; i < totalOperations; ++i) { + if (i % 50 == 0) { + deoptimizeAll(); + } + if (i % 50 == 25) { + undeoptimizeAll(); + } + } + done = true; + } else { + // This thread keeps doing new String() from a char array. + while (!done) { + $noinline$run0(); + } + } + } +} diff --git a/test/Android.libarttest.mk b/test/Android.libarttest.mk index d6f5d372a..1550cc440 100644 --- a/test/Android.libarttest.mk +++ b/test/Android.libarttest.mk @@ -43,7 +43,8 @@ LIBARTTEST_COMMON_SRC_FILES := \ 566-polymorphic-inlining/polymorphic_inline.cc \ 570-checker-osr/osr.cc \ 595-profile-saving/profile-saving.cc \ - 596-app-images/app_images.cc + 596-app-images/app_images.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 -- 2.11.0