RFR 8079562: Crash in C2 compiled code with STATUS_FLOAT_MULTIPLE_TRAPS

Wed Aug 3 02:48:48 UTC 2016

Hi, David, That would be a better test, I guess.  My goal here is to fix 
this test from failing on windows 32 and salvage something out of the 
existing test.   We could simply delete the test and file an RFE to test 
fpcw better.

On 8/2/16 10:34 PM, David Holmes wrote:
> Hi Coleen,
>
> So does this:
>
>      setCW( (FPU_DEFAULT | FPU_IEM) & ~EM_ZERODIVIDE );
>
> enable or disable hardware exceptions for FP-div-by-zero?

I think it disables hardware exceptions.   I guess Christian's going to 
have to answer this tomorrow.
>
> To be honest I'm not sure this test serves any real point in its 
> current form any more. A general cross-platform test for the 
> AlwaysRestoreFPU flag would be:
>
> static native int getFPUCW();
> static native void setFPUCW(int cw);
>
> test() {
>  int default_cw = getFPUCW();
>  int new_cw = ~default_cw;
>  setFPUCW(new_cw);
>  assert(getFPU_CW() == default_cw, "AlwaysRestoreFPU didn't restore it!);
> }
>
> but that kind of misses the point about triggering unhandled C++ 
> exceptions. And isn't getting to the bottom of the internal error that 
> was seen.
>

No, if that's required, I'll have to unassign myself from the bug, 
quarantine the test, and save it for jdk10, because it might take a long 
time to figure out, if we ever figure it out.  I don't think there's a 
product issue here at least from my reading of the situation.

> Sorry. Feel free to ignore me.
>

We never ignore you, David.

Coleen

> David
>
>
> On 3/08/2016 12:14 PM, Coleen Phillimore wrote:
>>
>>
>> On 8/2/16 9:32 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 3/08/2016 11:16 AM, Coleen Phillimore wrote:
>>>>
>>>>
>>>> On 8/2/16 8:35 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 3/08/2016 8:34 AM, Coleen Phillimore wrote:
>>>>>> Summary: Add AlwaysRestoreFPU and move the test to jtreg.
>>>>>> Contributed-by: myself and christian.tornqvist at oracle.com
>>>>>>
>>>>>> Christian moved the test, I did some cleanup and added
>>>>>> AlwaysRestoreFPU
>>>>>> to it.
>>>>>
>>>>> You modified the original C code such that this comment is no longer
>>>>> valid:
>>>>>
>>>>>   33 // Only valid for specific x86 based systems: linux-x86, or else
>>>>> x86 with fpu_control.h
>>>>
>>>> Thanks, David.  I took out the rest of the comments but missed this 
>>>> one.
>>>>>
>>>>> Not sure why you did that - we were running this test on more than
>>>>> just Windows, despite the naming.
>>>>
>>>> Actually, the #ifdefs were intended to only run on 32 bit linux as 
>>>> well
>>>> as windows, but I don't think the ifdefs were correct, so the test
>>>> always printed that it was skipped on platforms other than 
>>>> windows.  The
>>>> test is only useful on windows.   Modifying the fpcw didn't have an
>>>> effect on linux x86.  Seems silly to run the test on platforms where
>>>> it's not testing anything.
>>>
>>> It's been a while but I've been involved with this test in the past
>>> and I thought it was doing similar testing on other x86 platforms. As
>>> you say the main point is to verify that messing with the FPU control
>>> word doesn't break anything so I'm not sure that is a reason not to
>>> have such a test run on linux.
>>
>> The original point of the test is unknown to me.  The test was doing
>> something that can cause the vm to crash or get the wrong behavior. The
>> new point of the test is to test the AlwaysRestoreFPU flag. Since we can
>> provoke a crash on windows 32 bit only, this seems like a good
>> restriction for the test.
>>>
>>>>>
>>>>> I'm also unclear exactly what changed such that we need to add
>>>>> AlwaysRestoreFPU ??
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8076284 allegedly changed the
>>>> behavior in 2015.
>>>
>>> I certainly can't make any connection between that bug and handling of
>>> the FPU control word. :(
>>>
>>
>> Me neither but the failure I got was from the divide by zero getting a
>> windows internal error, not the C2 generated code.  Rereading the bug,
>> Vladimir thought it could have come from the VC++ compiler change and
>> not the fix.
>>
>>>> Setting floating point control word is something that we don't promise
>>>> to work with generated code or the jvm code, which is why this
>>>> -XX:+AlwaysRestoreFPU flag was added.
>>>
>>> Understood, just puzzled about why this test suddenly requires it.
>>> Given the FPU mode used by the VM has not changed, why does the test
>>> now fail without this??
>>>
>>
>> I don't know why it worked before.  There could have been a library
>> change also that gets the internal error.  It wasn't an unhandled
>> floating point exception.
>>
>>>>>
>>>>> Can you add @Summary info to the Java test (and/or other commentary)
>>>>> as it is far from clear exactly what this is testing.
>>>>>
>>>>
>>>> Yes, I can do that.   How about:
>>>>
>>>>  * @summary Test that modifying the floating point control word 
>>>> doesn't
>>>> cause unhandled Windows floating point exceptions
>>>
>>> That's fine -thanks. Can you also document with a comment exactly what
>>> our modification of the FPU control word is intended to do and how
>>> that relates to the normal FPU mode used by the VM. This is something
>>> I always have to go looking for :)
>>
>> Okay, how about:
>>
>>  * @summary Test that modifying the floating point control word doesn't
>> cause an internal
>>  *          error when turning off floating point exceptions for divide
>> by zero.
>>
>> See new webrev for comments about the default.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8079562.02/webrev
>>
>>
>> Again, adding the flag is the solution that we give customers. We also
>> test for changing the fpcw with -Xcheck:jni.
>>
>> Thanks,
>> Coleen
>>
>>>
>>>>
>>>> Can summary lines be > 1 line or do they have to be one line?
>>>
>>> A tag runs until the next tag is encountered so the summary can span
>>> multiple lines.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8079562.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8079562
>>>>>>
>>>>>> Tested with JPRT.
>>>>>>
>>>>>> thanks,
>>>>>> Coleen
>>>>
>>