[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
Wed Nov 5 13:34:34 UTC 2014
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