RFR: 8249451: Unconditional exceptions clearing logic in compiler code should honor Async Exceptions

[Jamsheed C M]

Hi David,

Thank you for the review and feedback.

Revised webrev here: http://cr.openjdk.java.net/~jcm/8249451/webrev.02/

On 03/09/2020 05:50, David Holmes wrote:
> Hi Jamsheed,
>
> On 1/09/2020 10:36 pm, Jamsheed C M wrote:
>> Hi David,
>>
>> I reworked the patch, revised webrev here: 
>> http://cr.openjdk.java.net/~jcm/8249451/webrev.01/
>
> Thanks. The new macros and injected field for InternalError look good.
>
> A couple of minor comments below but overall this looks good to me.
>
>> In addition I moved UnlockFlagSaver fs(this) to more local scope.
>>
>> also removed changes done for JDK-8246727, as it will be separately 
>> handled by the bug.
>>
>> Testing: injected and tested async exceptions randomly at compilation 
>> request path and deopt path.
>
> I noticed in deoptimization.cpp that here:
>
> 1965       load_class_by_index(constants, unloaded_class_index, THREAD);
>
> we can now return with a pending async exception and it is unclear 
> whether the code following this will be able to handle that, or indeed 
> whether the caller will be able to handle it. Did you specifically 
> test this site?
>
Yes, I browsed through the code path, it is equipped(let it be c2 only 
UC, or various deopt variant). JVMCI aot code too is equipped to handle 
it( there are forwarding code present at foreign call exit)

