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

[Vladimir Kozlov]

I agree with additional check tak != TypeKlassPtr::OBJECT (solution 2).

Instead of creating new LoadKlassNode::make() method you can set control in parseHelper.cpp since it is the only place 
which set it:

     Node* a_e_klass = LoadKlassNode::make(_gvn, immutable_memory(), p2, tak);
     if (always_see_exact_class) {
       a_e_klass->init_req(MemNode::Control, control());
     }
     a_e_klass = _gvn.transform(a_e_klass);

Also an other reason to have control edge in this place is the address of this klass load is based on ConP constant. In 
a normal case after class check you should have CheckCastPP attached to control and used as address's base.

And agree with Roland's suggestion about Op_LoadKlass check.

Thanks,
Vladimir

On 11/3/14 5:58 AM, Zoltán Majó wrote:
> 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
>