From 132eb43396bdb0b9bdacf069289f019d85d358fc Mon Sep 17 00:00:00 2001 From: Calin Juravle Date: Fri, 28 Oct 2016 20:09:56 +0100 Subject: [PATCH] Fix JIT crash due to unverified dead code The JIT compiler assumes that it only gets completely verified code. To work around potential unverified dead code it uses kAccDontBotherCompile flag set during runtime verification. However, if a class is verified during a prior dex2oat the flag is not persisted and JIT happily things that everything is ok. The simplest fix is to mark classes with potential unverified dex code as verify at runtime. We only do this for apps and assume that everything in the boot image is well formed. Test: m test-art-host-706-jit-skip-compilation Bug: 31000839 Change-Id: Ib73de1888581bb7202474cfd7aca70af4cc2cc00 --- runtime/verifier/method_verifier.cc | 12 +++++ test/706-jit-skip-compilation/expected.txt | 1 + test/706-jit-skip-compilation/info.txt | 4 ++ test/706-jit-skip-compilation/run | 19 ++++++++ test/706-jit-skip-compilation/smali/errclass.smali | 34 +++++++++++++ test/706-jit-skip-compilation/src/Main.java | 57 ++++++++++++++++++++++ test/common/runtime_state.cc | 24 +++++++++ test/etc/run-test-jar | 11 ++++- 8 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 test/706-jit-skip-compilation/expected.txt create mode 100644 test/706-jit-skip-compilation/info.txt create mode 100644 test/706-jit-skip-compilation/run create mode 100644 test/706-jit-skip-compilation/smali/errclass.smali create mode 100644 test/706-jit-skip-compilation/src/Main.java diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc index 7e23c8b4b..ff91fc380 100644 --- a/runtime/verifier/method_verifier.cc +++ b/runtime/verifier/method_verifier.cc @@ -421,6 +421,18 @@ MethodVerifier::FailureData MethodVerifier::VerifyMethod(Thread* self, if (method != nullptr) { if (verifier.HasInstructionThatWillThrow()) { method->AddAccessFlags(kAccCompileDontBother); + if (Runtime::Current()->IsAotCompiler() && !callbacks->IsBootImage()) { + // When compiling apps, make HasInstructionThatWillThrow a soft error to trigger + // re-verification at runtime. + // The dead code after the throw is not verified and might be invalid. This may cause + // the JIT compiler to crash since it assumes that all the code is valid. + // + // There's a strong assumption that the entire boot image is verified and all its dex + // code is valid (even the dead and unverified one). As such this is done only for apps. + // (CompilerDriver DCHECKs in VerifyClassVisitor that methods from boot image are + // fully verified). + result.kind = kSoftFailure; + } } if ((verifier.encountered_failure_types_ & VerifyError::VERIFY_ERROR_LOCKING) != 0) { method->AddAccessFlags(kAccMustCountLocks); diff --git a/test/706-jit-skip-compilation/expected.txt b/test/706-jit-skip-compilation/expected.txt new file mode 100644 index 000000000..6a5618ebc --- /dev/null +++ b/test/706-jit-skip-compilation/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/706-jit-skip-compilation/info.txt b/test/706-jit-skip-compilation/info.txt new file mode 100644 index 000000000..e9ef86bfb --- /dev/null +++ b/test/706-jit-skip-compilation/info.txt @@ -0,0 +1,4 @@ +Regression test for the JIT crashing when compiling a method with invalid +dead dex code. For not compilable methods we don't gather samples and we don't +trigger JIT compilation. However kAccDontBotherCompile is not persisted in the +oat file and so we may end up compiling a method which we shouldn't. diff --git a/test/706-jit-skip-compilation/run b/test/706-jit-skip-compilation/run new file mode 100644 index 000000000..6c5720a09 --- /dev/null +++ b/test/706-jit-skip-compilation/run @@ -0,0 +1,19 @@ +#!/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. + +# Run without the app image, otherwise the verification results will be cached +# in the ArtMethod of the image and the test will be skewed. +exec ${RUN} "${@}" --no-app-image diff --git a/test/706-jit-skip-compilation/smali/errclass.smali b/test/706-jit-skip-compilation/smali/errclass.smali new file mode 100644 index 000000000..410504cb2 --- /dev/null +++ b/test/706-jit-skip-compilation/smali/errclass.smali @@ -0,0 +1,34 @@ +# 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. + + +.class public LErrClass; + +.super Ljava/lang/Object; + +.method public static errMethod()J + .registers 8 + const/4 v0, 0x0 + const/4 v3, 0x0 + aget v1, v0, v3 # v0 is null, this will alays throw and the invalid code + # below will not be verified. + move v3, v4 + move-wide/from16 v6, v2 # should trigger a verification error if verified as + # v3 is a single register but used as a pair here. + return v6 +.end method + +# Add a field to work around demerger bug b/18051191. +# Failure to verify dex file '...': Offset(552) should be zero when size is zero for field-ids. +.field private a:I diff --git a/test/706-jit-skip-compilation/src/Main.java b/test/706-jit-skip-compilation/src/Main.java new file mode 100644 index 000000000..aa847248d --- /dev/null +++ b/test/706-jit-skip-compilation/src/Main.java @@ -0,0 +1,57 @@ +/* + * 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.InvocationTargetException; +import java.lang.reflect.Method; + +public class Main { + public static void main(String[] args) throws Exception { + System.loadLibrary(args[0]); + Class c = Class.forName("ErrClass"); + Method m = c.getMethod("errMethod"); + + // Print the counter before invokes. The golden file expects this to be 0. + int hotnessCounter = getHotnessCounter(c, "errMethod"); + if (hotnessCounter != 0) { + throw new RuntimeException("Unexpected hotnessCounter: " + hotnessCounter); + } + + // Loop enough to make sure the interpreter reports invocations count. + long result = 0; + for (int i = 0; i < 10000; i++) { + try { + result += (Long)m.invoke(null); + hotnessCounter = getHotnessCounter(c, "errMethod"); + if (hotnessCounter != 0) { + throw new RuntimeException( + "Unexpected hotnessCounter: " + hotnessCounter); + } + + } catch (InvocationTargetException e) { + if (!(e.getCause() instanceof NullPointerException)) { + throw e; + } + } + } + + // Not compilable methods should not increase their hotness counter. + if (hotnessCounter != 0) { + throw new RuntimeException("Unexpected hotnessCounter: " + hotnessCounter); + } + } + + public static native int getHotnessCounter(Class cls, String method_name); +} diff --git a/test/common/runtime_state.cc b/test/common/runtime_state.cc index 285f3aa54..f26e12258 100644 --- a/test/common/runtime_state.cc +++ b/test/common/runtime_state.cc @@ -188,4 +188,28 @@ extern "C" JNIEXPORT jboolean JNICALL Java_Main_hasSingleImplementation(JNIEnv* return method->HasSingleImplementation(); } +extern "C" JNIEXPORT int JNICALL Java_Main_getHotnessCounter(JNIEnv* env, + jclass, + jclass cls, + jstring method_name) { + jit::Jit* jit = Runtime::Current()->GetJit(); + if (jit == nullptr) { + // The hotness counter is valid only under JIT. + // If we don't JIT return 0 to match test expectations. + return 0; + } + + ArtMethod* method = nullptr; + { + ScopedObjectAccess soa(Thread::Current()); + + ScopedUtfChars chars(env, method_name); + CHECK(chars.c_str() != nullptr); + method = soa.Decode(cls)->FindDeclaredDirectMethodByName( + chars.c_str(), kRuntimePointerSize); + } + + return method->GetCounter(); +} + } // namespace art diff --git a/test/etc/run-test-jar b/test/etc/run-test-jar index f3958662d..566f7ba52 100755 --- a/test/etc/run-test-jar +++ b/test/etc/run-test-jar @@ -59,6 +59,7 @@ ARGS="" EXTERNAL_LOG_TAGS="n" # if y respect externally set ANDROID_LOG_TAGS. DRY_RUN="n" # if y prepare to run the test but don't run it. TEST_VDEX="n" +APP_IMAGE="y" while true; do if [ "x$1" = "x--quiet" ]; then @@ -132,6 +133,9 @@ while true; do elif [ "x$1" = "x--prebuild" ]; then PREBUILD="y" shift + elif [ "x$1" = "x--no-app-image" ]; then + APP_IMAGE="n" + shift elif [ "x$1" = "x--strip-dex" ]; then STRIP_DEX="y" shift @@ -453,11 +457,14 @@ vdex_cmdline="true" mkdir_locations="${DEX_LOCATION}/dalvik-cache/$ISA" strip_cmdline="true" -# Pick a base that will force the app image to get relocated. -app_image="--base=0x4000 --app-image-file=$DEX_LOCATION/oat/$ISA/$TEST_NAME.art" if [ "$PREBUILD" = "y" ]; then mkdir_locations="${mkdir_locations} ${DEX_LOCATION}/oat/$ISA" + if [ "$APP_IMAGE" = "y" ]; then + # Pick a base that will force the app image to get relocated. + app_image="--base=0x4000 --app-image-file=$DEX_LOCATION/oat/$ISA/$TEST_NAME.art" + fi + dex2oat_cmdline="$INVOKE_WITH $ANDROID_ROOT/bin/dex2oatd \ $COMPILE_FLAGS \ --boot-image=${BOOT_IMAGE} \ -- 2.11.0