Jit-zygote compiled code isn't debuggable
We check the debuggablity of compiled code by checking to see if it
was compiled by the JIT. This works since we throw all precompiled
code out when transitioning to debuggable. Unfortunately with
jit-zygote the non-debuggable zygote methods can be incorrectly seen
as debuggable when encountered on the stack. This can lead to
incorrect deoptimization and other issues. To fix this we explicitly
exclude jit-zygote code from the check.
Bug: 144947842
Test: ./test.py --host
Change-Id: I4e953f64f8261b7a16d7c3199cec89998af0c1cf
Merged-In: I4e953f64f8261b7a16d7c3199cec89998af0c1cf
(cherry picked from commit 280e6c323419ad08860514ff8c09eefb8fc8e969)
diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 2415a9f..c43fe11 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -273,8 +273,12 @@
JitCodeCache::~JitCodeCache() {}
+bool JitCodeCache::PrivateRegionContainsPc(const void* ptr) const {
+ return private_region_.IsInExecSpace(ptr);
+}
+
bool JitCodeCache::ContainsPc(const void* ptr) const {
- return private_region_.IsInExecSpace(ptr) || shared_region_.IsInExecSpace(ptr);
+ return PrivateRegionContainsPc(ptr) || shared_region_.IsInExecSpace(ptr);
}
bool JitCodeCache::WillExecuteJitCode(ArtMethod* method) {
diff --git a/runtime/jit/jit_code_cache.h b/runtime/jit/jit_code_cache.h
index 22b43cc..d8216d2 100644
--- a/runtime/jit/jit_code_cache.h
+++ b/runtime/jit/jit_code_cache.h
@@ -225,6 +225,9 @@
// Return true if the code cache contains this pc.
bool ContainsPc(const void* pc) const;
+ // Return true if the code cache contains this pc in the private region (i.e. not from zygote).
+ bool PrivateRegionContainsPc(const void* pc) const;
+
// Returns true if either the method's entrypoint is JIT compiled code or it is the
// instrumentation entrypoint and we can jump to jit code for this method. For testing use only.
bool WillExecuteJitCode(ArtMethod* method) REQUIRES(!Locks::jit_lock_);
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 8607991..b739450 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -2735,9 +2735,10 @@
// We could look at the oat file where `code` is being defined,
// and check whether it's been compiled debuggable, but we decided to
// only rely on the JIT for debuggable apps.
- return IsJavaDebuggable() &&
- GetJit() != nullptr &&
- GetJit()->GetCodeCache()->ContainsPc(reinterpret_cast<const void*>(code));
+ // The JIT-zygote is not debuggable so we need to be sure to exclude code from the non-private
+ // region as well.
+ return IsJavaDebuggable() && GetJit() != nullptr &&
+ GetJit()->GetCodeCache()->PrivateRegionContainsPc(reinterpret_cast<const void*>(code));
}
LinearAlloc* Runtime::CreateLinearAlloc() {
diff --git a/test/2031-zygote-compiled-frame-deopt/expected.txt b/test/2031-zygote-compiled-frame-deopt/expected.txt
new file mode 100644
index 0000000..21a75cf
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/expected.txt
@@ -0,0 +1,3 @@
+JNI_OnLoad called
+Starting up!
+This is my object!
diff --git a/test/2031-zygote-compiled-frame-deopt/info.txt b/test/2031-zygote-compiled-frame-deopt/info.txt
new file mode 100644
index 0000000..aa59e4f
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/info.txt
@@ -0,0 +1,5 @@
+Regression test for b/144947842
+
+Check that we correctly identify jit-zygote compiled frames as non-debuggable.
+
+We would hit DCHECKS before (part of) the fix to this bug.
\ No newline at end of file
diff --git a/test/2031-zygote-compiled-frame-deopt/native-wait.cc b/test/2031-zygote-compiled-frame-deopt/native-wait.cc
new file mode 100644
index 0000000..bd1d224
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/native-wait.cc
@@ -0,0 +1,91 @@
+/*
+ * Copyright 2020 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 <atomic>
+#include <sstream>
+
+#include "base/globals.h"
+#include "jit/jit.h"
+#include "jit/jit_code_cache.h"
+#include "jni.h"
+#include "runtime.h"
+#include "thread_list.h"
+
+namespace art {
+namespace NativeWait {
+
+std::atomic<bool> native_waiting = false;
+std::atomic<bool> native_wait = false;
+
+// Perform late debuggable switch in the same way the zygote would (clear-jit,
+// unmark-zygote, set-debuggable, deopt boot, restart jit). NB This skips
+// restarting the heap threads since that doesn't seem to be needed to trigger
+// b/144947842.
+extern "C" JNIEXPORT void JNICALL Java_art_Test2031_simulateZygoteFork(JNIEnv*, jclass) {
+ Runtime* runtime = Runtime::Current();
+ bool has_jit = runtime->GetJit() != nullptr;
+ if (has_jit) {
+ runtime->GetJit()->PreZygoteFork();
+ }
+ runtime->SetAsZygoteChild(/*is_system_server=*/false, /*is_zygote=*/false);
+ runtime->AddCompilerOption("--debuggable");
+ runtime->SetJavaDebuggable(true);
+ {
+ // Deoptimize the boot image as it may be non-debuggable.
+ ScopedSuspendAll ssa(__FUNCTION__);
+ runtime->DeoptimizeBootImage();
+ }
+
+ if (has_jit) {
+ runtime->GetJitCodeCache()->PostForkChildAction(false, false);
+ runtime->GetJit()->PostForkChildAction(false, false);
+ // We have "zygote" code that isn't really part of the BCP. just don't collect it.
+ runtime->GetJitCodeCache()->SetGarbageCollectCode(false);
+ }
+}
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test2031_setupJvmti(JNIEnv* env,
+ jclass,
+ jstring testdir) {
+ const char* td = env->GetStringUTFChars(testdir, nullptr);
+ std::string testdir_str;
+ testdir_str.resize(env->GetStringUTFLength(testdir));
+ memcpy(testdir_str.data(), td, testdir_str.size());
+ env->ReleaseStringUTFChars(testdir, td);
+ std::ostringstream oss;
+ Runtime* runtime = Runtime::Current();
+ oss << testdir_str << (kIsDebugBuild ? "libtiagentd.so" : "libtiagent.so")
+ << "=2031-zygote-compiled-frame-deopt,art";
+ LOG(INFO) << "agent " << oss.str();
+ runtime->AttachAgent(env, oss.str(), nullptr);
+}
+extern "C" JNIEXPORT void JNICALL Java_art_Test2031_waitForNativeSleep(JNIEnv*, jclass) {
+ while (!native_waiting) {
+ }
+}
+extern "C" JNIEXPORT void JNICALL Java_art_Test2031_wakeupNativeSleep(JNIEnv*, jclass) {
+ native_wait = false;
+}
+extern "C" JNIEXPORT void JNICALL Java_art_Test2031_nativeSleep(JNIEnv*, jclass) {
+ native_wait = true;
+ do {
+ native_waiting = true;
+ } while (native_wait);
+ native_waiting = false;
+}
+
+} // namespace NativeWait
+} // namespace art
diff --git a/test/2031-zygote-compiled-frame-deopt/run b/test/2031-zygote-compiled-frame-deopt/run
new file mode 100755
index 0000000..900099f
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/run
@@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# Copyright 2020 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.
+
+# The -Xopaque-jni-ids makes sure we can do structural redefinition. The --add-libdir-argument tells
+# default-run to pass the directory where the jvmti-agent is so we can load it later. The others
+# set the process to zygote mode and setup the jit cache size. We use a larger than normal jit-size
+# to avoid having to deal with jit-gc, a complication that's not relevant to this test.
+./default-run "$@" --runtime-option -Xopaque-jni-ids:true --add-libdir-argument --runtime-option -Xzygote --runtime-option -Xjitinitialsize:64M
diff --git a/test/2031-zygote-compiled-frame-deopt/src/Main.java b/test/2031-zygote-compiled-frame-deopt/src/Main.java
new file mode 100644
index 0000000..5c2eab8
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/src/Main.java
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2020 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 {
+ public static void main(String[] args) throws Exception {
+ System.loadLibrary(args[0]);
+ art.Test2031.$noinline$run(args[1]);
+ }
+}
diff --git a/test/2031-zygote-compiled-frame-deopt/src/art/Redefinition.java b/test/2031-zygote-compiled-frame-deopt/src/art/Redefinition.java
new file mode 120000
index 0000000..81eaf31
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/src/art/Redefinition.java
@@ -0,0 +1 @@
+../../../jvmti-common/Redefinition.java
\ No newline at end of file
diff --git a/test/2031-zygote-compiled-frame-deopt/src/art/Test2031.java b/test/2031-zygote-compiled-frame-deopt/src/art/Test2031.java
new file mode 100644
index 0000000..71cbebd
--- /dev/null
+++ b/test/2031-zygote-compiled-frame-deopt/src/art/Test2031.java
@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) 2020 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.
+ */
+
+package art;
+
+import java.lang.ref.*;
+import java.lang.reflect.*;
+import java.util.*;
+
+public class Test2031 {
+ public static class MyClass {
+ public void starting() {
+ System.out.println("Starting up!");
+ }
+ public String toString() {
+ return "This is my object!";
+ }
+ }
+
+ public static void $noinline$doSimulateZygoteFork() {
+ simulateZygoteFork();
+ }
+
+ public static void $noinline$run(String testdir) throws Exception {
+ $noinline$doSimulateZygoteFork();
+ final MyClass myObject = new MyClass();
+ $noinline$startTest(testdir, myObject);
+ System.out.println(myObject);
+ }
+
+ public static void $noinline$startTest(String testdir, final MyClass myObject) throws Exception {
+ Thread thr = new Thread(() -> {
+ try {
+ waitForNativeSleep();
+ setupJvmti(testdir);
+ Redefinition.setTestConfiguration(Redefinition.Config.COMMON_REDEFINE);
+ Thread tester = new Thread(() -> {
+ try {
+ myObject.starting();
+ doTest();
+ } catch (Exception e) {
+ throw new Error("Failure!", e);
+ }
+ });
+ tester.start();
+ tester.join();
+ wakeupNativeSleep();
+ } catch (Exception e) {
+ throw new Error("Failure!", e);
+ }
+ });
+ thr.start();
+ nativeSleep();
+ thr.join();
+ }
+
+ public static native void simulateZygoteFork();
+ public static native void setupJvmti(String testdir);
+ public static native void waitForNativeSleep();
+ public static native void wakeupNativeSleep();
+ public static native void nativeSleep();
+
+ private static void doRedefinition() {
+ Redefinition.doCommonStructuralClassRedefinition(Transform.class, REDEFINED_DEX_BYTES);
+ }
+
+ public static class SuperTransform {
+ public int id;
+
+ public SuperTransform(int id) {
+ this.id = id;
+ }
+
+ public String toString() {
+ return "SuperTransform { id: " + id + ", class: " + getClass() + " }";
+ }
+ }
+
+ public static class Transform extends SuperTransform {
+ static {
+ }
+
+ public static Object BAR =
+ new Object() {
+ public String toString() {
+ return "value of <" + this.get() + ">";
+ }
+
+ public Object get() {
+ return "BAR FIELD";
+ }
+ };
+ public static Object FOO =
+ new Object() {
+ public String toString() {
+ return "value of <" + this.get() + ">";
+ }
+
+ public Object get() {
+ return "FOO FIELD";
+ }
+ };
+ // This class has no virtual fields or methods. This means we can structurally redefine it
+ // without having to change the size of any instances.
+ public Transform(int id) {
+ super(id);
+ }
+
+ public static String staticToString() {
+ return Transform.class.toString() + "[FOO: " + FOO + ", BAR: " + BAR + "]";
+ }
+ }
+
+ public static class SubTransform extends Transform {
+ public SubTransform(int id) {
+ super(id);
+ }
+
+ public String myToString() {
+ return "SubTransform (subclass of: " + staticToString() + ") { id: " + id + " }";
+ }
+ }
+
+ /* Base64 encoded class of:
+ * public static class Transform extends SuperTransform {
+ * static {}
+ * public Transform(int id) { super(id + 1000); }
+ * // NB This is the order the fields will be laid out in memory.
+ * public static Object BAR;
+ * public static Object BAZ;
+ * public static Object FOO;
+ * public static String staticToString() {
+ * return Transform.class.toString() + "[FOO: " + FOO + ", BAR: " + BAR + ", BAZ: " + BAZ + "]";
+ * }
+ * }
+ */
+ private static byte[] REDEFINED_DEX_BYTES =
+ Base64.getDecoder()
+ .decode(
+"ZGV4CjAzNQC78lC18jI6omumTaKUcf/8pvcR4/Hx2u3QBQAAcAAAAHhWNBIAAAAAAAAAAAwFAAAg" +
+"AAAAcAAAAAsAAADwAAAABQAAABwBAAADAAAAWAEAAAkAAABwAQAAAQAAALgBAAD4AwAA2AEAAKoC" +
+"AACzAgAAvAIAAMYCAADOAgAA0wIAANgCAADdAgAA4AIAAOMCAADnAgAABgMAACADAAAwAwAAVAMA" +
+"AHQDAACHAwAAmwMAAK8DAADKAwAA2QMAAOQDAADnAwAA6wMAAPMDAAD2AwAAAwQAAAsEAAARBAAA" +
+"IQQAACsEAAAyBAAABwAAAAoAAAALAAAADAAAAA0AAAAOAAAADwAAABAAAAARAAAAEgAAABUAAAAI" +
+"AAAACAAAAAAAAAAJAAAACQAAAJQCAAAJAAAACQAAAJwCAAAVAAAACgAAAAAAAAAWAAAACgAAAKQC" +
+"AAACAAcABAAAAAIABwAFAAAAAgAHAAYAAAABAAQAAwAAAAIAAwACAAAAAgAEAAMAAAACAAAAHAAA" +
+"AAYAAAAdAAAACQADAAMAAAAJAAEAGgAAAAkAAgAaAAAACQAAAB0AAAACAAAAAQAAAAEAAAAAAAAA" +
+"EwAAAPwEAADQBAAAAAAAAAUAAAACAAAAjQIAADYAAAAcAAIAbhAEAAAADABiAQIAYgIAAGIDAQAi" +
+"BAkAcBAFAAQAbiAHAAQAGgAXAG4gBwAEAG4gBgAUABoAAABuIAcABABuIAYAJAAaAAEAbiAHAAQA" +
+"biAGADQAGgAYAG4gBwAEAG4QCAAEAAwAEQAAAAAAAAAAAIQCAAABAAAADgAAAAIAAgACAAAAiAIA" +
+"AAYAAADQEegDcCAAABAADgAPAA4AEAEADgAWAA4AAAAAAQAAAAcAAAABAAAACAAAAAEAAAAAAAcs" +
+"IEJBUjogAAcsIEJBWjogAAg8Y2xpbml0PgAGPGluaXQ+AANCQVIAA0JBWgADRk9PAAFJAAFMAAJM" +
+"TAAdTGFydC9UZXN0MjAzMSRTdXBlclRyYW5zZm9ybTsAGExhcnQvVGVzdDIwMzEkVHJhbnNmb3Jt" +
+"OwAOTGFydC9UZXN0MjAzMTsAIkxkYWx2aWsvYW5ub3RhdGlvbi9FbmNsb3NpbmdDbGFzczsAHkxk" +
+"YWx2aWsvYW5ub3RhdGlvbi9Jbm5lckNsYXNzOwARTGphdmEvbGFuZy9DbGFzczsAEkxqYXZhL2xh" +
+"bmcvT2JqZWN0OwASTGphdmEvbGFuZy9TdHJpbmc7ABlMamF2YS9sYW5nL1N0cmluZ0J1aWxkZXI7" +
+"AA1UZXN0MjAzMS5qYXZhAAlUcmFuc2Zvcm0AAVYAAlZJAAZbRk9POiAAAV0AC2FjY2Vzc0ZsYWdz" +
+"AAZhcHBlbmQABG5hbWUADnN0YXRpY1RvU3RyaW5nAAh0b1N0cmluZwAFdmFsdWUAjAF+fkQ4eyJj" +
+"b21waWxhdGlvbi1tb2RlIjoiZGVidWciLCJoYXMtY2hlY2tzdW1zIjpmYWxzZSwibWluLWFwaSI6" +
+"MSwic2hhLTEiOiJkMWQ1MWMxY2IzZTg1YWEzMGUwMGE2ODIyY2NhODNiYmUxZGZlOTQ1IiwidmVy" +
+"c2lvbiI6IjIuMC4xMy1kZXYifQACBAEeGAMCBQIZBAkbFxQDAAMAAAkBCQEJAYiABNQEAYGABOgE" +
+"AQnYAwAAAAAAAAIAAADBBAAAxwQAAPAEAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAEAAAAAAAAAAQAA" +
+"ACAAAABwAAAAAgAAAAsAAADwAAAAAwAAAAUAAAAcAQAABAAAAAMAAABYAQAABQAAAAkAAABwAQAA" +
+"BgAAAAEAAAC4AQAAASAAAAMAAADYAQAAAyAAAAMAAACEAgAAARAAAAMAAACUAgAAAiAAACAAAACq" +
+"AgAABCAAAAIAAADBBAAAACAAAAEAAADQBAAAAxAAAAIAAADsBAAABiAAAAEAAAD8BAAAABAAAAEA" +
+"AAAMBQAA");
+
+ public static void doTest() throws Exception {
+ Transform t1 = new Transform(1);
+ SuperTransform t2 = new SubTransform(2);
+ doRedefinition();
+ }
+}
diff --git a/test/Android.bp b/test/Android.bp
index 400fc3a..dec938e 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -400,6 +400,7 @@
"1964-add-to-dex-classloader-file/add_to_loader.cc",
"1963-add-to-dex-classloader-in-memory/check_memfd_create.cc",
"2012-structural-redefinition-failures-jni-id/set-jni-id-used.cc",
+ "2031-zygote-compiled-frame-deopt/native-wait.cc",
],
static_libs: [
"libz",
@@ -594,6 +595,7 @@
"1972-jni-id-swap-indices/jni_id.cc",
"1985-structural-redefine-stack-scope/stack_scope.cc",
"2011-stack-walk-concurrent-instrument/stack_walk_concurrent.cc",
+ "2031-zygote-compiled-frame-deopt/native-wait.cc",
"common/runtime_state.cc",
"common/stack_inspect.cc",
],
diff --git a/test/etc/run-test-jar b/test/etc/run-test-jar
index 220b7c4..58aa0eb 100755
--- a/test/etc/run-test-jar
+++ b/test/etc/run-test-jar
@@ -52,6 +52,7 @@
JIT="n"
INVOKE_WITH=""
IS_JVMTI_TEST="n"
+ADD_LIBDIR_ARGUMENTS="n"
ISA=x86
LIBRARY_DIRECTORY="lib"
TEST_DIRECTORY="nativetest"
@@ -131,6 +132,9 @@
USE_JVMTI="y"
IS_JVMTI_TEST="y"
shift
+ elif [ "x$1" = "x--add-libdir-argument" ]; then
+ ADD_LIBDIR_ARGUMENTS="y"
+ shift
elif [ "x$1" = "x-O" ]; then
TEST_IS_NDEBUG="y"
shift
@@ -592,6 +596,14 @@
fi
fi
+# Add the libdir to the argv passed to the main function.
+if [ "$ADD_LIBDIR_ARGUMENTS" = "y" ]; then
+ if [[ "$HOST" = "y" ]]; then
+ ARGS="${ARGS} ${ANDROID_HOST_OUT}/${TEST_DIRECTORY}/"
+ else
+ ARGS="${ARGS} /data/${TEST_DIRECTORY}/art/${ISA}/"
+ fi
+fi
if [ "$IS_JVMTI_TEST" = "y" ]; then
agent=libtiagentd.so
lib=tiagentd
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 1dd4fe3..cbf54d9 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1333,6 +1333,13 @@
"description": ["Occasional timeouts."]
},
{
+ "tests": ["2031-zygote-compiled-frame-deopt"],
+ "zipapex": true,
+ "bug": "b/144947842",
+ "description": ["This test requires strong knowledge about where the libdir is",
+ "which the zipapex runner breaks."]
+ },
+ {
"tests": ["909-attach-agent", "126-miranda-multidex"],
"zipapex": true,
"bug": "b/135507613",
diff --git a/test/ti-agent/common_load.cc b/test/ti-agent/common_load.cc
index bfd165d..28265fc 100644
--- a/test/ti-agent/common_load.cc
+++ b/test/ti-agent/common_load.cc
@@ -83,6 +83,7 @@
{ "941-recursive-obsolete-jit", common_redefine::OnLoad, nullptr },
{ "943-private-recursive-jit", common_redefine::OnLoad, nullptr },
{ "1919-vminit-thread-start-timing", Test1919VMInitThreadStart::OnLoad, nullptr },
+ { "2031-zygote-compiled-frame-deopt", nullptr, MinimalOnLoad },
};
static AgentLib* FindAgent(char* name) {
