[9] RFR(S): 8057622: java/util/stream/test/org/openjdk/tests/java/util/stream/InfiniteStreamWithLimitOpTest: SEGV inside compiled code (sparc)

[Zoltán Majó]

Hi,


please review the following patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8057622


Problem:


We have five tests that fail with SIGSEGV in our nightlies:

java/util/stream/test/org/openjdk/tests/java/util/stream/InfiniteStreamWithLimitOpTest.java
java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java
java/util/stream/test/org/openjdk/tests/java/util/stream/StreamSpliteratorTest.java
java/util/stream/test/org/openjdk/tests/java/util/stream/StreamBuilderTest.java
java/util/stream/test/org/openjdk/tests/java/util/stream/MapOp.java

All tests fail when executing the C2-compiled version of
java/util/stream/SpinedBuffer at OfPrimitive.inflateSpine():


489: private void inflateSpine() {
490:    if (spine == null) {
491:        spine = newArrayArray(MIN_SPINE_SIZE);
492:        priorElementCount = new long[MIN_SPINE_SIZE];
493:        spine[0] = curChunk;
494:    }
495: }


The failure is due to the 'aastore' at line 493; the failure is caused
by the monomorphic array check optimization
(-XX:+MonomorphicArrayCheck, enabled by default).

In InfiniteStreamWithLimitOpTest.java, C2 determines (based on
profiling information) that inflateSpine() has two hot receiver types,
SpinedBuffer.OfInt and SpinedBuffer.OfDouble:

- in SpinedBuffer.OfInt, the variable 'spine' is of type int[][]

- in SpinedBuffer.ofDouble, the variable 'spine' is of type
   double[][].

Please consider the following pseudo Java code that illustrates how C2
sees SpinedBuffer.OfPrimitive.inflateSpine() after inlining based on
the two hot receiver types SpinedBuffer.OfInt and
SpinedBuffer.OfDouble:


static void inflateSpine(boolean isOfDouble, Object curChunk) {
     Object[] spine = isOfDouble ? new double[8][] : new int[8][];
     spine[0] = curChunk;
}


If MonomorphicArrayCheck is disabled, C2 checks the element type
of 'spine' *at runtime*. The check looks something like:


if (!spine.getClass().getComponentType().isInstance(curChunk)) {
     throw new ArrayStoreException();
}


If MonomorphicArrayCheck is enabled (our case), C2 creates an array
check like this:


if (!TYPE.getClass().getComponentType().isInstance(curChunk)) {
     throw new ArrayStoreException();
}


where TYPE is the type of the 'spine' variable, as it is determined by
C2 *at compile time*. The optimization treats TYPE as a constant and
thus saves a load from memory + an offset calculation (I think).

The problem is that due to 'spine' being either of type int[][] or of
type double[][], C2 determines that TYPE==java/lang/Object.

As a result, when the inflateSpine() method is executed, it first loads the
Klass* corresponding to java/lang/Object (TYPE). Let us call this Klass*
array_klass. Then, the method obtains a Klass* from 
array_klass->_element_klass.
Then, the method reads from offset 20 from array_klass->_element_klass, 
which
results in a SIGSEGV.

The reason for the SIGSEGV is that the Klass* array_klass is of type
java/lang/Object and is therefore represented as an 'InstanceKlass'.
But _element_klass is defined only in objects of type 'ObjArrayKlass'.
The compiler reads array_klass->_element_klass because it expects the
destination of an 'aastore' to be an array, which is not true if
TYPE==java/lang/Object.


Solution:


The compiler already checks if the compile-time type of the array (TYPE) is
the same as the array's type at runtime. If that check fails, the method
branches to an uncommon trap. In our case the SIGSEGV happens because
the load of array_klass->_element_klass "floats" above the check.

I propose two solutions:

Solution 1. Add a control edge between the IfTrue branch of the check and
the load of _array_klass->_element_klass. That prohibits the load of
_array_klass->element_klass floating above the check.

Solution 2. In addition to the changes in Solution 1, the compiler checks if
the compile-time type of the array (TYPE) is java/lang/Object. If
TYPE==java/lang/Object, the compiler should not assume array_type to be
TYPE, but should determine array type at runtime instead.

The reason is for Solution 2 is: We can't do an array store into a 
java/lang/Object.
So, if we see TYPE==java/lang/Object at compile time, it is pretty sure 
that the
method will deoptimize at runtime. Instead of assuming TYPE to be 
java/lang/Object,
we read at runtime the type of the array store's destination. That still 
gives us some
chance that the compiled version of the method will successfully 
execute. The
additional check is in parseHelper.cpp in Parse::array_store_check() on 
line 171.


Webrev:

Solution 1: http://cr.openjdk.java.net/~zmajo/8057622/webrev.00/
Solution 2: http://cr.openjdk.java.net/~zmajo/8057622/webrev.01/


Testing (for both solutions):

JPRT, manual testing with failing test cases.


Please note that both solutions are somewhat "hacky", because that 
allowed me to
produce a solution within reasonable time and with reasonable effort. 
Any suggestions
or feedback is welcome.


Thank you and best regards,


Zoltan