From 54accbca0b549b1b1ad3ef09655dad438bc1e104 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Wed, 13 Aug 2014 03:40:45 +0100 Subject: [PATCH] Fix bugs in the x86 and arm versions of generic JNI. Also fix the run script of 116-nodex2oat to use the non-prebuild script for target. Bug: 17000769 Change-Id: I439fcd710fb8bb408f3288ff8fb34fef23890adb --- compiler/jni/jni_compiler_test.cc | 100 ++++++++++++++++++--- runtime/arch/arm/quick_entrypoints_arm.S | 24 +++-- runtime/arch/x86/quick_entrypoints_x86.S | 6 +- runtime/common_runtime_test.h | 6 ++ .../quick/quick_trampoline_entrypoints.cc | 10 ++- test/116-nodex2oat/run | 3 + test/MyClassNatives/MyClassNatives.java | 12 ++- 7 files changed, 137 insertions(+), 24 deletions(-) diff --git a/compiler/jni/jni_compiler_test.cc b/compiler/jni/jni_compiler_test.cc index 75d3030ba..deefdecba 100644 --- a/compiler/jni/jni_compiler_test.cc +++ b/compiler/jni/jni_compiler_test.cc @@ -16,6 +16,8 @@ #include +#include + #include "class_linker.h" #include "common_compiler_test.h" #include "dex_file.h" @@ -47,7 +49,7 @@ namespace art { class JniCompilerTest : public CommonCompilerTest { protected: void CompileForTest(jobject class_loader, bool direct, - const char* method_name, const char* method_sig) { + const char* method_name, const char* method_sig, bool generic = false) { ScopedObjectAccess soa(Thread::Current()); StackHandleScope<1> hs(soa.Self()); Handle loader( @@ -61,25 +63,29 @@ class JniCompilerTest : public CommonCompilerTest { method = c->FindVirtualMethod(method_name, method_sig); } ASSERT_TRUE(method != nullptr) << method_name << " " << method_sig; - if (method->GetEntryPointFromQuickCompiledCode() == nullptr || - method->GetEntryPointFromQuickCompiledCode() == class_linker_->GetQuickGenericJniTrampoline()) { - CompileMethod(method); - ASSERT_TRUE(method->GetEntryPointFromQuickCompiledCode() != nullptr) - << method_name << " " << method_sig; - ASSERT_TRUE(method->GetEntryPointFromPortableCompiledCode() != nullptr) - << method_name << " " << method_sig; + if (generic) { + method->SetEntryPointFromQuickCompiledCode(class_linker_->GetQuickGenericJniTrampoline()); + } else { + if (method->GetEntryPointFromQuickCompiledCode() == nullptr || + method->GetEntryPointFromQuickCompiledCode() == class_linker_->GetQuickGenericJniTrampoline()) { + CompileMethod(method); + ASSERT_TRUE(method->GetEntryPointFromQuickCompiledCode() != nullptr) + << method_name << " " << method_sig; + ASSERT_TRUE(method->GetEntryPointFromPortableCompiledCode() != nullptr) + << method_name << " " << method_sig; + } } } void SetUpForTest(bool direct, const char* method_name, const char* method_sig, - void* native_fnptr) { + void* native_fnptr, bool generic = false) { // Initialize class loader and compile method when runtime not started. if (!runtime_->IsStarted()) { { ScopedObjectAccess soa(Thread::Current()); class_loader_ = LoadDex("MyClassNatives"); } - CompileForTest(class_loader_, direct, method_name, method_sig); + CompileForTest(class_loader_, direct, method_name, method_sig, generic); // Start runtime. Thread::Current()->TransitionFromSuspendedToRunnable(); bool started = runtime_->Start(); @@ -424,6 +430,80 @@ TEST_F(JniCompilerTest, CompileAndRunStaticDoubleDoubleMethod) { EXPECT_EQ(2, gJava_MyClassNatives_fooSDD_calls); } +// The x86 generic JNI code had a bug where it assumed a floating +// point return value would be in xmm0. We use log, to somehow ensure +// the compiler will use the floating point stack. + +jdouble Java_MyClassNatives_logD(JNIEnv* env, jclass klass, jdouble x) { + return log(x); +} + +TEST_F(JniCompilerTest, RunGenericStaticLogDoubleethod) { + TEST_DISABLED_FOR_PORTABLE(); + TEST_DISABLED_FOR_MIPS(); + SetUpForTest(true, "logD", "(D)D", + reinterpret_cast(&Java_MyClassNatives_logD), true); + + jdouble result = env_->CallStaticDoubleMethod(jklass_, jmethod_, 2.0); + EXPECT_EQ(log(2.0), result); +} + +jfloat Java_MyClassNatives_logF(JNIEnv* env, jclass klass, jfloat x) { + return logf(x); +} + +TEST_F(JniCompilerTest, RunGenericStaticLogFloatMethod) { + TEST_DISABLED_FOR_PORTABLE(); + TEST_DISABLED_FOR_MIPS(); + SetUpForTest(true, "logF", "(F)F", + reinterpret_cast(&Java_MyClassNatives_logF), true); + + jfloat result = env_->CallStaticFloatMethod(jklass_, jmethod_, 2.0); + EXPECT_EQ(logf(2.0), result); +} + +jboolean Java_MyClassNatives_returnTrue(JNIEnv* env, jclass klass) { + return JNI_TRUE; +} + +jboolean Java_MyClassNatives_returnFalse(JNIEnv* env, jclass klass) { + return JNI_FALSE; +} + +jint Java_MyClassNatives_returnInt(JNIEnv* env, jclass klass) { + return 42; +} + +TEST_F(JniCompilerTest, RunGenericStaticReturnTrue) { + TEST_DISABLED_FOR_PORTABLE(); + TEST_DISABLED_FOR_MIPS(); + SetUpForTest(true, "returnTrue", "()Z", + reinterpret_cast(&Java_MyClassNatives_returnTrue), true); + + jboolean result = env_->CallStaticBooleanMethod(jklass_, jmethod_); + EXPECT_TRUE(result); +} + +TEST_F(JniCompilerTest, RunGenericStaticReturnFalse) { + TEST_DISABLED_FOR_PORTABLE(); + TEST_DISABLED_FOR_MIPS(); + SetUpForTest(true, "returnFalse", "()Z", + reinterpret_cast(&Java_MyClassNatives_returnFalse), true); + + jboolean result = env_->CallStaticBooleanMethod(jklass_, jmethod_); + EXPECT_FALSE(result); +} + +TEST_F(JniCompilerTest, RunGenericStaticReturnInt) { + TEST_DISABLED_FOR_PORTABLE(); + TEST_DISABLED_FOR_MIPS(); + SetUpForTest(true, "returnInt", "()I", + reinterpret_cast(&Java_MyClassNatives_returnInt), true); + + jint result = env_->CallStaticIntMethod(jklass_, jmethod_); + EXPECT_EQ(42, result); +} + int gJava_MyClassNatives_fooSIOO_calls = 0; jobject Java_MyClassNatives_fooSIOO(JNIEnv* env, jclass klass, jint x, jobject y, jobject z) { diff --git a/runtime/arch/arm/quick_entrypoints_arm.S b/runtime/arch/arm/quick_entrypoints_arm.S index 6c63a1afc..5ab70ea7e 100644 --- a/runtime/arch/arm/quick_entrypoints_arm.S +++ b/runtime/arch/arm/quick_entrypoints_arm.S @@ -1059,14 +1059,13 @@ ENTRY art_quick_generic_jni_trampoline // result sign extension is handled in C code // prepare for artQuickGenericJniEndTrampoline call // (Thread*, result, result_f) - // r0 r1,r2 r3,stack <= C calling convention + // r0 r2,r3 stack <= C calling convention // r11 r0,r1 r0,r1 <= where they are - sub sp, sp, #12 // Stack alignment. + sub sp, sp, #8 // Stack alignment. - push {r1} - mov r3, r0 - mov r2, r1 - mov r1, r0 + push {r0-r1} + mov r3, r1 + mov r2, r0 mov r0, r11 blx artQuickGenericJniEndTrampoline @@ -1083,7 +1082,18 @@ ENTRY art_quick_generic_jni_trampoline cbnz r2, .Lexception_in_native // Tear down the callee-save frame. - RESTORE_REF_AND_ARGS_CALLEE_SAVE_FRAME + add sp, #12 @ rewind sp + // Do not pop r0 and r1, they contain the return value. + pop {r2-r3, r5-r8, r10-r11, lr} @ 9 words of callee saves + .cfi_restore r2 + .cfi_restore r3 + .cfi_restore r5 + .cfi_restore r6 + .cfi_restore r7 + .cfi_restore r8 + .cfi_restore r10 + .cfi_restore r11 + .cfi_adjust_cfa_offset -48 bx lr // ret diff --git a/runtime/arch/x86/quick_entrypoints_x86.S b/runtime/arch/x86/quick_entrypoints_x86.S index dc4019d2a..117738af9 100644 --- a/runtime/arch/x86/quick_entrypoints_x86.S +++ b/runtime/arch/x86/quick_entrypoints_x86.S @@ -1183,10 +1183,10 @@ DEFINE_FUNCTION art_quick_generic_jni_trampoline // prepare for artQuickGenericJniEndTrampoline call // (Thread*, result, result_f) // (esp) 4(esp) 12(esp) <= C calling convention - // fs:... eax:edx xmm0 <= where they are + // fs:... eax:edx fp0 <= where they are subl LITERAL(20), %esp // Padding & pass float result. - movsd %xmm0, (%esp) + fstpl (%esp) pushl %edx // Pass int result. pushl %eax pushl %fs:THREAD_SELF_OFFSET // Pass Thread::Current(). @@ -1211,7 +1211,7 @@ DEFINE_FUNCTION art_quick_generic_jni_trampoline POP ebp // Restore callee saves POP esi POP edi - // store into fpr, for when it's a fpr return... + // Quick expects the return value to be in xmm0. movd %eax, %xmm0 movd %edx, %xmm1 punpckldq %xmm1, %xmm0 diff --git a/runtime/common_runtime_test.h b/runtime/common_runtime_test.h index 12c124127..ddb6c8171 100644 --- a/runtime/common_runtime_test.h +++ b/runtime/common_runtime_test.h @@ -161,6 +161,12 @@ class CheckJniAbortCatcher { return; \ } +#define TEST_DISABLED_FOR_MIPS() \ + if (kRuntimeISA == kMips || kRuntimeISA == kMips64) { \ + printf("WARNING: TEST DISABLED FOR MIPS\n"); \ + return; \ + } + } // namespace art namespace std { diff --git a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc index 4730701f2..dfd2e11fc 100644 --- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc +++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc @@ -1699,7 +1699,15 @@ extern "C" uint64_t artQuickGenericJniEndTrampoline(Thread* self, jvalue result, artQuickGenericJniEndJNINonRef(self, cookie, lock); switch (return_shorty_char) { - case 'F': // Fall-through. + case 'F': { + if (kRuntimeISA == kX86) { + // Convert back the result to float. + double d = bit_cast(result_f); + return bit_cast(static_cast(d)); + } else { + return result_f; + } + } case 'D': return result_f; case 'Z': diff --git a/test/116-nodex2oat/run b/test/116-nodex2oat/run index 5ffeecdbb..2df670575 100755 --- a/test/116-nodex2oat/run +++ b/test/116-nodex2oat/run @@ -17,6 +17,9 @@ # Remove prebuild from the flags, this test is for testing not having oat files. flags="${@/--prebuild/}" +# Use the non-prebuild script. +RUN="${RUN/push-and-run-prebuilt-test-jar/push-and-run-test-jar}" + # Make sure we can run without an oat file, echo "Run -Xnodex2oat" ${RUN} ${flags} --runtime-option -Xnodex2oat diff --git a/test/MyClassNatives/MyClassNatives.java b/test/MyClassNatives/MyClassNatives.java index b5e0204ab..fab153b62 100644 --- a/test/MyClassNatives/MyClassNatives.java +++ b/test/MyClassNatives/MyClassNatives.java @@ -80,16 +80,22 @@ class MyClassNatives { Object o248, Object o249, Object o250, Object o251, Object o252, Object o253); native void withoutImplementation(); - + native static void stackArgsIntsFirst(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int i9, int i10, float f1, float f2, float f3, float f4, float f5, float f6, float f7, float f8, float f9, float f10); - + native static void stackArgsFloatsFirst(float f1, float f2, float f3, float f4, float f5, float f6, float f7, float f8, float f9, float f10, int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int i9, int i10); - + native static void stackArgsMixed(int i1, float f1, int i2, float f2, int i3, float f3, int i4, float f4, int i5, float f5, int i6, float f6, int i7, float f7, int i8, float f8, int i9, float f9, int i10, float f10); + + static native double logD(double d); + static native float logF(float f); + static native boolean returnTrue(); + static native boolean returnFalse(); + static native int returnInt(); } -- 2.11.0