[9] RFR(S): 8057622: java/util/stream/test/org/openjdk/tests/java/util/stream/InfiniteStreamWithLimitOpTest: SEGV inside compiled code (sparc)
Albert Noll
albert.noll at oracle.com
Tue Nov 4 08:38:38 UTC 2014
Hi,
please see comments inline.
On 11/03/2014 07:00 PM, Vladimir Kozlov wrote:
> 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);
>
An alternative solution is to make a default parameter for control. This
way we can set the control in make() without having to add a new make()
function.
> 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.
>
What you are suggesting is to make a super class (LoadNode) aware of its
subclasses (LoadPNode and LoadKlassNode). From a software engineering
point of view, we should try to avoid doing that. Another solution
would be to add a method (e.g., can_remove_control()) to LoadNode,
LoadPNode, and LoadKlassNode. The function returns 'false' for LoadPNode
and LoadKlassNode; and returns 'true' for LoadNode. What do you think?
Best,
Albert
> 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
>>
More information about the hotspot-compiler-dev
mailing list