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

[David Holmes]

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/

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()) {

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.

---

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

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.

---

+   // 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/


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

Thanks again for tackling this difficult problem!

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