[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 08:44:51 UTC 2014
Thank you, Vladimir!
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.
I added the label.
Best regards,
Zoltan
>
> 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