[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
Tue Nov 4 13:04:18 UTC 2014
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.
>> 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?
>>
>> 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.
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