RFR 8079562: Crash in C2 compiled code with STATUS_FLOAT_MULTIPLE_TRAPS

[Coleen Phillimore]

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
>>