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

Tue Sep 1 12:36:17 UTC 2020

Hi David,

I reworked the patch, revised webrev here: 
http://cr.openjdk.java.net/~jcm/8249451/webrev.01/

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.

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