Report a bug in HotSpot (original) (raw)

Xi Yang hiyangxi at gmail.com
Tue Nov 27 15:28:36 PST 2012

Hi all,

Thanks Kris for forwarding the problem here.

On 28 November 2012 01:59, Krystal Mo <krystal.mo at oracle.com> wrote:

Hi David and Karen,

Thanks for the reply. I don't have a strong opinion on whether this should be viewed as a "bug" and fixed, just forwarding concerns from Xi Yang.

It is hard to say this is bug.

However, I think it is worth to look at this problem because:

  1. x86 ABI does not define this very clearly.

  2. When this problem is happened (bad guy touches MMx register in native code, which causes x87 operations in Java world leading to wrong results), Java keeps silent. So programmer is hard to notice something wrong in the code.

It is very easy to fix the problem or provide a option to allow the user to fix the problem, insert "emms" instructions after back from native to Java world.

Well, there is no problem to say that the problem is from native code and native code should preserve x87 state when using MMx instructions, and then just ignore this. No problem, 100% fine.

Here is the description of "EMMS" instruction I copied from intel arch manual:

"Sets the values of all the tags in the x87 FPU tag word to empty (all 1s). This opera- tion marks the x87 FPU data registers (which are aliased to the MMX technology registers) as available for use by x87 FPU floating-point instructions. (See Figure 8-7 in the Intel® 64 and IA-32 Architectures Software Developer’s Manual, Volume 1, for the format of the x87 FPU tag word.) All other MMX instructions (other than the EMMS instruction) set all the tags in x87 FPU tag word to valid (all 0s). The EMMS instruction must be used to clear the MMX technology state at the end of all MMX technology procedures or subroutines and before calling other procedures or subroutines that may execute x87 floating-point instructions. If a floating-point instruction loads one of the registers in the x87 FPU data register stack before the x87 FPU tag word has been reset by the EMMS instruction, an x87 floating-point register stack overflow can occur that will result in an x87 floating-point exception or incorrect result. EMMS operation is the same in non-64-bit modes and 64-bit mode."

Regards.

Perhaps Xi would like to explain his rationales for having such concerns in the first place. There's a follow up on the test case: Xi provided a modified test case that would affect amd64 as well:

class Hello { private static native void abc(); public static void main(String[] args) { System.out.println("I am main"); System.loadLibrary("hello"); abc(); long a = 100; double b = (double)a; double c = 3.14; System.out.println("Double a%c is " + a%c); } } Even though HotSpot's interpreter doesn't use the FPU stack itself, the "drem" bytecode instruction is implemented with a call to SharedRuntime::drem(double,double), which uses the FPU and thus affected by dirty FPU stack: Dump of assembler code for function ZN13SharedRuntime4dremEdd: 0x00007ffff6e8d790 <+0>: push %rbp 0x00007ffff6e8d791 <+1>: mov %rsp,%rbp 0x00007ffff6e8d794 <+4>: sub $0x10,%rsp 0x00007ffff6e8d798 <+8>: movsd %xmm0,-0x10(%rbp) 0x00007ffff6e8d79d <+13>: fldl -0x10(%rbp) 0x00007ffff6e8d7a0 <+16>: movsd %xmm1,-0x10(%rbp) 0x00007ffff6e8d7a5 <+21>: fldl -0x10(%rbp) 0x00007ffff6e8d7a8 <+24>: fld %st(1) 0x00007ffff6e8d7aa <+26>: fld %st(1) 0x00007ffff6e8d7ac <+28>: fxch %st(1) 0x00007ffff6e8d7ae <+30>: fprem 0x00007ffff6e8d7b0 <+32>: fnstsw %ax 0x00007ffff6e8d7b2 <+34>: test $0x4,%ah 0x00007ffff6e8d7b5 <+37>: jne 0x7ffff6e8d7ae <SharedRuntime::drem(double, double)+30> 0x00007ffff6e8d7b7 <+39>: fstp %st(1) 0x00007ffff6e8d7b9 <+41>: fstpl -0x8(%rbp) 0x00007ffff6e8d7bc <+44>: movsd -0x8(%rbp),%xmm0 0x00007ffff6e8d7c1 <+49>: ucomisd %xmm0,%xmm0 0x00007ffff6e8d7c5 <+53>: jp 0x7ffff6e8d7d0 <SharedRuntime::drem(double, double)+64> 0x00007ffff6e8d7c7 <+55>: jne 0x7ffff6e8d7d0 <SharedRuntime::drem(double, double)+64> 0x00007ffff6e8d7c9 <+57>: fstp %st(0) 0x00007ffff6e8d7cb <+59>: fstp %st(0) 0x00007ffff6e8d7cd <+61>: leaveq 0x00007ffff6e8d7ce <+62>: retq 0x00007ffff6e8d7cf <+63>: nop 0x00007ffff6e8d7d0 <+64>: fstpl -0x10(%rbp) 0x00007ffff6e8d7d3 <+67>: movsd -0x10(%rbp),%xmm1 0x00007ffff6e8d7d8 <+72>: fstpl -0x10(%rbp) 0x00007ffff6e8d7db <+75>: movsd -0x10(%rbp),%xmm0 0x00007ffff6e8d7e0 <+80>: callq 0x7ffff6857fa8 <fmod at plt> 0x00007ffff6e8d7e5 <+85>: leaveq 0x00007ffff6e8d7e6 <+86>: retq (disassembly from JDK7u9 on Ubuntu 12.04, amd64) I had a partial fix to this problem in x86: diff -r fb2d98043048 src/cpu/x86/vm/sharedRuntimex8632.cpp --- a/src/cpu/x86/vm/sharedRuntimex8632.cpp Fri Sep 14 15:00:02 2012 -0700 +++ b/src/cpu/x86/vm/sharedRuntimex8632.cpp Tue Nov 27 20:59:44 2012 +0800 @@ -2054,10 +2054,15 @@ // no exception, we're almost done - // check that only result value is on FPU stack - _ verifyFPU(rettype == TFLOAT || rettype == TDOUBLE ? 1 : 0, "nativewrapper normal exit"); - - // Fixup floating pointer results so that result looks like a return from a compiled method + // Check that only result value is on FPU stack for floating point return type, + // Empty the FPU stack otherwise, just in case it was left dirty by the native call. + if (rettype == TFLOAT || rettype == TDOUBLE) { + _ verifyFPU(1, "nativewrapper normal exit"); + } else { + _ emptyFPUstack(); + } + + // Fixup floating point results so that result looks like a return from a compiled method if (rettype == TFLOAT) { if (UseSSE >= 1) { // Pop st0 and store as float and reload into xmm register diff -r fb2d98043048 src/cpu/x86/vm/templateInterpreterx8632.cpp --- a/src/cpu/x86/vm/templateInterpreterx8632.cpp Fri Sep 14 15:00:02 2012 -0700 +++ b/src/cpu/x86/vm/templateInterpreterx8632.cpp Tue Nov 27 20:59:44 2012 +0800 @@ -1119,6 +1119,10 @@ __ bind(L);_ } __ push(ltos);_ + + // Clear FPU stack here, just in case the native call left it dirty. + // It's safe since the potential result is saved already. + _ emptyFPUstack(); // change thread state __ getthread(thread);_ (diff against tip of hsx/hsx23.6) This patch empties the FPU stack upon return from the native call, in both the native entry for the interpter and the native wrapper for compiled code. It's straightforward to do in the interpreter native entry case, because the FPU stack is supposed to be empty at that point anyway; it's not so straightforward for the native wrapper case, because there's potential return value left in st(0). I could pop st(0) to the stack, empty the FPU stack, and then restore the return value to st(0), but I haven't done so in this patch yet, since it'd be weird to return a floating point result with a dirty FPU stack anyway. Regards, Kris On 11/27/2012 09:01 PM, Karen Kinnear wrote:

