From 4493f92dfc70e01d95ad57bc4c0c38f59a616f70 Mon Sep 17 00:00:00 2001 From: Alex Light Date: Fri, 10 Feb 2017 16:33:26 +0000 Subject: [PATCH] Revert "Revert "Make class redefinition work with native methods on stack."" When we were scanning the stack for tracing we were getting the wrong stack-frame size for obsolete native methods. This fixes it by creating real obsolete native methods so we can recognize them and then doing the (rather long) calculation to find their real stack-frame size. This reverts commit 7558d27ccd0837fef7c4bfbff7fc82f07a787316. Reason for revert: Fixed tracing failures. Test: mma -j40 test-art-host Test: ART_TEST_TRACE=true ART_TEST_INTERPRETER=true mma -j40 test-art-host Change-Id: Ic65da1b51a43733ff60131832753afa0c4ce66b1 --- runtime/art_method.cc | 43 +++++++++++++++- runtime/openjdkjvmti/ti_redefine.cc | 5 ++ runtime/stack.cc | 8 +-- test/945-obsolete-native/build | 17 +++++++ test/945-obsolete-native/expected.txt | 9 ++++ test/945-obsolete-native/info.txt | 1 + test/945-obsolete-native/obsolete_native.cc | 51 +++++++++++++++++++ test/945-obsolete-native/run | 17 +++++++ test/945-obsolete-native/src/Main.java | 77 +++++++++++++++++++++++++++++ test/945-obsolete-native/src/Transform.java | 25 ++++++++++ test/Android.bp | 1 + test/ti-agent/common_load.cc | 1 + 12 files changed, 250 insertions(+), 5 deletions(-) create mode 100755 test/945-obsolete-native/build create mode 100644 test/945-obsolete-native/expected.txt create mode 100644 test/945-obsolete-native/info.txt create mode 100644 test/945-obsolete-native/obsolete_native.cc create mode 100755 test/945-obsolete-native/run create mode 100644 test/945-obsolete-native/src/Main.java create mode 100644 test/945-obsolete-native/src/Transform.java diff --git a/runtime/art_method.cc b/runtime/art_method.cc index 6cb854461..5a93e2966 100644 --- a/runtime/art_method.cc +++ b/runtime/art_method.cc @@ -442,12 +442,51 @@ static uint32_t GetOatMethodIndexFromMethodIndex(const DexFile& dex_file, UNREACHABLE(); } +// We use the method's DexFile and declaring class name to find the OatMethod for an obsolete +// method. This is extremely slow but we need it if we want to be able to have obsolete native +// methods since we need this to find the size of it's stack frames. +static const OatFile::OatMethod FindOatMethodFromDexFileFor(ArtMethod* method, bool* found) + REQUIRES_SHARED(Locks::mutator_lock_) { + DCHECK(method->IsObsolete() && method->IsNative()); + const DexFile* dex_file = method->GetDexFile(); + + // recreate the class_def_index from the descriptor. + std::string descriptor_storage; + const DexFile::TypeId* declaring_class_type_id = + dex_file->FindTypeId(method->GetDeclaringClass()->GetDescriptor(&descriptor_storage)); + CHECK(declaring_class_type_id != nullptr); + dex::TypeIndex declaring_class_type_index = dex_file->GetIndexForTypeId(*declaring_class_type_id); + const DexFile::ClassDef* declaring_class_type_def = + dex_file->FindClassDef(declaring_class_type_index); + CHECK(declaring_class_type_def != nullptr); + uint16_t declaring_class_def_index = dex_file->GetIndexForClassDef(*declaring_class_type_def); + + size_t oat_method_index = GetOatMethodIndexFromMethodIndex(*dex_file, + declaring_class_def_index, + method->GetDexMethodIndex()); + + OatFile::OatClass oat_class = OatFile::FindOatClass(*dex_file, + declaring_class_def_index, + found); + if (!(*found)) { + return OatFile::OatMethod::Invalid(); + } + return oat_class.GetOatMethod(oat_method_index); +} + static const OatFile::OatMethod FindOatMethodFor(ArtMethod* method, PointerSize pointer_size, bool* found) REQUIRES_SHARED(Locks::mutator_lock_) { - // We shouldn't be calling this with obsolete methods. - DCHECK(!method->IsObsolete()); + if (UNLIKELY(method->IsObsolete())) { + // We shouldn't be calling this with obsolete methods except for native obsolete methods for + // which we need to use the oat method to figure out how large the quick frame is. + DCHECK(method->IsNative()) << "We should only be finding the OatMethod of obsolete methods in " + << "order to allow stack walking. Other obsolete methods should " + << "never need to access this information."; + DCHECK_EQ(pointer_size, kRuntimePointerSize) << "Obsolete method in compiler!"; + return FindOatMethodFromDexFileFor(method, found); + } // Although we overwrite the trampoline of non-static methods, we may get here via the resolution // method for direct methods (or virtual methods made direct). mirror::Class* declaring_class = method->GetDeclaringClass(); diff --git a/runtime/openjdkjvmti/ti_redefine.cc b/runtime/openjdkjvmti/ti_redefine.cc index f0c0dbcbf..c35419cef 100644 --- a/runtime/openjdkjvmti/ti_redefine.cc +++ b/runtime/openjdkjvmti/ti_redefine.cc @@ -452,6 +452,11 @@ void Redefiner::ClassRedefinition::FindAndAllocateObsoleteMethods(art::mirror::C CallbackCtx ctx(linker->GetAllocatorForClassLoader(art_klass->GetClassLoader())); // Add all the declared methods to the map for (auto& m : art_klass->GetDeclaredMethods(art::kRuntimePointerSize)) { + // TODO It should be possible to simply filter out some methods where they cannot really become + // obsolete, such as native methods and keep their original (possibly optimized) + // implementations. We don't do this, however, since we would need to mark these functions + // (still in the classes declared_methods array) as obsolete so we will find the correct dex + // file to get meta-data from (for example about stack-frame size). ctx.obsolete_methods.insert(&m); // TODO Allow this or check in IsModifiableClass. DCHECK(!m.IsIntrinsic()); diff --git a/runtime/stack.cc b/runtime/stack.cc index d7ba1d75d..96fc66458 100644 --- a/runtime/stack.cc +++ b/runtime/stack.cc @@ -874,9 +874,11 @@ void StackVisitor::WalkStack(bool include_transitions) { CHECK_EQ(GetMethod(), callee) << "Expected: " << ArtMethod::PrettyMethod(callee) << " Found: " << ArtMethod::PrettyMethod(GetMethod()); } else { - CHECK_EQ(instrumentation_frame.method_, GetMethod()) + // Instrumentation generally doesn't distinguish between a method's obsolete and + // non-obsolete version. + CHECK_EQ(instrumentation_frame.method_, GetMethod()->GetNonObsoleteMethod()) << "Expected: " << ArtMethod::PrettyMethod(instrumentation_frame.method_) - << " Found: " << ArtMethod::PrettyMethod(GetMethod()); + << " Found: " << ArtMethod::PrettyMethod(GetMethod()->GetNonObsoleteMethod()); } if (num_frames_ != 0) { // Check agreement of frame Ids only if num_frames_ is computed to avoid infinite @@ -903,7 +905,7 @@ void StackVisitor::WalkStack(bool include_transitions) { << " native=" << method->IsNative() << std::noboolalpha << " entrypoints=" << method->GetEntryPointFromQuickCompiledCode() - << "," << method->GetEntryPointFromJni() + << "," << (method->IsNative() ? method->GetEntryPointFromJni() : nullptr) << " next=" << *cur_quick_frame_; } diff --git a/test/945-obsolete-native/build b/test/945-obsolete-native/build new file mode 100755 index 000000000..898e2e54a --- /dev/null +++ b/test/945-obsolete-native/build @@ -0,0 +1,17 @@ +#!/bin/bash +# +# Copyright 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. + +./default-build "$@" --experimental agents diff --git a/test/945-obsolete-native/expected.txt b/test/945-obsolete-native/expected.txt new file mode 100644 index 000000000..83efda144 --- /dev/null +++ b/test/945-obsolete-native/expected.txt @@ -0,0 +1,9 @@ +hello +Not doing anything here +goodbye +hello +transforming calling function +goodbye +Hello - Transformed +Not doing anything here +Goodbye - Transformed diff --git a/test/945-obsolete-native/info.txt b/test/945-obsolete-native/info.txt new file mode 100644 index 000000000..c8b892ced --- /dev/null +++ b/test/945-obsolete-native/info.txt @@ -0,0 +1 @@ +Tests basic obsolete method support diff --git a/test/945-obsolete-native/obsolete_native.cc b/test/945-obsolete-native/obsolete_native.cc new file mode 100644 index 000000000..061e7afbb --- /dev/null +++ b/test/945-obsolete-native/obsolete_native.cc @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2017 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 +#include +#include + +#include "android-base/stringprintf.h" + +#include "android-base/stringprintf.h" +#include "base/logging.h" +#include "base/macros.h" +#include "jni.h" +#include "openjdkjvmti/jvmti.h" +#include "ScopedLocalRef.h" +#include "ti-agent/common_helper.h" +#include "ti-agent/common_load.h" + +namespace art { +namespace Test945ObsoleteNative { + +extern "C" JNIEXPORT void JNICALL Java_Main_bindTest945ObsoleteNative( + JNIEnv* env, jclass klass ATTRIBUTE_UNUSED) { + BindFunctions(jvmti_env, env, "Transform"); +} + +extern "C" JNIEXPORT void JNICALL Java_Transform_doExecute(JNIEnv* env, + jclass klass ATTRIBUTE_UNUSED, + jobject runnable) { + jclass runnable_klass = env->FindClass("java/lang/Runnable"); + DCHECK(runnable_klass != nullptr); + jmethodID run_method = env->GetMethodID(runnable_klass, "run", "()V"); + env->CallVoidMethod(runnable, run_method); +} + + +} // namespace Test945ObsoleteNative +} // namespace art diff --git a/test/945-obsolete-native/run b/test/945-obsolete-native/run new file mode 100755 index 000000000..c6e62ae6c --- /dev/null +++ b/test/945-obsolete-native/run @@ -0,0 +1,17 @@ +#!/bin/bash +# +# Copyright 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. + +./default-run "$@" --jvmti diff --git a/test/945-obsolete-native/src/Main.java b/test/945-obsolete-native/src/Main.java new file mode 100644 index 000000000..5e2154e9a --- /dev/null +++ b/test/945-obsolete-native/src/Main.java @@ -0,0 +1,77 @@ +/* + * 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.util.Base64; + +public class Main { + // class Transform { + // public void sayHi(Runnable r) { + // System.out.println("Hello - Transformed"); + // doExecute(r); + // System.out.println("Goodbye - Transformed"); + // } + // + // private static native void doExecute(Runnable r); + // } + private static final byte[] CLASS_BYTES = Base64.getDecoder().decode( + "yv66vgAAADQAIgoACAASCQATABQIABUKABYAFwoABwAYCAAZBwAaBwAbAQAGPGluaXQ+AQADKClW" + + "AQAEQ29kZQEAD0xpbmVOdW1iZXJUYWJsZQEABXNheUhpAQAXKExqYXZhL2xhbmcvUnVubmFibGU7" + + "KVYBAAlkb0V4ZWN1dGUBAApTb3VyY2VGaWxlAQAOVHJhbnNmb3JtLmphdmEMAAkACgcAHAwAHQAe" + + "AQATSGVsbG8gLSBUcmFuc2Zvcm1lZAcAHwwAIAAhDAAPAA4BABVHb29kYnllIC0gVHJhbnNmb3Jt" + + "ZWQBAAlUcmFuc2Zvcm0BABBqYXZhL2xhbmcvT2JqZWN0AQAQamF2YS9sYW5nL1N5c3RlbQEAA291" + + "dAEAFUxqYXZhL2lvL1ByaW50U3RyZWFtOwEAE2phdmEvaW8vUHJpbnRTdHJlYW0BAAdwcmludGxu" + + "AQAVKExqYXZhL2xhbmcvU3RyaW5nOylWACAABwAIAAAAAAADAAAACQAKAAEACwAAAB0AAQABAAAA" + + "BSq3AAGxAAAAAQAMAAAABgABAAAAEQABAA0ADgABAAsAAAA5AAIAAgAAABWyAAISA7YABCu4AAWy" + + "AAISBrYABLEAAAABAAwAAAASAAQAAAATAAgAFAAMABUAFAAWAQoADwAOAAAAAQAQAAAAAgAR"); + private static final byte[] DEX_BYTES = Base64.getDecoder().decode( + "ZGV4CjAzNQB1fZcJR/opPuXacK8mIla5shH0LSg72qJYAwAAcAAAAHhWNBIAAAAAAAAAALgCAAAR" + + "AAAAcAAAAAcAAAC0AAAAAwAAANAAAAABAAAA9AAAAAUAAAD8AAAAAQAAACQBAAAUAgAARAEAAKIB" + + "AACqAQAAwQEAANYBAADjAQAA+gEAAA4CAAAkAgAAOAIAAEwCAABcAgAAXwIAAGMCAABuAgAAggIA" + + "AIcCAACQAgAAAwAAAAQAAAAFAAAABgAAAAcAAAAIAAAACgAAAAoAAAAGAAAAAAAAAAsAAAAGAAAA" + + "lAEAAAsAAAAGAAAAnAEAAAUAAQAOAAAAAAAAAAAAAAAAAAEADAAAAAAAAQAQAAAAAQACAA8AAAAC" + + "AAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAJAAAAAAAAAKUCAAAAAAAAAQABAAEAAACXAgAABAAAAHAQ" + + "BAAAAA4ABAACAAIAAACcAgAAFAAAAGIAAAAbAQIAAABuIAMAEABxEAEAAwBiAAAAGwEBAAAAbiAD" + + "ABAADgABAAAAAwAAAAEAAAAEAAY8aW5pdD4AFUdvb2RieWUgLSBUcmFuc2Zvcm1lZAATSGVsbG8g" + + "LSBUcmFuc2Zvcm1lZAALTFRyYW5zZm9ybTsAFUxqYXZhL2lvL1ByaW50U3RyZWFtOwASTGphdmEv" + + "bGFuZy9PYmplY3Q7ABRMamF2YS9sYW5nL1J1bm5hYmxlOwASTGphdmEvbGFuZy9TdHJpbmc7ABJM" + + "amF2YS9sYW5nL1N5c3RlbTsADlRyYW5zZm9ybS5qYXZhAAFWAAJWTAAJZG9FeGVjdXRlABJlbWl0" + + "dGVyOiBqYWNrLTQuMjUAA291dAAHcHJpbnRsbgAFc2F5SGkAEQAHDgATAQAHDoc8hwAAAAIBAICA" + + "BMQCAYoCAAIB3AIADQAAAAAAAAABAAAAAAAAAAEAAAARAAAAcAAAAAIAAAAHAAAAtAAAAAMAAAAD" + + "AAAA0AAAAAQAAAABAAAA9AAAAAUAAAAFAAAA/AAAAAYAAAABAAAAJAEAAAEgAAACAAAARAEAAAEQ" + + "AAACAAAAlAEAAAIgAAARAAAAogEAAAMgAAACAAAAlwIAAAAgAAABAAAApQIAAAAQAAABAAAAuAIA" + + "AA=="); + + public static void main(String[] args) { + bindTest945ObsoleteNative(); + doTest(new Transform()); + } + + public static void doTest(Transform t) { + t.sayHi(() -> { System.out.println("Not doing anything here"); }); + t.sayHi(() -> { + System.out.println("transforming calling function"); + doCommonClassRedefinition(Transform.class, CLASS_BYTES, DEX_BYTES); + }); + t.sayHi(() -> { System.out.println("Not doing anything here"); }); + } + + // Transforms the class + private static native void doCommonClassRedefinition(Class target, + byte[] classfile, + byte[] dexfile); + + private static native void bindTest945ObsoleteNative(); +} diff --git a/test/945-obsolete-native/src/Transform.java b/test/945-obsolete-native/src/Transform.java new file mode 100644 index 000000000..2b7cc1b3a --- /dev/null +++ b/test/945-obsolete-native/src/Transform.java @@ -0,0 +1,25 @@ +/* + * 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 Transform { + public void sayHi(Runnable r) { + System.out.println("hello"); + doExecute(r); + System.out.println("goodbye"); + } + + private static native void doExecute(Runnable r); +} diff --git a/test/Android.bp b/test/Android.bp index d3244a683..00c890a83 100644 --- a/test/Android.bp +++ b/test/Android.bp @@ -274,6 +274,7 @@ art_cc_defaults { "933-misc-events/misc_events.cc", "936-search-onload/search_onload.cc", "944-transform-classloaders/classloader.cc", + "945-obsolete-native/obsolete_native.cc", ], shared_libs: [ "libbase", diff --git a/test/ti-agent/common_load.cc b/test/ti-agent/common_load.cc index c5a93568c..351857d1d 100644 --- a/test/ti-agent/common_load.cc +++ b/test/ti-agent/common_load.cc @@ -122,6 +122,7 @@ static AgentLib agents[] = { { "942-private-recursive", common_redefine::OnLoad, nullptr }, { "943-private-recursive-jit", common_redefine::OnLoad, nullptr }, { "944-transform-classloaders", common_redefine::OnLoad, nullptr }, + { "945-obsolete-native", common_redefine::OnLoad, nullptr }, }; static AgentLib* FindAgent(char* name) { -- 2.11.0