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
Bug: 31000839
Change-Id: I092d2e9553cd1c577036d78e8563a7a39d6cb7b9
diff --git a/compiler/image_test.cc b/compiler/image_test.cc
index 1290379..2686abc 100644
--- a/compiler/image_test.cc
+++ b/compiler/image_test.cc
@@ -24,8 +24,10 @@
#include "base/unix_file/fd_file.h"
#include "class_linker-inl.h"
+#include "compiler_callbacks.h"
#include "common_compiler_test.h"
#include "debug/method_debug_info.h"
+#include "dex/quick_compiler_callbacks.h"
#include "driver/compiler_options.h"
#include "elf_writer.h"
#include "elf_writer_quick.h"
@@ -76,6 +78,14 @@
const std::string& extra_dex = "",
const std::string& image_class = "");
+ void SetUpRuntimeOptions(RuntimeOptions* options) OVERRIDE {
+ CommonCompilerTest::SetUpRuntimeOptions(options);
+ callbacks_.reset(new QuickCompilerCallbacks(
+ verification_results_.get(),
+ CompilerCallbacks::CallbackMode::kCompileBootImage));
+ options->push_back(std::make_pair("compilercallbacks", callbacks_.get()));
+ }
+
std::unordered_set<std::string>* GetImageClasses() OVERRIDE {
return new std::unordered_set<std::string>(image_classes_);
}
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 7e23c8b..715b237 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -421,6 +421,19 @@
if (method != nullptr) {
if (verifier.HasInstructionThatWillThrow()) {
method->AddAccessFlags(kAccCompileDontBother);
+ if (Runtime::Current()->IsAotCompiler() &&
+ (callbacks != nullptr) && !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 0000000..6a5618e
--- /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 0000000..e9ef86b
--- /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 0000000..6c5720a
--- /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 0000000..410504c
--- /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 0000000..aa84724
--- /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 285f3aa..f26e122 100644
--- a/test/common/runtime_state.cc
+++ b/test/common/runtime_state.cc
@@ -188,4 +188,28 @@
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<mirror::Class>(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 f395866..566f7ba 100755
--- a/test/etc/run-test-jar
+++ b/test/etc/run-test-jar
@@ -59,6 +59,7 @@
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 @@
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 @@
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} \