Hi Kris, I would concur. And there is also an explicit intention to not slow down JNI transitions by having the JVM do extra work. The model is to have the JNI code restore a good state. Looks like we already have a flag that lets you know that you need to fix the jni code, so no need to add another way to do that. thanks, Karen On Nov 26, 2012, at 10:57 PM, David Holmes wrote:

Hi Kris,

I think historically hotspot assumes/requires JNI code to be well behaved about these things. There have been some well known issues with FPU state in the past. I'm not that familiar with MMX so can't say whether it is reasonable for the VM to know when it has to do this kind of cleanup. David On 27/11/2012 4:29 AM, Krystal Mo wrote:

Hi all, Xi Yang has reported a bug in HotSpot's interpreter that it doesn't empty the FPU stack on return from JNI calls. His mail is included below. e.g. If a native function called via JNI is using MMX registers without emptying the FPU stack before returning, then after returning to Java the FPU stack will be in a bad state. The test case Xi gave is demonstrated here on JDK7u9, x86: https://gist.github.com/4148771 Running the example with -XX:+VerifyFPU shows what's going on. This test case shows the bug affecting 32-bit x86 version of HotSpot's interpreter. Not really familiar with how to file a bug on JBS yet, I'll file a bug to track this after I learn how to do it. Regards, Kris

-------- Original Message -------- Subject: Fwd: Report a bug in HotSpot Date: Tue, 27 Nov 2012 02:02:59 +0800 From: Krystal Mok <rednaxelafx at gmail.com> To: Krystal Mo <krystal.mo at oracle.com>

---------- Forwarded message ---------- From: Xi Yang <hiyangxi at gmail.com> Date: Tue, Nov 20, 2012 at 1:44 PM Subject: Report a bug in HotSpot To: Krystal Mok <rednaxelafx at gmail.com> Hi, It looks like HotSpot does not do "emms" after backing from JNI. Here is the code to show the bug. Would you like to try the newest version? Hello.java class Hello { private static native void abc(); public static void main(String[] args) { System.out.println("I am main"); System.loadLibrary("Hello"); abc(); long a = 100; double b = (double)a; System.out.println("Double a is " + b); } } Hello.c #include <jni.h> #include <stdio.h> JNIEXPORT void JNICALL JavaHelloabc(JNIEnv *env, jclass cls) { printf("I mmmmmmmmmmmmmmmmmmmmmmmmm java Helo world\n"); unsigned int dummy; asm volatile("movd %%mm0, %0\n":"=r"(dummy)); printf("dummy is %x\n", dummy); } gcc -m32 -shared ./Hello.c -o ./libHello.so /opt/jdk1.7.0/bin/java -Djava.library.path=. Hello I am main I mmmmmmmmmmmmmmmmmmmmmmmmm java Helo world dummy is 0 Double a is NaN $ /opt/jdk1.7.0/bin/java -version java version "1.7.0-ea" Java(TM) SE Runtime Environment (build 1.7.0-ea-b93) Java HotSpot(TM) Server VM (build 18.0-b04, mixed mode)

More information about the hotspot-dev mailing list