Regarding 8033626: C2: Merging exception state: reexecute=true vs reexecute=false

[Vladimir Kozlov]

Unfortunately it may not be a whole solution. The uncommon trap may 
still needs to be recorded as an exceptions handler for compiled code so 
that when we return from new_instance_C() runtime call with OOM 
exception it will find it. See GraphKit::gen_stub(), 
generate_catch_exception() and 
SharedRuntime::raw_exception_handler_for_return_address()

Also do additional tests with StressCompiledExceptionHandlers otherwise 
we deoptimize immediately on the return from new_instance_C().

In short, please investigate all return paths from new_instance_C() when 
there is pending OOM exception and we have uncommon trap instead of 
CreateExNode.

thanks,
Vladimir

On 6/2/14 12:21 PM, Vladimir Ivanov wrote:
> Yes, uncommon trap should solve the problem.
> Thanks for excellent idea!
>
> Best regards,
> Vladimir Ivanov
>
> On 5/31/14 6:30 AM, Vladimir Kozlov wrote:
>> I thought about this more. I think we should not generate exceptions in
>> intrinsic when we are going to re-execute the method. Interpreter will
>> throw exception if needed and intrinsic has wrong bci anyway (bci of
>> call site and not allocation). *We should generate plain "intrinsic"
>> uncommon trap in such case*.
>> There are GraphKit::is_LibraryCallKit() and is_Parse() methods which
>> allow to check where the request for exception generation is coming from.
>>
>> Regards,
>> Vladimir
>>
>> On 5/29/14 7:00 PM, Vladimir Kozlov wrote:
>>> Now I understand the problem. I think we need to keep separate an
>>> exception code which requires re-execution. Currently
>>> we don't check for should_reexecute() when we merge or search for
>>> existing exception code (we only compare type).
>>>
>>> Vladimir K.
>>>
>>> On 5/29/14 2:14 AM, Vladimir Ivanov wrote:
>>>> The problem is with exception state of two branches [1].
>>>> Exception on fast path comes from GraphKit::new_instance during
>>>> intrinsic construction in LibraryCallKit::inline_native_clone. It is
>>>> executed under should_reexecute=true. PreserveReexecuteState doesn't
>>>> touch exception state, so kit.add_exception_states_from(new_jvms) (@
>>>> callGenerator.cpp:703) tries to accumulate incompatible exceptions from
>>>> fast & slow paths.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> [1] FAST PATH (intrinsic):
>>>>
>>>> JVMS depth=1 loc=5 stk=9 arg=10 mon=14 scalar=14 end=14 mondepth=0 sp=1
>>>> bci=12 reexecute=false method=virtual jobject
>>>>
>>>> Exception state:
>>>>
>>>> JVMS depth=1 loc=5 stk=9 arg=9 mon=14 scalar=14 end=14 mondepth=0 sp=0
>>>> bci=12 reexecute=true method=virtual jobject
>>>>
>>>> SLOW PATH
>>>>
>>>> JVMS depth=1 loc=5 stk=9 arg=10 mon=14 scalar=14 end=14 mondepth=0 sp=1
>>>> bci=12 reexecute=false method=virtual jobject
>>>>
>>>> Exception state:
>>>>
>>>> JVMS depth=1 loc=5 stk=9 arg=9 mon=14 scalar=14 end=14 mondepth=0 sp=0
>>>> bci=12 reexecute=false method=virtual jobject
>>>>
>>>>
>>>> On 5/29/14 2:18 AM, Vladimir Kozlov wrote:
>>>>> PredictedIntrinsicGenerator was added to have a general predicate. For
>>>>> example, AES crypto predicate checks klass of an object pointed by a
>>>>> field. PredictedCallGenerator checks klass of method's holder only.
>>>>>
>>>>> Both cases would have the same problem with reexecute. Using
>>>>> PredictedIntrinsicGenerator will not help you.
>>>>>
>>>>> But you should not have this problem at the merge point, because
>>>>> reexecute should be reset back after intrinsic is generated:
>>>>>
>>>>>    { PreserveReexecuteState preexecs(this);
>>>>>      jvms()->set_should_reexecute(true);
>>>>>
>>>>> It is strange that it is still true after PreserveReexecuteState
>>>>> destructor.
>>>>>
>>>>> Vladimir K
>>>>>
>>>>> On 5/28/14 9:37 AM, Vladimir Ivanov wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I'm looking at 8033626 [1]:
>>>>>> "assert(ex_map->jvms()->same_calls_as(_exceptions->jvms())) failed:
>>>>>> all
>>>>>> collected exceptions must come from the same place".
>>>>>>
>>>>>> 8014447 [2] introduced new code shape for virtual intrinsifiable
>>>>>> methods
>>>>>> (Object::hashCode & Object::clone): "invokevirtual Object.clone()"
>>>>>> can
>>>>>> be represented as a PredictedCallGenerator with LibraryIntrinsic on
>>>>>> fast
>>>>>> path and VirtualCallGenerator on slow path.
>>>>>>
>>>>>> The problem arises for Object::clone, because for OOM case
>>>>>> intrinsified
>>>>>> version requires reexecution (reexecute=true), but slow path (virtual
>>>>>> dispatch) doesn't (reexecute=false). Further attempt to finish the
>>>>>> diamond and merge 2 exception states fires the assertion.
>>>>>>
>>>>>> So, I have 2 questions:
>>>>>>    (1) I don't see any way to merge reexecute=true & reexecute=false
>>>>>> states, so both branches should have the same reexecute status. Am I
>>>>>> missing something?
>>>>>>
>>>>>>    (2) Does PredictedCallGenerator actually support such shape
>>>>>> (intrinsic on fast path)? There are 2 call generators:
>>>>>> PredictedIntrinsicGenerator & PredictedCallGenerator. What's the
>>>>>> difference?
>>>>>>
>>>>>> I see there's a way to customize predicate in
>>>>>> LibraryCallKit::try_to_predicate() for PredictedIntrinsicGenerator,
>>>>>> but
>>>>>> is it the only reason why PredictedIntrinsicGenerator was introduced?
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8033626
>>>>>>
>>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8014447
>>>>>>      "Object.hashCode intrinsic breaks inline caches"