[9] RFR(S): 8057622: java/util/stream/test/org/openjdk/tests/java/util/stream/InfiniteStreamWithLimitOpTest: SEGV inside compiled code (sparc)
Zoltán Majó
zoltan.majo at oracle.com
Thu Nov 6 10:22:00 UTC 2014
Thank you, Roland!
Zoltan
On 11/06/2014 11:05 AM, Roland Westrelin wrote:
> For the record, final webrev looks good to me.
>
> Roland.
>
>> On Nov 6, 2014, at 9:45 AM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
>>
>> Thank you, Roland, Vladimir, and Albert, for the reviews!
>>
>>
>> Zoltan
>>
>> On 11/05/2014 06:18 PM, Vladimir Kozlov wrote:
>>> This looks good. Don't forget to add label noreg-sqe since we have tests already.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 11/5/14 5:34 AM, Zoltán Majó wrote:
>>>> Hi Vladimir,
>>>>
>>>>
>>>> thank you for the feedback.
>>>>
>>>> On 11/04/2014 07:40 PM, Vladimir Kozlov wrote:
>>>>> On 11/4/14 5:04 AM, Zoltán Majó wrote:
>>>>>> Hi Roland, Vladimir, and Albert!
>>>>>>
>>>>>>
>>>>>> thank you for your feedback and for your suggestions! Please see comments on specific issues inline.
>>>>>>
>>>>>> On 11/04/2014 09:38 AM, Albert Noll wrote:
>>>>>>> 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.
>>>>>> I took Albert's alternative solution. Now we also set the default value for parameter TypeKlassPtr* tk in
>>>>>> parseHelper.cpp at line 230.
>>>>> As I replied to Albert, I think it should be explicit parameter and not default.
>>>>> Yes, you would need to modify more files but it is better to be explicit I think.
>>>> OK, I see. I updated the code so that we have one LoadKlassNode::make() method, with the control node as second
>>>> parameter. In the new version the control parameter is explicit and we pass in NULL explicitly at all call sites where
>>>> control is not needed.
>>>>
>>>>>>>> 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.
>>>>>> I see, Vladimir. Just a related question: If the address of the klass load were *not* based on a ConP constant, would it
>>>>>> be better (or required) to check with a CheckCastPP the value that was loaded?
>>>>> Yes, you would use CheckCastPP to cast type to exact class you checked for and pin it to 'true' projection of the
>>>>> check. As we do for in gen_checkcast() or other places (speculative types).
>>>>>
>>>>> if (a.class == exact_class)
>>>>> (exact_class)a
>>>> Thank you for the explanation.
>>>>
>>>>> In this case you don't need control on following load.
>>>>>
>>>>>>>> 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?
>>>>>> I think Albert's suggestion has the advantage that if we will ever decide to inherit from LoadKlass, we won't have to
>>>>>> worry about modifying its superclass. So I think it will be good if we go with it.
>>>>> I prefer Albert's name can_remove_control()
>>>> OK, I changed the method's name.
>>>>
>>>> Here is the new webrev: http://cr.openjdk.java.net/~zmajo/8057622/webrev.03/
>>>>
>>>> All JPRT tests and also the failing test cases pass with the new version.
>>>>
>>>> Thank you and best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>> Here is the new webrev: http://cr.openjdk.java.net/~zmajo/8057622/webrev.02/
>>>>>>
>>>>>> JPRT and also the failing test cases pass.
>>>>>>
>>>>>>
>>>>>> Thank you and best regards,
>>>>>>
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>>>> 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