RFR 8079562: Crash in C2 compiled code with STATUS_FLOAT_MULTIPLE_TRAPS

[David Holmes]

Hi Coleen,

So does this:

      setCW( (FPU_DEFAULT | FPU_IEM) & ~EM_ZERODIVIDE );

enable or disable hardware exceptions for FP-div-by-zero?

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.

Sorry. Feel free to ignore me.

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