> ---
>
> src/hotspot/share/jvmci/jvmciRuntime.cpp
>
> The comment at:
>
>   80 //   1. The pending exception is cleared
>
> should be updated now that asyncs are not cleared.
Done.
>
> ---
>
> src/hotspot/share/compiler/tieredThresholdPolicy.*
>
> The changes from JavaThread* to Thread* look unnecessary for 90% of 
> the cases, but the overall change seems to be dictated by the few 
> methods that do use CHECK*. :( No point agonising over this now as I'm 
> trying to deal with this general problem as a separate RFE - JDK-8252685.
>
Thank you and Best regards,

Jamsheed

> Thanks,
> David
> -----
>
>> Best regards,
>>
>> Jamsheed
>>
>> On 24/08/2020 11:06, Jamsheed C M wrote:
>>> Hi David,
>>>
>>> Thank you for the review and feedback. Agree on all of them. I will 
>>> rework and get back.
>>>
>>> On 10/08/2020 07:33, David Holmes wrote:
>>>> Hi Jamsheed,
>>>>
>>>> On 6/08/2020 10:07 pm, Jamsheed C M wrote:
>>>>> Hi all,
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249451
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~jcm/8249451/webrev.00/
>>>>
>>>> Thanks for tackling this messy issue. Overall I like the use of 
>>>> TRAPS to more clearly document which methods can return with an 
>>>> exception pending. I think there are some problems with the 
>>>> proposed changes. I'll start with those comments and then move on 
>>>> to more general comments.
>>>>
>>>> src/hotspot/share/utilities/exceptions.cpp
>>>> src/hotspot/share/utilities/exceptions.hpp
>>>>
>>>> I don't think the changes here are correct or safe in general.
>>>>
>>>> First, adding the new macro and function to only clear non-async 
>>>> exceptions is fine itself. But naming wise the fact only non-async 
>>>> exceptions are cleared should be evident, and there is no "check" 
>>>> involved (in the sense of the existing CHECK_ macros) so I suggest:
>>>>
>>>> s/CHECK_CLEAR_PENDING_EXCEPTION/CLEAR_PENDING_NONASYNC_EXCEPTIONS/
>>>> s/check_clear_pending_exception/clear_pending_nonasync_exceptions/
>>>>
>>> Ok
>>>> But changing the existing CHECK_AND_CLEAR macros to now leave async 
>>>> exceptions pending seems potentially dangerous as calling code may 
>>>> not be prepared for there to now be a pending exception. For 
>>>> example the use in thread.cpp:
>>>>
>>>>  JDK_Version::set_runtime_name(get_java_runtime_name(THREAD));
>>>>  JDK_Version::set_runtime_version(get_java_runtime_version(THREAD));
>>>>
>>>> get_java_runtime_name() is currently guaranteed to clear all 
>>>> exceptions, so all the other code is known to be safe to call. But 
>>>> that would no longer be true. That said, this is VM initialization 
>>>> code and an async exception is impossible at this stage.
>>>>
>>>> I think I would rather see CHECK_AND_CLEAR left as-is, and an 
>>>> actual CHECK_AND_CLEAR_NONASYNC introduced for those users of 
>>>> CHECK_AND_CLEAR that can encounter async exceptions and which 
>>>> should not clear them.
>>>>
>>>> +   if 
>>>> (!_pending_exception->is_a(SystemDictionary::ThreadDeath_klass()) &&
>>>> +       _pending_exception->klass() != 
>>>> SystemDictionary::InternalError_klass()) {
>>>>
>>> Ok
>>>> Flagging all InternalErrors as async exceptions is probably also 
>>>> not correct. I don't see a good solution to this at the moment. I 
>>>> think we would need to introduce a new subclass of InternalError 
>>>> for the unsafe access error case**. Now it may be that all the 
>>>> other InternalError usages are "impossible" in the context of where 
>>>> the new macros are to be used, but that is very difficult to 
>>>> establish or assert.
>>>>
>>>> ** Or perhaps we could inject a field that allows the VM to 
>>>> identify instances related to unsafe access errors ... Ideally of 
>>>> course these unsafe access errors would be distinct from the async 
>>>> exception mechanism - something I would still like to pursue.
>>>>
>>> Ok
>>>> ---
>>>>
>>>> General comments ...
>>>>
>>>> There is a general change from "JavaThread* thread" to "Thread* 
>>>> THREAD" (or TRAPS) to allow the use of the CHECK macros. This is 
>>>> unfortunate because the fact the thread is restricted to being a 
>>>> JavaThread is no longer evident in the method signatures. That is a 
>>>> flaw with the TRAPS/CHECK mechanism unfortunately :( . But as the 
>>>> methods no longer take a JavaThread* arg, they should assert that 
>>>> THREAD->is_Java_thread(). I will also look at an RFE to have 
>>>> as_JavaThread() to avoid the need for separate assertion checks 
>>>> before casting from "Thread*" to "JavaThread*".
>>>>
>>> Ok
>>>> Note there's no need to use CHECK when the enclosing method is 
>>>> going to return immediately after the call that contains the CHECK. 
>>>> It just adds unnecessary checking of the exception state. The use 
>>>> of TRAPS shows that the methods may return with an exception 
>>>> pending. I've flagged all such occurrences I spotted below.
>>>>
>>> Ok
>>>> ---
>>>>
>>>> +   // Only metaspace OOM is expected. no Java code executed.
>>>>
>>>> Nit: s/no/No
>>>>
>>>>
>>>> src/hotspot/share/compiler/compilationPolicy.cpp
>>>>
>>>>
>>>>  410       method_invocation_event(method, CHECK_NULL);
>>>>  489       CompileBroker::compile_method(m, InvocationEntryBci, 
>>>> comp_level, m, hot_count, CompileTask::Reason_InvocationCount, CHECK);
>>>>
>>>> Nit: there's no need to use CHECK here.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/compiler/tieredThresholdPolicy.cpp
>>>>
>>>>  504     method_invocation_event(method, inlinee, comp_level, nm, 
>>>> CHECK_NULL);
>>>>  570         compile(mh, bci, CompLevel_simple, CHECK);
>>>>  581         compile(mh, bci, CompLevel_simple, CHECK);
>>>>  595     CompileBroker::compile_method(mh, bci, level, mh, 
>>>> hot_count, CompileTask::Reason_Tiered, CHECK);
>>>> 1062       compile(mh, InvocationEntryBci, next_level, CHECK);
>>>>
>>>> Nit: there's no need to use CHECK here.
>>>>
>>>> 814 void TieredThresholdPolicy::create_mdo(const methodHandle& mh, 
>>>> Thread* THREAD) {
>>>>
>>>> Thank you for correcting this misuse of the THREAD name on a 
>>>> JavaThread* type.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/interpreter/linkResolver.cpp
>>>>
>>>>  128 CompilationPolicy::compile_if_required(selected_method, CHECK);
>>>>
>>>> Nit: there's no need to use CHECK here.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/jvmci/compilerRuntime.cpp
>>>>
>>>>  260     CompilationPolicy::policy()->event(emh, mh, 
>>>> InvocationEntryBci, InvocationEntryBci, CompLevel_aot, cm, CHECK);
>>>>  280     nmethod* osr_nm = CompilationPolicy::policy()->event(emh, 
>>>> mh, branch_bci, target_bci, CompLevel_aot, cm, CHECK);
>>>>
>>>> Nit: there's no need to use CHECK here.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>>
>>>>  102         // Donot clear probable async exceptions.
>>>>
>>>> typo: s/Donot/Do not/
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/runtime/deoptimization.cpp
>>>>
>>>> 1686 void Deoptimization::load_class_by_index(const 
>>>> constantPoolHandle& constant_pool, int index) {
>>>>
>>>> This method should be declared with TRAPS now.
>>>>
>>>> 1693     // Donot clear probable Async Exceptions.
>>>>
>>>> typo: s/Donot/Do not/
>>>>
>>>>
>>> Ok
>>>>> testing : mach1-5(links in jbs)
>>>>
>>>> There is very little existing testing that will actually test the 
>>>> key changes you have made here. You will need to do direct 
>>>> fault-injection testing anywhere you now allow async exceptions to 
>>>> remain, to see if the calling code can tolerate that. It will be 
>>>> difficult to test thoroughly.
>>>>
>>> Ok
>>>> Thanks again for tackling this difficult problem!
>>>
>>> Best regards,
>>>
>>> Jamsheed
>>>
>>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> While working on JDK-8246381 it was noticed that compilation 
>>>>> request path clears all exceptions(including async) and doesn't 
>>>>> propagate[1].
>>>>>
>>>>> Fix: patch restores the propagation behavior for the probable 
>>>>> async exceptions.
>>>>>
>>>>> Compilation request path propagate exception as in [2]. MDO and 
>>>>> MethodCounter doesn't expect any exception other than metaspace 
>>>>> OOM(added comments).
>>>>>
>>>>> Deoptimization path doesn't clear probable async exceptions and 
>>>>> take unpack_exception path for non uncommontraps.
>>>>>
>>>>> Added java_lang_InternalError to well known classes.
>>>>>
>>>>> Request for review.
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> Jamsheed
>>>>>
>>>>> [1] w.r.t changes done for JDK-7131259
>>>>>
>>>>> [2]
>>>>>
>>>>>      (a)
>>>>>      -----> 
>>>>> c1_Runtime1.cpp/interpreterRuntime.cpp/compilerRuntime.cpp
>>>>>        |
>>>>>         ----- compilationPolicy.cpp/tieredThresholdPolicy.cpp
>>>>>           |
>>>>>            ------ compileBroker.cpp
>>>>>
>>>>>      (b)
>>>>>      Xcomp versions
>>>>>      ------> compilationPolicy.cpp
>>>>>         |
>>>>>          ------> compileBroker.cpp
>>>>>
>>>>>      (c)
>>>>>
>>>>>      Direct call to  compile_method in compileBroker.cpp
>>>>>
>>>>>      JVMCI bootstrap, whitebox, replayCompile.
>>>>>
>>>>>