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. 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.
>>
>>