JDK-8233193 : Incorrect bailout from possibly_add_compiler_threads
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 11,12,13,14
  • Priority: P5
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-10-30
  • Updated: 2019-12-02
  • Resolved: 2019-11-18
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 14
14 b24Fixed
Related Reports
Relates :  
JDK-8198756 - Lazy allocation of compiler threads
Description
CompileBroker::possibly_add_compiler_threads() contains usage of the macro "CHECK" which contains a return statement (in case of exception).
This should never happen, but if it did, returning would mess things up (e.g. CompileThread_lock doesn't get unlocked).
Comments
URL: https://hg.openjdk.java.net/jdk/jdk/rev/5ac4a49f5399 User: mdoerr Date: 2019-11-18 17:21:43 +0000
18-11-2019
Yes sorry I overlooked the init bailout. I still find this a sloppy/confused approach to exception handling.
11-11-2019
We already do call vm_exit_during_initialization when an exception occurs during init_compiler_sweeper_threads: ExceptionMark destructor calls it as long as is_init_completed is not yet true.
11-11-2019
ILW = Incorrect usage of CHECK macro (should not be a problem with current code), no known issues, no workaround = MLH = P4
04-11-2019
But we won't run into vm_exit_during_initialization we will hit the abort in the EXCEPTION_MARK. If we encounter an exception in in init_compiler_sweeper_threads then we should either explicitly call vm_exit_during_initialization with a useful and clear error message, or else propragate the initialization failure to a higher-level that will call vm_exit_during_initialization. Use of CHECK with make_thread should be replaced with THREAD (and anywhere else an exception is actually impossible.)
30-10-2019
I think this issue is rather cleanup than fixing real problems. The behavior may be ok. When an Exception occurs during initialization, running into vm_exit_during_initialization makes sense to me. What else should we do if the VM can't start as desired? I guess it wouldn't make much sense to continue starting. And as you already mentioned, make_thread doesn't throw Exceptions. Maybe we should assert this explicitly? Using CHECK is basically the same as a guarantee(!HAS_PENDING_EXCEPTION... in possibly_add_compiler_threads where the initialization is completed.
30-10-2019
It isn't just possibly_add_compiler_threads() but also CompileBroker::init_compiler_sweeper_threads(), and possibly other places. The issue is that these methods start with an EXCEPTION_MARK but then also use the CHECK macro. Such a usage doesn't make sense. The EXCEPTION_MARK is used to indicate a code region that should not be entered or exited with an exception pending, else the VM will abort. The CHECK is used to return when an exception becomes pending - thus guaranteeing an abort due to the EXCEPTION_MARK. Exceptions need to be handled correctly and either processed in some way or propagated, but they should not in general be assumed to be fatal errors that must abort the VM. In particular possibly_add_compiler_threads() is allowed to fail so exceptions should be logged and cleared. That said, the problematic line in possibly_add_compiler_threads() is: JavaThread *ct = make_thread(compiler2_object(i), _c2_compile_queue, _compilers[1], CHECK); but as far as I can see make_thread executes no Java code (it may be executed by threads that can't execute Java code!) and so no exception is possible anyway! So the CHECK should really just be THREAD in that case.
30-10-2019