Report a bug in HotSpot
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/sharedRuntime_x86_32.cpp
> --- a/src/cpu/x86/vm/sharedRuntime_x86_32.cpp Fri Sep 14 15:00:02 2012
> -0700
> +++ b/src/cpu/x86/vm/sharedRuntime_x86_32.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
> - __ verify_FPU(ret_type == T_FLOAT || ret_type == T_DOUBLE ? 1 : 0,
> "native_wrapper 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 (ret_type == T_FLOAT || ret_type == T_DOUBLE) {
> + __ verify_FPU(1, "native_wrapper normal exit");
> + } else {
> + __ empty_FPU_stack();
> + }
> +
> + // Fixup floating point results so that result looks like a return from a
> compiled method
> if (ret_type == T_FLOAT) {
> if (UseSSE >= 1) {
> // Pop st0 and store as float and reload into xmm register
> diff -r fb2d98043048 src/cpu/x86/vm/templateInterpreter_x86_32.cpp
> --- a/src/cpu/x86/vm/templateInterpreter_x86_32.cpp Fri Sep 14 15:00:02
> 2012 -0700
> +++ b/src/cpu/x86/vm/templateInterpreter_x86_32.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.
> + __ empty_FPU_stack();
>
> // change thread state
> __ get_thread(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
>>>> Java_Hello_abc(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